| Message ID | 20191105194317.16232-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] xen/livepatch: Add a return value to load hooks | expand |
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@citrix.com> > --- > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Ross Lagerwall <ross.lagerwall@citrix.com> > CC: Juergen Gross <jgross@suse.com> > > 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 >
On 05.11.2019 20:43, Andrew Cooper wrote: > --- 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); Is there a possible problem here if some of the load_funcs() succeeded before one fails? Or are those required to not do any state change to the system (which would need rolling back)? Jan
On 11/5/19 7:43 PM, 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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
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);
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> CC: Juergen Gross <jgross@suse.com> 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(-)