Re: [Xen-devel] [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Aug 05, 2016 at 09:35:49AM -0600, Jan Beulich wrote:
> >>> On 04.08.16 at 17:49, <konrad.wilk@xxxxxxxxxx> wrote:
> > In general, the hooks provide flexibility when having to deal with
> > unforeseen cases, but their application should be rarely required (<
> > 10%)."
> 
> But the greater flexibility of course comes with increased chances
> of screwing things up. I'm therefore still not entirely convinced that
> such XSAs wouldn't then better not be live patched.
> 
> > --- a/xen/arch/x86/test/xen_hello_world.c
> > +++ b/xen/arch/x86/test/xen_hello_world.c
> > @@ -4,14 +4,50 @@
> >   */
> >  
> >  #include "config.h"
> > +#include <xen/lib.h>
> >  #include <xen/types.h>
> >  #include <xen/version.h>
> >  #include <xen/livepatch.h>
> > +#include <xen/livepatch_payload.h>
> >  
> >  #include <public/sysctl.h>
> >  
> >  static char hello_world_patch_this_fnc[] = "xen_extra_version";
> >  extern const char *xen_hello_world(void);
> > +static unsigned int cnt;
> > +
> > +static void apply_hook(void)
> > +{
> > +    printk(KERN_DEBUG "Hook executing.\n");
> > +}
> > +
> > +static void revert_hook(void)
> > +{
> > +    printk(KERN_DEBUG "Hook unloaded.\n");
> > +}
> > +
> > +static void hi_func(void)
> > +{
> > +    printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
> > +};
> > +
> > +/* If we are sorted we _MUST_ be the last .livepatch.hook section. */
> > +static void Z_check_fnc(void)
> 
> And that Z_ prefix is supposed to guarantee that being last? Isn't
> it that upper case letters sort before lower case ones?

Yes. And the code is actually flexible enough not to be the last one.

B/c:
> 
> > +{
> > +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
> > +    BUG_ON(cnt == 0 || cnt > 2);
> > +    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 
> > 4! */
> 
> Isn't this an indication of a general weakness: Shouldn't a patch
> module be allowed to assume it's being run in a clean state, i.e.
> without any of its static data altered from their load time values?

Of this bug. (which I obviously need to fix).
> 
> > @@ -70,7 +71,11 @@ struct payload {
> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
> > symbols. */
> >      struct livepatch_build_id id;        /* 
> > ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
> >      struct livepatch_build_id dep;       /* 
> > ELFNOTE_DESC(.livepatch.depends). */
> > -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
> > +    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call 
> > after */
> > +    livepatch_unloadcall_t **unload_funcs;/* load and unload of the 
> > payload. */
> 
> These both seem like they want a const in the middle.

I did that initially and found out that the calling ->load_funcs[i] resulted
in _no_ code being called.

I did not dig in the assembler output to figure out what happend, let me
do that now.


> 
> > @@ -515,6 +520,27 @@ static int prepare_payload(struct payload *payload,
> >          }
> >      }
> >  
> > +    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
> 
> Is the .hooks infix really needed?
> 
> > +    if ( sec )
> > +    {
> > +        if ( !sec->sec->sh_size ||
> 
> What's wrong with a zero size section (holding no hooks)? We've
> had that same case elsewhere in the original series ...

That would be a bit odd (having zero size).

But yes there is nothing wrong with it.  I will fix it up.

> 
> > @@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
> >  
> >      arch_livepatch_quiesce();
> >  
> > +    /*
> > +     * The hooks may call common code which expects spinlocks to be certain
> > +     * type, as such disable this temporarily.
> > +     */
> > +    spin_debug_disable();
> > +    for ( i = 0; i < data->n_load_funcs; i++ )
> > +        data->load_funcs[i]();
> > +    spin_debug_enable();
> 
> I have to admit that I can't really make sense of the comment, and
> hence the code.

Perhaps:

Since we are running with IRQs disabled and the hooks may call common
code - which expects the spinlocks to run with IRQs enabled - we temporarly
disable the spin locks "IRQ affinity state"

?
> 
> > --- /dev/null
> > +++ b/xen/include/xen/livepatch_payload.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + * Copyright (C) 2016 Citrix Systems R&D Ltd.
> > + */
> > +
> > +#ifndef __XEN_LIVEPATCH_PAYLOAD_H__
> > +#define __XEN_LIVEPATCH_PAYLOAD_H__
> > +
> > +/*
> > + * The following definitions are to be used in patches. They are taken
> > + * from kpatch.
> > + */
> > +typedef void livepatch_loadcall_t(void);
> > +typedef void livepatch_unloadcall_t(void);
> > +
> > +/*
> > + * LIVEPATCH_LOAD_HOOK macro
> > + *
> > + * Declares a function pointer to be allocated in a new
> > + * .livepatch.hook.load section.  This livepatch_load_data symbol is later
> > + * stripped by create-diff-object so that it can be declared in multiple
> > + * objects that are later linked together, avoiding global symbol
> > + * collision.  Since multiple hooks can be registered, the
> > + * .livepatch.hook.load section is a table of functions that will be
> > + * executed in series by the livepatch infrastructure at patch load time.
> > + */
> > +#define LIVEPATCH_LOAD_HOOK(_fn) \
> > +    livepatch_loadcall_t *__attribute__((weak)) \
> > +        livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
> 
> There's again a const desirable here, to render the section r/o.

<nods> Let me dig in this. I can't recall when I initially tried
making the livepatch_loadcall_t be const whether I had this as const or not..

Thank you for your quick review!
> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel