Re: [PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Aug 23, 2016 at 10:22:12PM -0400, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Add hook functions which run during patch apply and patch revert.
> Hook functions are used by livepatch payloads to manipulate data
> structures during patching, etc.
> 
> One use case is the XSA91. As Martin mentions it:
> "If we have shadow variables, we also need an unload hook to garbage
> collect all the variables introduced by a hotpatch to prevent memory
> leaks.  Potentially, we also want to pre-reserve memory for static or
> existing dynamic objects in the load-hook instead of on the fly.
> 
> For testing and debugging, various applications are possible.
> 
> In general, the hooks provide flexibility when having to deal with
> unforeseen cases, but their application should be rarely required (<
> 10%)."
> 
> Furthermore include a test-case for it.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


So... anybody willing to review it :-)

> 
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>


In regards to this going in v4.8 my recollection is that:

 George: 0
 Andrew: +1
 Jan: 0 (with a slight leaning towards -1)?

I think that means folks are OK or 'don't care'.

And the livepatch maintainers:
 Ross: +1 (obviously since he wrote it)
 Konrad: +1

> 
> v0.4..v0.11: Defered for v4.8
> v0.12: s/xsplice/livepatch/
> 
> v3: Clarify the comments about spin_debug_enable
>     Rename one of the hooks to lower-case (Z->z) to guarantee it being
>     called last.
>     Learned a lot of about 'const'
> v4: Do the actual const of the hooks.
>     Remove the requirement for the tests-case hooks to be sorted
>     (they never were to start with).
>     Fix up the comment about spin_debug_enable so more.
> ---
>  docs/misc/livepatch.markdown        | 23 +++++++++++++++++
>  xen/arch/x86/test/xen_hello_world.c | 34 +++++++++++++++++++++++++
>  xen/common/livepatch.c              | 50 ++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 155 insertions(+), 1 deletion(-)
>  create mode 100644 xen/include/xen/livepatch_payload.h
> 
> diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
> index ad0df26..9f52aeb 100644
> --- a/docs/misc/livepatch.markdown
> +++ b/docs/misc/livepatch.markdown
> @@ -343,6 +343,13 @@ When reverting a patch, the hypervisor iterates over each `livepatch_func`
>  and the core code copies the data from the undo buffer (private internal copy)
>  to `old_addr`.
>  
> +It optionally may contain the address of functions to be called right before
> +being applied and after being reverted:
> +
> + * `.livepatch.hooks.load` - an array of function pointers.
> + * `.livepatch.hooks.unload` - an array of function pointers.
> +
> +
>  ### Example of .livepatch.funcs
>  
>  A simple example of what a payload file can be:
> @@ -380,6 +387,22 @@ struct livepatch_func livepatch_hello_world = {
>  
>  Code must be compiled with -fPIC.
>  
> +### .livepatch.hooks.load and .livepatch.hooks.unload
> +
> +This section contains an array of function pointers to be executed
> +before payload is being applied (.livepatch.funcs) or after reverting
> +the payload. This is useful to prepare data structures that need to
> +be modified patching.
> +
> +Each entry in this array is eight bytes.
> +
> +The type definition of the function are as follow:
> +
> +<pre>
> +typedef void (*livepatch_loadcall_t)(void);  
> +typedef void (*livepatch_unloadcall_t)(void);   
> +</pre>
> +
>  ### .livepatch.depends and .note.gnu.build-id
>  
>  To support dependencies checking and safe loading (to load the
> diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
> index 422bdf1..cb5e27a 100644
> --- a/xen/arch/x86/test/xen_hello_world.c
> +++ b/xen/arch/x86/test/xen_hello_world.c
> @@ -4,14 +4,48 @@
>   */
>  
>  #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);
> +};
> +
> +static void check_fnc(void)
> +{
> +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
> +    BUG_ON(cnt == 0 || cnt > 2);
> +}
> +
> +LIVEPATCH_LOAD_HOOK(apply_hook);
> +LIVEPATCH_UNLOAD_HOOK(revert_hook);
> +
> +/* Imbalance here. Two load and three unload. */
> +
> +LIVEPATCH_LOAD_HOOK(hi_func);
> +LIVEPATCH_UNLOAD_HOOK(hi_func);
> +
> +LIVEPATCH_UNLOAD_HOOK(check_fnc);
>  
>  struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
>      .version = LIVEPATCH_PAYLOAD_VERSION,
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index dd24a07..528c0c9 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -23,6 +23,7 @@
>  #include <xen/wait.h>
>  #include <xen/livepatch_elf.h>
>  #include <xen/livepatch.h>
> +#include <xen/livepatch_payload.h>
>  
>  #include <asm/event.h>
>  
> @@ -73,7 +74,11 @@ struct payload {
>      void **bss;                          /* .bss's of the payload. */
>      size_t *bss_size;                    /* and their sizes. */
>      size_t n_bss;                        /* Size of the array. */
> -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
> +    livepatch_loadcall_t *const *load_funcs;   /* The array of funcs to call after */
> +    livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
> +    unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
> +    unsigned int n_unload_funcs;         /* Nr of funcs to call durung unload. */
> +    char name[XEN_LIVEPATCH_NAME_SIZE];  /* Name of it. */
>  };
>  
>  /* Defines an outstanding patching action. */
> @@ -583,6 +588,25 @@ static int prepare_payload(struct payload *payload,
>              return rc;
>      }
>  
> +    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
> +    if ( sec )
> +    {
> +        if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
> +            return -EINVAL;
> +
> +        payload->load_funcs = sec->load_addr;
> +        payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
> +    }
> +
> +    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
> +    if ( sec )
> +    {
> +        if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
> +            return -EINVAL;
> +
> +        payload->unload_funcs = sec->load_addr;
> +        payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
> +    }
>      sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
>      if ( sec )
>      {
> @@ -1071,6 +1095,18 @@ static int apply_payload(struct payload *data)
>  
>      arch_livepatch_quiesce();
>  
> +    /*
> +     * Since we are running with IRQs disabled and the hooks may call common
> +     * code - which expects certain spinlocks to run with IRQs enabled - we
> +     * temporarily disable the spin locks IRQ state checks.
> +     */
> +    spin_debug_disable();
> +    for ( i = 0; i < data->n_load_funcs; i++ )
> +        data->load_funcs[i]();
> +    spin_debug_enable();
> +
> +    ASSERT(!local_irq_is_enabled());
> +
>      for ( i = 0; i < data->nfuncs; i++ )
>          arch_livepatch_apply_jmp(&data->funcs[i]);
>  
> @@ -1097,6 +1133,18 @@ static int revert_payload(struct payload *data)
>      for ( i = 0; i < data->nfuncs; i++ )
>          arch_livepatch_revert_jmp(&data->funcs[i]);
>  
> +    /*
> +     * Since we are running with IRQs disabled and the hooks may call common
> +     * code - which expects certain spinlocks to run with IRQs enabled - we
> +     * temporarily disable the spin locks IRQ state checks.
> +     */
> +    spin_debug_disable();
> +    for ( i = 0; i < data->n_unload_funcs; i++ )
> +        data->unload_funcs[i]();
> +    spin_debug_enable();
> +
> +    ASSERT(!local_irq_is_enabled());
> +
>      arch_livepatch_revive();
>  
>      /*
> diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
> new file mode 100644
> index 0000000..8f38cc2
> --- /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)) \
> +        const livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
> +
> +/*
> + * LIVEPATCH_UNLOAD_HOOK macro
> + *
> + * Same as LOAD hook with s/load/unload/
> + */
> +#define LIVEPATCH_UNLOAD_HOOK(_fn) \
> +     livepatch_unloadcall_t *__attribute__((weak)) \
> +        const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
> +
> +#endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.4.11
> 

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