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);
+    revert_payload_tail(payload);
+    local_irq_restore(flags);
 
     printk(KERN_DEBUG "%s: Hook done.\n", __func__);
 }
-- 
2.43.0

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.