On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger....@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....@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
>
