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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +{
> +    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?

> @@ -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.

> @@ -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 ...

> @@ -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.

> --- /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.

Jan

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