Re: [Xen-devel] [PATCH 1/2] xen/livepatch: Add a return value to load hooks

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

On Tue, Nov 05, 2019 at 07:43:16PM +0000, Andrew Cooper wrote:
> One use of load hooks is to perform a safety check of the system in its
> quiesced state.  Any non-zero return value from a load hook will abort the
> apply attempt.
> 

Shouldn't you also update the documentation (design doc?)

Thanks.
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> 
> For several years, the following patch in the series has been shipped in every
> XenServer livepatch, implemented as a void load hook which modifies its return
> address to skip to the end of apply_payload().  This method is distinctly less
> hacky.
> ---
>  xen/common/livepatch.c               | 30 +++++++++++++++++++-----------
>  xen/include/xen/livepatch_payload.h  |  2 +-
>  xen/test/livepatch/xen_hello_world.c | 12 +++++++++---
>  3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 7caa30c202..962647616a 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1076,25 +1076,33 @@ static int apply_payload(struct payload *data)
>       * temporarily disable the spin locks IRQ state checks.
>       */
>      spin_debug_disable();
> -    for ( i = 0; i < data->n_load_funcs; i++ )
> -        data->load_funcs[i]();
> +    for ( i = 0; !rc && i < data->n_load_funcs; i++ )
> +        rc = data->load_funcs[i]();
>      spin_debug_enable();
>  
> +    if ( rc )
> +        printk(XENLOG_ERR LIVEPATCH "%s: load_funcs[%u] failed: %d\n",
> +               data->name, i, rc);
> +
>      ASSERT(!local_irq_is_enabled());
>  
> -    for ( i = 0; i < data->nfuncs; i++ )
> -        arch_livepatch_apply(&data->funcs[i]);
> +    if ( !rc )
> +        for ( i = 0; i < data->nfuncs; i++ )
> +            arch_livepatch_apply(&data->funcs[i]);
>  
>      arch_livepatch_revive();
>  
> -    /*
> -     * We need RCU variant (which has barriers) in case we crash here.
> -     * The applied_list is iterated by the trap code.
> -     */
> -    list_add_tail_rcu(&data->applied_list, &applied_list);
> -    register_virtual_region(&data->region);
> +    if ( !rc )
> +    {
> +        /*
> +         * We need RCU variant (which has barriers) in case we crash here.
> +         * The applied_list is iterated by the trap code.
> +         */
> +        list_add_tail_rcu(&data->applied_list, &applied_list);
> +        register_virtual_region(&data->region);
> +    }
>  
> -    return 0;
> +    return rc;
>  }
>  
>  static int revert_payload(struct payload *data)
> diff --git a/xen/include/xen/livepatch_payload.h 
> b/xen/include/xen/livepatch_payload.h
> index 4a1a96d054..3788b52ddf 100644
> --- a/xen/include/xen/livepatch_payload.h
> +++ b/xen/include/xen/livepatch_payload.h
> @@ -9,7 +9,7 @@
>   * The following definitions are to be used in patches. They are taken
>   * from kpatch.
>   */
> -typedef void livepatch_loadcall_t(void);
> +typedef int livepatch_loadcall_t(void);
>  typedef void livepatch_unloadcall_t(void);
>  
>  /*
> diff --git a/xen/test/livepatch/xen_hello_world.c 
> b/xen/test/livepatch/xen_hello_world.c
> index 02f3f85dc0..d720001f07 100644
> --- a/xen/test/livepatch/xen_hello_world.c
> +++ b/xen/test/livepatch/xen_hello_world.c
> @@ -16,9 +16,11 @@ static const 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)
> +static int apply_hook(void)
>  {
>      printk(KERN_DEBUG "Hook executing.\n");
> +
> +    return 0;
>  }
>  
>  static void revert_hook(void)
> @@ -26,10 +28,14 @@ static void revert_hook(void)
>      printk(KERN_DEBUG "Hook unloaded.\n");
>  }
>  
> -static void  hi_func(void)
> +static int hi_func(void)
>  {
>      printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
> +
> +    return 0;
>  };
> +/* Safe to cast away the return value for this trivial case. */
> +void (void_hi_func)(void) __attribute__((alias("hi_func")));
>  
>  static void check_fnc(void)
>  {
> @@ -43,7 +49,7 @@ 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(void_hi_func);
>  
>  LIVEPATCH_UNLOAD_HOOK(check_fnc);
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel