| Message ID | 20231130142944.46322-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/livepatch: fixes for the pre-apply / post-revert hooks | expand |
On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > The purpose of the norevert test is to install a dummy handler that replaces > the internal Xen revert code, and then perform the revert in the post-revert > hook. For that purpose the usage of the previous common_livepatch_revert() is > not enough, as that just reverts specific functions, but not the whole state of > the payload. > > Remove both common_livepatch_{apply,revert}() and instead expose > revert_payload{,_tail}() in order to perform the patch revert from the > post-revert hook. > > Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/common/livepatch.c | 41 +++++++++++++++++-- > xen/include/xen/livepatch.h | 32 ++------------- > .../livepatch/xen_action_hooks_norevert.c | 22 +++------- > 3 files changed, 46 insertions(+), 49 deletions(-) > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index 4e775be66571..d81f3d11d655 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -1367,7 +1367,22 @@ static int apply_payload(struct payload *data) > ASSERT(!local_irq_is_enabled()); > > for ( i = 0; i < data->nfuncs; i++ ) > - common_livepatch_apply(&data->funcs[i], &data->fstate[i]); > + { > + const struct livepatch_func *func = &data->funcs[i]; > + struct livepatch_fstate *state = &data->fstate[i]; > + > + /* If the action has been already executed on this function, do nothing. */ > + if ( state->applied == LIVEPATCH_FUNC_APPLIED ) > + { > + printk(XENLOG_WARNING LIVEPATCH > + "%s: %s has been already applied before\n", > + __func__, func->name); > + continue; > + } > + > + arch_livepatch_apply(func, state); > + state->applied = LIVEPATCH_FUNC_APPLIED; > + } > > arch_livepatch_revive(); > > @@ -1383,7 +1398,7 @@ static inline void apply_payload_tail(struct payload *data) > data->state = LIVEPATCH_STATE_APPLIED; > } > > -static int revert_payload(struct payload *data) > +int revert_payload(struct payload *data) > { > unsigned int i; > int rc; > @@ -1398,7 +1413,25 @@ static int revert_payload(struct payload *data) > } > > for ( i = 0; i < data->nfuncs; i++ ) > - common_livepatch_revert(&data->funcs[i], &data->fstate[i]); > + { > + const struct livepatch_func *func = &data->funcs[i]; > + struct livepatch_fstate *state = &data->fstate[i]; > + > + /* > + * If the apply action hasn't been executed on this function, do > + * nothing. > + */ > + if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED ) > + { > + printk(XENLOG_WARNING LIVEPATCH > + "%s: %s has not been applied before\n", > + __func__, func->name); > + continue; > + } > + > + arch_livepatch_revert(func, state); > + state->applied = LIVEPATCH_FUNC_NOT_APPLIED; > + } > > /* > * Since we are running with IRQs disabled and the hooks may call common > @@ -1416,7 +1449,7 @@ static int revert_payload(struct payload *data) > return 0; > } > > -static inline void revert_payload_tail(struct payload *data) > +void revert_payload_tail(struct payload *data) > { > list_del(&data->applied_list); > > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h > index df339a134e40..9da8b6939878 100644 > --- a/xen/include/xen/livepatch.h > +++ b/xen/include/xen/livepatch.h > @@ -136,35 +136,11 @@ void arch_livepatch_post_action(void); > void arch_livepatch_mask(void); > void arch_livepatch_unmask(void); > > -static inline void common_livepatch_apply(const struct livepatch_func *func, > - struct livepatch_fstate *state) > -{ > - /* If the action has been already executed on this function, do nothing. */ > - if ( state->applied == LIVEPATCH_FUNC_APPLIED ) > - { > - printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", > - __func__, func->name); > - return; > - } > - > - arch_livepatch_apply(func, state); > - state->applied = LIVEPATCH_FUNC_APPLIED; > -} > +/* Only for testing purposes. */ > +struct payload; > +int revert_payload(struct payload *data); > +void revert_payload_tail(struct payload *data); > > -static inline void common_livepatch_revert(const struct livepatch_func *func, > - struct livepatch_fstate *state) > -{ > - /* If the apply action hasn't been executed on this function, do nothing. */ > - if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED ) > - { > - printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", > - __func__, func->name); > - return; > - } > - > - arch_livepatch_revert(func, state); > - state->applied = LIVEPATCH_FUNC_NOT_APPLIED; > -} > #else > > /* > diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c > index 3e21ade6abfc..074f8e1d56ce 100644 > --- a/xen/test/livepatch/xen_action_hooks_norevert.c > +++ b/xen/test/livepatch/xen_action_hooks_norevert.c > @@ -96,26 +96,14 @@ static int revert_hook(livepatch_payload_t *payload) > > static void post_revert_hook(livepatch_payload_t *payload) > { > - int i; > + unsigned long flags; > > printk(KERN_DEBUG "%s: Hook starting.\n", __func__); > > - for (i = 0; i < payload->nfuncs; i++) > - { > - const struct livepatch_func *func = &payload->funcs[i]; > - struct livepatch_fstate *fstate = &payload->fstate[i]; > - > - BUG_ON(revert_cnt != 1); > - BUG_ON(fstate->applied != LIVEPATCH_FUNC_APPLIED); > - > - /* Outside of quiesce zone: MAY TRIGGER HOST CRASH/UNDEFINED BEHAVIOR */ > - arch_livepatch_quiesce(); > - common_livepatch_revert(payload); > - arch_livepatch_revive(); > - BUG_ON(fstate->applied == LIVEPATCH_FUNC_APPLIED); > - > - printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name); > - } > + local_irq_save(flags); > + revert_payload(payload); Should this check or assert the return value of revert_payload()? Ross > + revert_payload_tail(payload); > + local_irq_restore(flags); > > printk(KERN_DEBUG "%s: Hook done.\n", __func__); > } > -- > 2.43.0 >
On Fri, Feb 23, 2024 at 10:10:38AM +0000, Ross Lagerwall wrote: > On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c > > index 3e21ade6abfc..074f8e1d56ce 100644 > > --- a/xen/test/livepatch/xen_action_hooks_norevert.c > > +++ b/xen/test/livepatch/xen_action_hooks_norevert.c > > @@ -96,26 +96,14 @@ static int revert_hook(livepatch_payload_t *payload) > > > > static void post_revert_hook(livepatch_payload_t *payload) > > { > > - int i; > > + unsigned long flags; > > > > printk(KERN_DEBUG "%s: Hook starting.\n", __func__); > > > > - for (i = 0; i < payload->nfuncs; i++) > > - { > > - const struct livepatch_func *func = &payload->funcs[i]; > > - struct livepatch_fstate *fstate = &payload->fstate[i]; > > - > > - BUG_ON(revert_cnt != 1); > > - BUG_ON(fstate->applied != LIVEPATCH_FUNC_APPLIED); > > - > > - /* Outside of quiesce zone: MAY TRIGGER HOST CRASH/UNDEFINED BEHAVIOR */ > > - arch_livepatch_quiesce(); > > - common_livepatch_revert(payload); > > - arch_livepatch_revive(); > > - BUG_ON(fstate->applied == LIVEPATCH_FUNC_APPLIED); > > - > > - printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name); > > - } > > + local_irq_save(flags); > > + revert_payload(payload); > > Should this check or assert the return value of revert_payload()? Hm, yes, why not. I will put this inside of a BUG_ON(), as post-revert hook doesn't return any value. Thanks, Roger.
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 4e775be66571..d81f3d11d655 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -1367,7 +1367,22 @@ static int apply_payload(struct payload *data) ASSERT(!local_irq_is_enabled()); for ( i = 0; i < data->nfuncs; i++ ) - common_livepatch_apply(&data->funcs[i], &data->fstate[i]); + { + const struct livepatch_func *func = &data->funcs[i]; + struct livepatch_fstate *state = &data->fstate[i]; + + /* If the action has been already executed on this function, do nothing. */ + if ( state->applied == LIVEPATCH_FUNC_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH + "%s: %s has been already applied before\n", + __func__, func->name); + continue; + } + + arch_livepatch_apply(func, state); + state->applied = LIVEPATCH_FUNC_APPLIED; + } arch_livepatch_revive(); @@ -1383,7 +1398,7 @@ static inline void apply_payload_tail(struct payload *data) data->state = LIVEPATCH_STATE_APPLIED; } -static int revert_payload(struct payload *data) +int revert_payload(struct payload *data) { unsigned int i; int rc; @@ -1398,7 +1413,25 @@ static int revert_payload(struct payload *data) } for ( i = 0; i < data->nfuncs; i++ ) - common_livepatch_revert(&data->funcs[i], &data->fstate[i]); + { + const struct livepatch_func *func = &data->funcs[i]; + struct livepatch_fstate *state = &data->fstate[i]; + + /* + * If the apply action hasn't been executed on this function, do + * nothing. + */ + if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH + "%s: %s has not been applied before\n", + __func__, func->name); + continue; + } + + arch_livepatch_revert(func, state); + state->applied = LIVEPATCH_FUNC_NOT_APPLIED; + } /* * Since we are running with IRQs disabled and the hooks may call common @@ -1416,7 +1449,7 @@ static int revert_payload(struct payload *data) return 0; } -static inline void revert_payload_tail(struct payload *data) +void revert_payload_tail(struct payload *data) { list_del(&data->applied_list); diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index df339a134e40..9da8b6939878 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -136,35 +136,11 @@ void arch_livepatch_post_action(void); void arch_livepatch_mask(void); void arch_livepatch_unmask(void); -static inline void common_livepatch_apply(const struct livepatch_func *func, - struct livepatch_fstate *state) -{ - /* If the action has been already executed on this function, do nothing. */ - if ( state->applied == LIVEPATCH_FUNC_APPLIED ) - { - printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", - __func__, func->name); - return; - } - - arch_livepatch_apply(func, state); - state->applied = LIVEPATCH_FUNC_APPLIED; -} +/* Only for testing purposes. */ +struct payload; +int revert_payload(struct payload *data); +void revert_payload_tail(struct payload *data); -static inline void common_livepatch_revert(const struct livepatch_func *func, - struct livepatch_fstate *state) -{ - /* If the apply action hasn't been executed on this function, do nothing. */ - if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED ) - { - printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", - __func__, func->name); - return; - } - - arch_livepatch_revert(func, state); - state->applied = LIVEPATCH_FUNC_NOT_APPLIED; -} #else /* diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c index 3e21ade6abfc..074f8e1d56ce 100644 --- a/xen/test/livepatch/xen_action_hooks_norevert.c +++ b/xen/test/livepatch/xen_action_hooks_norevert.c @@ -96,26 +96,14 @@ static int revert_hook(livepatch_payload_t *payload) static void post_revert_hook(livepatch_payload_t *payload) { - int i; + unsigned long flags; printk(KERN_DEBUG "%s: Hook starting.\n", __func__); - for (i = 0; i < payload->nfuncs; i++) - { - const struct livepatch_func *func = &payload->funcs[i]; - struct livepatch_fstate *fstate = &payload->fstate[i]; - - BUG_ON(revert_cnt != 1); - BUG_ON(fstate->applied != LIVEPATCH_FUNC_APPLIED); - - /* Outside of quiesce zone: MAY TRIGGER HOST CRASH/UNDEFINED BEHAVIOR */ - arch_livepatch_quiesce(); - common_livepatch_revert(payload); - arch_livepatch_revive(); - BUG_ON(fstate->applied == LIVEPATCH_FUNC_APPLIED); - - printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name); - } + local_irq_save(flags); + revert_payload(payload); + revert_payload_tail(payload); + local_irq_restore(flags); printk(KERN_DEBUG "%s: Hook done.\n", __func__); }
The purpose of the norevert test is to install a dummy handler that replaces the internal Xen revert code, and then perform the revert in the post-revert hook. For that purpose the usage of the previous common_livepatch_revert() is not enough, as that just reverts specific functions, but not the whole state of the payload. Remove both common_livepatch_{apply,revert}() and instead expose revert_payload{,_tail}() in order to perform the patch revert from the post-revert hook. Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/livepatch.c | 41 +++++++++++++++++-- xen/include/xen/livepatch.h | 32 ++------------- .../livepatch/xen_action_hooks_norevert.c | 22 +++------- 3 files changed, 46 insertions(+), 49 deletions(-)