[PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[Roger Pau Monne]

Hello,

The follow series contain a misc of fixes mostly related to the usage of
the pre-apply / post-revert hooks.  The norevert test is also fixed to
work as I think was expected.  Finally both the no{apply,revert}
tests are fixed to build properly, as the files where previously
unhooked from the build system completely.

I'm unsure how useful the apply and revert hooks really are, as without
calling the internal apply/revert functions the state of the payload
structure is quite likely inconsistent with the code expectations.

Thanks, Roger.

Roger Pau Monne (5):
  xen/livepatch: register livepatch regions when loaded
  xen/livepatch: search for symbols in all loaded payloads
  xen/livepatch: fix norevert test attempt to open-code revert
  xen/livepatch: fix norevert test hook setup typo
  xen/livepatch: properly build the noapply and norevert tests

 xen/common/livepatch.c                        | 95 +++++++++++--------
 xen/common/virtual_region.c                   | 40 +++-----
 xen/include/xen/livepatch.h                   | 32 +------
 xen/test/livepatch/Makefile                   |  4 +-
 .../livepatch/xen_action_hooks_norevert.c     | 24 ++---
 5 files changed, 81 insertions(+), 114 deletions(-)

-- 
2.43.0

[PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Roger Pau Monne]

Currently livepatch regions are registered as virtual regions only after the
livepatch has been applied.

This can lead to issues when using the pre-apply or post-revert hooks, as at
the point the livepatch is not in the virtual regions list.  If a livepatch
pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
code to handle the bug frame won't be able to find the instruction pointer that
triggered the #UD in any of the registered virtual regions, and hence crash.

Fix this by adding the livepatch payloads as virtual regions as soon as loaded,
and only remove them once the payload is unloaded.  This requires some changes
to the virtual regions code, as the removal of the virtual regions is no longer
done in stop machine context, and hence an RCU barrier is added in order to
make sure there are no users of the virtual region after it's been removed from
the list.

Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/livepatch.c      |  5 +++--
 xen/common/virtual_region.c | 40 +++++++++++--------------------------
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 1209fea2566c..3199432f11f5 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
         }
     }
 
+    register_virtual_region(region);
+
     return 0;
 }
 
@@ -1071,6 +1073,7 @@ static int build_symbol_table(struct payload *payload,
 static void free_payload(struct payload *data)
 {
     ASSERT(spin_is_locked(&payload_lock));
+    unregister_virtual_region(&data->region);
     list_del(&data->list);
     payload_cnt--;
     payload_version++;
@@ -1386,7 +1389,6 @@ static inline void apply_payload_tail(struct payload *data)
      * The applied_list is iterated by the trap code.
      */
     list_add_tail_rcu(&data->applied_list, &applied_list);
-    register_virtual_region(&data->region);
 
     data->state = LIVEPATCH_STATE_APPLIED;
 }
@@ -1432,7 +1434,6 @@ static inline void revert_payload_tail(struct payload *data)
      * The applied_list is iterated by the trap code.
      */
     list_del_rcu(&data->applied_list);
-    unregister_virtual_region(&data->region);
 
     data->reverted = true;
     data->state = LIVEPATCH_STATE_CHECKED;
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index 5f89703f513b..b444253848cf 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -23,14 +23,8 @@ static struct virtual_region core_init __initdata = {
 };
 
 /*
- * RCU locking. Additions are done either at startup (when there is only
- * one CPU) or when all CPUs are running without IRQs.
- *
- * Deletions are bit tricky. We do it when Live Patch (all CPUs running
- * without IRQs) or during bootup (when clearing the init).
- *
- * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
- * on deletion.
+ * RCU locking. Modifications to the list must be done in exclusive mode, and
+ * hence need to hold the spinlock.
  *
  * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
  */
@@ -58,38 +52,28 @@ const struct virtual_region *find_text_region(unsigned long addr)
 
 void register_virtual_region(struct virtual_region *r)
 {
-    ASSERT(!local_irq_is_enabled());
+    unsigned long flags;
 
+    spin_lock_irqsave(&virtual_region_lock, flags);
     list_add_tail_rcu(&r->list, &virtual_region_list);
+    spin_unlock_irqrestore(&virtual_region_lock, flags);
 }
 
 static void remove_virtual_region(struct virtual_region *r)
 {
-    unsigned long flags;
+     unsigned long flags;
 
-    spin_lock_irqsave(&virtual_region_lock, flags);
-    list_del_rcu(&r->list);
-    spin_unlock_irqrestore(&virtual_region_lock, flags);
-    /*
-     * We do not need to invoke call_rcu.
-     *
-     * This is due to the fact that on the deletion we have made sure
-     * to use spinlocks (to guard against somebody else calling
-     * unregister_virtual_region) and list_deletion spiced with
-     * memory barrier.
-     *
-     * That protects us from corrupting the list as the readers all
-     * use list_for_each_entry_rcu which is safe against concurrent
-     * deletions.
-     */
+     spin_lock_irqsave(&virtual_region_lock, flags);
+     list_del_rcu(&r->list);
+     spin_unlock_irqrestore(&virtual_region_lock, flags);
 }
 
 void unregister_virtual_region(struct virtual_region *r)
 {
-    /* Expected to be called from Live Patch - which has IRQs disabled. */
-    ASSERT(!local_irq_is_enabled());
-
     remove_virtual_region(r);
+
+    /* Assert that no CPU might be using the removed region. */
+    rcu_barrier();
 }
 
 #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_X86)
-- 
2.43.0

[PATCH 2/5] xen/livepatch: search for symbols in all loaded payloads

[Roger Pau Monne]

When checking if an address belongs to a patch, or when resolving a symbol,
take into account all loaded livepatch payloads, even if not applied.

This is required in order for the pre-apply and post-revert hooks to work
properly, or else Xen won't detect the intruction pointer belonging to those
hooks as being part of the currently active text.

Move the RCU handling to be used for payload_list instead of applied_list, as
now the calls from trap code will iterate over the payload_list.

Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/livepatch.c | 49 +++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 3199432f11f5..4e775be66571 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -36,13 +36,14 @@
  * caller in schedule_work.
  */
 static DEFINE_SPINLOCK(payload_lock);
-static LIST_HEAD(payload_list);
-
 /*
- * Patches which have been applied. Need RCU in case we crash (and then
- * traps code would iterate via applied_list) when adding entries on the list.
+ * Need RCU in case we crash (and then traps code would iterate via
+ * payload_list) when adding entries on the list.
  */
-static DEFINE_RCU_READ_LOCK(rcu_applied_lock);
+static DEFINE_RCU_READ_LOCK(rcu_payload_lock);
+static LIST_HEAD(payload_list);
+
+/* Patches which have been applied. Only modified from stop machine context. */
 static LIST_HEAD(applied_list);
 
 static unsigned int payload_cnt;
@@ -111,12 +112,8 @@ bool is_patch(const void *ptr)
     const struct payload *data;
     bool r = false;
 
-    /*
-     * Only RCU locking since this list is only ever changed during apply
-     * or revert context. And in case it dies there we need an safe list.
-     */
-    rcu_read_lock(&rcu_applied_lock);
-    list_for_each_entry_rcu ( data, &applied_list, applied_list )
+    rcu_read_lock(&rcu_payload_lock);
+    list_for_each_entry_rcu ( data, &payload_list, list )
     {
         if ( (ptr >= data->rw_addr &&
               ptr < (data->rw_addr + data->rw_size)) ||
@@ -130,7 +127,7 @@ bool is_patch(const void *ptr)
         }
 
     }
-    rcu_read_unlock(&rcu_applied_lock);
+    rcu_read_unlock(&rcu_payload_lock);
 
     return r;
 }
@@ -166,12 +163,8 @@ static const char *cf_check livepatch_symbols_lookup(
     const void *va = (const void *)addr;
     const char *n = NULL;
 
-    /*
-     * Only RCU locking since this list is only ever changed during apply
-     * or revert context. And in case it dies there we need an safe list.
-     */
-    rcu_read_lock(&rcu_applied_lock);
-    list_for_each_entry_rcu ( data, &applied_list, applied_list )
+    rcu_read_lock(&rcu_payload_lock);
+    list_for_each_entry_rcu ( data, &payload_list, list )
     {
         if ( va < data->text_addr ||
              va >= (data->text_addr + data->text_size) )
@@ -200,7 +193,7 @@ static const char *cf_check livepatch_symbols_lookup(
         n = data->symtab[best].name;
         break;
     }
-    rcu_read_unlock(&rcu_applied_lock);
+    rcu_read_unlock(&rcu_payload_lock);
 
     return n;
 }
@@ -1074,7 +1067,8 @@ static void free_payload(struct payload *data)
 {
     ASSERT(spin_is_locked(&payload_lock));
     unregister_virtual_region(&data->region);
-    list_del(&data->list);
+    list_del_rcu(&data->list);
+    rcu_barrier();
     payload_cnt--;
     payload_version++;
     free_payload_data(data);
@@ -1173,7 +1167,7 @@ static int livepatch_upload(struct xen_sysctl_livepatch_upload *upload)
         INIT_LIST_HEAD(&data->list);
         INIT_LIST_HEAD(&data->applied_list);
 
-        list_add_tail(&data->list, &payload_list);
+        list_add_tail_rcu(&data->list, &payload_list);
         payload_cnt++;
         payload_version++;
     }
@@ -1384,11 +1378,7 @@ static int apply_payload(struct payload *data)
 
 static inline void apply_payload_tail(struct payload *data)
 {
-    /*
-     * 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);
+    list_add_tail(&data->applied_list, &applied_list);
 
     data->state = LIVEPATCH_STATE_APPLIED;
 }
@@ -1428,12 +1418,7 @@ static int revert_payload(struct payload *data)
 
 static inline void revert_payload_tail(struct payload *data)
 {
-
-    /*
-     * We need RCU variant (which has barriers) in case we crash here.
-     * The applied_list is iterated by the trap code.
-     */
-    list_del_rcu(&data->applied_list);
+    list_del(&data->applied_list);
 
     data->reverted = true;
     data->state = LIVEPATCH_STATE_CHECKED;
-- 
2.43.0

[PATCH 3/5] xen/livepatch: fix norevert test attempt to open-code revert

[Roger Pau Monne]

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

[PATCH 4/5] xen/livepatch: fix norevert test hook setup typo

[Roger Pau Monne]

The test code has a typo in using LIVEPATCH_APPLY_HOOK() instead of
LIVEPATCH_REVERT_HOOK().

Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/test/livepatch/xen_action_hooks_norevert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c
index 074f8e1d56ce..cdfff156cede 100644
--- a/xen/test/livepatch/xen_action_hooks_norevert.c
+++ b/xen/test/livepatch/xen_action_hooks_norevert.c
@@ -108,7 +108,7 @@ static void post_revert_hook(livepatch_payload_t *payload)
     printk(KERN_DEBUG "%s: Hook done.\n", __func__);
 }
 
-LIVEPATCH_APPLY_HOOK(revert_hook);
+LIVEPATCH_REVERT_HOOK(revert_hook);
 
 LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
 LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
-- 
2.43.0

[PATCH 5/5] xen/livepatch: properly build the noapply and norevert tests

[Roger Pau Monne]

It seems the build variables for those tests where copy-pasted from
xen_action_hooks_marker-objs and not adjusted to use the correct source files.

Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/test/livepatch/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index c258ab0b5940..d987a8367f15 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -118,12 +118,12 @@ xen_action_hooks_marker-objs := xen_action_hooks_marker.o xen_hello_world_func.o
 $(obj)/xen_action_hooks_noapply.o: $(obj)/config.h
 
 extra-y += xen_action_hooks_noapply.livepatch
-xen_action_hooks_noapply-objs := xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+xen_action_hooks_noapply-objs := xen_action_hooks_noapply.o xen_hello_world_func.o note.o xen_note.o
 
 $(obj)/xen_action_hooks_norevert.o: $(obj)/config.h
 
 extra-y += xen_action_hooks_norevert.livepatch
-xen_action_hooks_norevert-objs := xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+xen_action_hooks_norevert-objs := xen_action_hooks_norevert.o xen_hello_world_func.o note.o xen_note.o
 
 EXPECT_BYTES_COUNT := 8
 CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=1 $(1) | sed -n -e '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(EXPECT_BYTES_COUNT) | awk '{$$0=$$2; printf "%s", substr($$0,length-1)}' | sed 's/.\{2\}/0x&,/g' | sed 's/^/{/;s/,$$/}/g')
-- 
2.43.0

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Jan Beulich]

On 30.11.2023 15:29, Roger Pau Monne wrote:
> Currently livepatch regions are registered as virtual regions only after the
> livepatch has been applied.
> 
> This can lead to issues when using the pre-apply or post-revert hooks, as at
> the point the livepatch is not in the virtual regions list.  If a livepatch
> pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
> code to handle the bug frame won't be able to find the instruction pointer that
> triggered the #UD in any of the registered virtual regions, and hence crash.
> 
> Fix this by adding the livepatch payloads as virtual regions as soon as loaded,
> and only remove them once the payload is unloaded.  This requires some changes
> to the virtual regions code, as the removal of the virtual regions is no longer
> done in stop machine context, and hence an RCU barrier is added in order to
> make sure there are no users of the virtual region after it's been removed from
> the list.
> 
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/common/livepatch.c      |  5 +++--
>  xen/common/virtual_region.c | 40 +++++++++++--------------------------
>  2 files changed, 15 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 1209fea2566c..3199432f11f5 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
>          }
>      }
>  
> +    register_virtual_region(region);
> +
>      return 0;
>  }
>  
> @@ -1071,6 +1073,7 @@ static int build_symbol_table(struct payload *payload,
>  static void free_payload(struct payload *data)
>  {
>      ASSERT(spin_is_locked(&payload_lock));
> +    unregister_virtual_region(&data->region);
>      list_del(&data->list);
>      payload_cnt--;
>      payload_version++;
> @@ -1386,7 +1389,6 @@ static inline void apply_payload_tail(struct payload *data)
>       * The applied_list is iterated by the trap code.
>       */
>      list_add_tail_rcu(&data->applied_list, &applied_list);
> -    register_virtual_region(&data->region);
>  
>      data->state = LIVEPATCH_STATE_APPLIED;
>  }
> @@ -1432,7 +1434,6 @@ static inline void revert_payload_tail(struct payload *data)
>       * The applied_list is iterated by the trap code.
>       */
>      list_del_rcu(&data->applied_list);
> -    unregister_virtual_region(&data->region);
>  
>      data->reverted = true;
>      data->state = LIVEPATCH_STATE_CHECKED;
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index 5f89703f513b..b444253848cf 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -23,14 +23,8 @@ static struct virtual_region core_init __initdata = {
>  };
>  
>  /*
> - * RCU locking. Additions are done either at startup (when there is only
> - * one CPU) or when all CPUs are running without IRQs.
> - *
> - * Deletions are bit tricky. We do it when Live Patch (all CPUs running
> - * without IRQs) or during bootup (when clearing the init).
> - *
> - * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
> - * on deletion.
> + * RCU locking. Modifications to the list must be done in exclusive mode, and
> + * hence need to hold the spinlock.
>   *
>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>   */
> @@ -58,38 +52,28 @@ const struct virtual_region *find_text_region(unsigned long addr)
>  
>  void register_virtual_region(struct virtual_region *r)
>  {
> -    ASSERT(!local_irq_is_enabled());
> +    unsigned long flags;
>  
> +    spin_lock_irqsave(&virtual_region_lock, flags);
>      list_add_tail_rcu(&r->list, &virtual_region_list);
> +    spin_unlock_irqrestore(&virtual_region_lock, flags);
>  }
>  
>  static void remove_virtual_region(struct virtual_region *r)
>  {
> -    unsigned long flags;
> +     unsigned long flags;

Nit: Stray blank added?

> -    spin_lock_irqsave(&virtual_region_lock, flags);
> -    list_del_rcu(&r->list);
> -    spin_unlock_irqrestore(&virtual_region_lock, flags);
> -    /*
> -     * We do not need to invoke call_rcu.
> -     *
> -     * This is due to the fact that on the deletion we have made sure
> -     * to use spinlocks (to guard against somebody else calling
> -     * unregister_virtual_region) and list_deletion spiced with
> -     * memory barrier.
> -     *
> -     * That protects us from corrupting the list as the readers all
> -     * use list_for_each_entry_rcu which is safe against concurrent
> -     * deletions.
> -     */
> +     spin_lock_irqsave(&virtual_region_lock, flags);
> +     list_del_rcu(&r->list);
> +     spin_unlock_irqrestore(&virtual_region_lock, flags);
>  }
>  
>  void unregister_virtual_region(struct virtual_region *r)
>  {
> -    /* Expected to be called from Live Patch - which has IRQs disabled. */
> -    ASSERT(!local_irq_is_enabled());
> -
>      remove_virtual_region(r);
> +
> +    /* Assert that no CPU might be using the removed region. */
> +    rcu_barrier();
>  }

rcu_barrier() is a relatively heavy operation aiui. Seeing ...

>  #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_X86)

... this I'd like to ask to consider hiding {,un}register_virtual_region()
in "#ifdef CONFIG_LIVEPATCH" as well, to make clear these aren't supposed
to be used for other purpose. Would at the same time address two Misra
violations, I think (both functions having no callers when !LIVEPATCH).

Jan

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Roger Pau Monné]

On Tue, Dec 05, 2023 at 02:47:56PM +0100, Jan Beulich wrote:
> On 30.11.2023 15:29, Roger Pau Monne wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index 5f89703f513b..b444253848cf 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -23,14 +23,8 @@ static struct virtual_region core_init __initdata = {
> >  };
> >  
> >  /*
> > - * RCU locking. Additions are done either at startup (when there is only
> > - * one CPU) or when all CPUs are running without IRQs.
> > - *
> > - * Deletions are bit tricky. We do it when Live Patch (all CPUs running
> > - * without IRQs) or during bootup (when clearing the init).
> > - *
> > - * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
> > - * on deletion.
> > + * RCU locking. Modifications to the list must be done in exclusive mode, and
> > + * hence need to hold the spinlock.
> >   *
> >   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
> >   */
> > @@ -58,38 +52,28 @@ const struct virtual_region *find_text_region(unsigned long addr)
> >  
> >  void register_virtual_region(struct virtual_region *r)
> >  {
> > -    ASSERT(!local_irq_is_enabled());
> > +    unsigned long flags;
> >  
> > +    spin_lock_irqsave(&virtual_region_lock, flags);
> >      list_add_tail_rcu(&r->list, &virtual_region_list);
> > +    spin_unlock_irqrestore(&virtual_region_lock, flags);
> >  }
> >  
> >  static void remove_virtual_region(struct virtual_region *r)
> >  {
> > -    unsigned long flags;
> > +     unsigned long flags;
> 
> Nit: Stray blank added?

Oh, my bad.

> > -    spin_lock_irqsave(&virtual_region_lock, flags);
> > -    list_del_rcu(&r->list);
> > -    spin_unlock_irqrestore(&virtual_region_lock, flags);
> > -    /*
> > -     * We do not need to invoke call_rcu.
> > -     *
> > -     * This is due to the fact that on the deletion we have made sure
> > -     * to use spinlocks (to guard against somebody else calling
> > -     * unregister_virtual_region) and list_deletion spiced with
> > -     * memory barrier.
> > -     *
> > -     * That protects us from corrupting the list as the readers all
> > -     * use list_for_each_entry_rcu which is safe against concurrent
> > -     * deletions.
> > -     */
> > +     spin_lock_irqsave(&virtual_region_lock, flags);
> > +     list_del_rcu(&r->list);
> > +     spin_unlock_irqrestore(&virtual_region_lock, flags);
> >  }
> >  
> >  void unregister_virtual_region(struct virtual_region *r)
> >  {
> > -    /* Expected to be called from Live Patch - which has IRQs disabled. */
> > -    ASSERT(!local_irq_is_enabled());
> > -
> >      remove_virtual_region(r);
> > +
> > +    /* Assert that no CPU might be using the removed region. */
> > +    rcu_barrier();
> >  }
> 
> rcu_barrier() is a relatively heavy operation aiui. Seeing ...
> 
> >  #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_X86)
> 
> ... this I'd like to ask to consider hiding {,un}register_virtual_region()
> in "#ifdef CONFIG_LIVEPATCH" as well, to make clear these aren't supposed
> to be used for other purpose. Would at the same time address two Misra
> violations, I think (both functions having no callers when !LIVEPATCH).

That's fine, I can do it this same patch unless you prefer such
adjustment to be in a separate change.

Thanks, Roger.

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Jan Beulich]

On 05.12.2023 16:04, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 02:47:56PM +0100, Jan Beulich wrote:
>> On 30.11.2023 15:29, Roger Pau Monne wrote:
>>> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
>>> index 5f89703f513b..b444253848cf 100644
>>> --- a/xen/common/virtual_region.c
>>> +++ b/xen/common/virtual_region.c
>>> @@ -23,14 +23,8 @@ static struct virtual_region core_init __initdata = {
>>>  };
>>>  
>>>  /*
>>> - * RCU locking. Additions are done either at startup (when there is only
>>> - * one CPU) or when all CPUs are running without IRQs.
>>> - *
>>> - * Deletions are bit tricky. We do it when Live Patch (all CPUs running
>>> - * without IRQs) or during bootup (when clearing the init).
>>> - *
>>> - * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
>>> - * on deletion.
>>> + * RCU locking. Modifications to the list must be done in exclusive mode, and
>>> + * hence need to hold the spinlock.
>>>   *
>>>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>>>   */
>>> @@ -58,38 +52,28 @@ const struct virtual_region *find_text_region(unsigned long addr)
>>>  
>>>  void register_virtual_region(struct virtual_region *r)
>>>  {
>>> -    ASSERT(!local_irq_is_enabled());
>>> +    unsigned long flags;
>>>  
>>> +    spin_lock_irqsave(&virtual_region_lock, flags);
>>>      list_add_tail_rcu(&r->list, &virtual_region_list);
>>> +    spin_unlock_irqrestore(&virtual_region_lock, flags);
>>>  }
>>>  
>>>  static void remove_virtual_region(struct virtual_region *r)
>>>  {
>>> -    unsigned long flags;
>>> +     unsigned long flags;
>>
>> Nit: Stray blank added?
> 
> Oh, my bad.
> 
>>> -    spin_lock_irqsave(&virtual_region_lock, flags);
>>> -    list_del_rcu(&r->list);
>>> -    spin_unlock_irqrestore(&virtual_region_lock, flags);
>>> -    /*
>>> -     * We do not need to invoke call_rcu.
>>> -     *
>>> -     * This is due to the fact that on the deletion we have made sure
>>> -     * to use spinlocks (to guard against somebody else calling
>>> -     * unregister_virtual_region) and list_deletion spiced with
>>> -     * memory barrier.
>>> -     *
>>> -     * That protects us from corrupting the list as the readers all
>>> -     * use list_for_each_entry_rcu which is safe against concurrent
>>> -     * deletions.
>>> -     */
>>> +     spin_lock_irqsave(&virtual_region_lock, flags);
>>> +     list_del_rcu(&r->list);
>>> +     spin_unlock_irqrestore(&virtual_region_lock, flags);
>>>  }
>>>  
>>>  void unregister_virtual_region(struct virtual_region *r)
>>>  {
>>> -    /* Expected to be called from Live Patch - which has IRQs disabled. */
>>> -    ASSERT(!local_irq_is_enabled());
>>> -
>>>      remove_virtual_region(r);
>>> +
>>> +    /* Assert that no CPU might be using the removed region. */
>>> +    rcu_barrier();
>>>  }
>>
>> rcu_barrier() is a relatively heavy operation aiui. Seeing ...
>>
>>>  #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_X86)
>>
>> ... this I'd like to ask to consider hiding {,un}register_virtual_region()
>> in "#ifdef CONFIG_LIVEPATCH" as well, to make clear these aren't supposed
>> to be used for other purpose. Would at the same time address two Misra
>> violations, I think (both functions having no callers when !LIVEPATCH).
> 
> That's fine, I can do it this same patch unless you prefer such
> adjustment to be in a separate change.

Since the change itself constitutes at least part of the reason for the
adjustment, this would be fine with me. (A separate change, if preferred
by others, would still be fine, too.)

Jan

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> Currently livepatch regions are registered as virtual regions only after the
> livepatch has been applied.
>
> This can lead to issues when using the pre-apply or post-revert hooks, as at
> the point the livepatch is not in the virtual regions list.  If a livepatch
> pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
> code to handle the bug frame won't be able to find the instruction pointer that
> triggered the #UD in any of the registered virtual regions, and hence crash.
>
> Fix this by adding the livepatch payloads as virtual regions as soon as loaded,
> and only remove them once the payload is unloaded.  This requires some changes
> to the virtual regions code, as the removal of the virtual regions is no longer
> done in stop machine context, and hence an RCU barrier is added in order to
> make sure there are no users of the virtual region after it's been removed from
> the list.
>
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/common/livepatch.c      |  5 +++--
>  xen/common/virtual_region.c | 40 +++++++++++--------------------------
>  2 files changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 1209fea2566c..3199432f11f5 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
>          }
>      }
>
> +    register_virtual_region(region);
> +
>      return 0;
>  }
>

The region is registered in prepare_payload() but if e.g. the build id
check in load_payload_data() fails, it won't unregister the region
since the failure path calls free_payload_data(), not free_payload().
Perhaps the call to register_virtual_region() could be done as the last
thing in load_payload_data()?

Ross

Re: [PATCH 3/5] xen/livepatch: fix norevert test attempt to open-code revert

[Ross Lagerwall]

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
>

Re: [PATCH 2/5] xen/livepatch: search for symbols in all loaded payloads

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> When checking if an address belongs to a patch, or when resolving a symbol,
> take into account all loaded livepatch payloads, even if not applied.
>
> This is required in order for the pre-apply and post-revert hooks to work
> properly, or else Xen won't detect the intruction pointer belonging to those
> hooks as being part of the currently active text.
>
> Move the RCU handling to be used for payload_list instead of applied_list, as
> now the calls from trap code will iterate over the payload_list.
>
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 4/5] xen/livepatch: fix norevert test hook setup typo

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:31 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> The test code has a typo in using LIVEPATCH_APPLY_HOOK() instead of
> LIVEPATCH_REVERT_HOOK().
>
> Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/test/livepatch/xen_action_hooks_norevert.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c
> index 074f8e1d56ce..cdfff156cede 100644
> --- a/xen/test/livepatch/xen_action_hooks_norevert.c
> +++ b/xen/test/livepatch/xen_action_hooks_norevert.c
> @@ -108,7 +108,7 @@ static void post_revert_hook(livepatch_payload_t *payload)
>      printk(KERN_DEBUG "%s: Hook done.\n", __func__);
>  }
>
> -LIVEPATCH_APPLY_HOOK(revert_hook);
> +LIVEPATCH_REVERT_HOOK(revert_hook);
>
>  LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
>  LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
> --

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 5/5] xen/livepatch: properly build the noapply and norevert tests

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:31 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> It seems the build variables for those tests where copy-pasted from
> xen_action_hooks_marker-objs and not adjusted to use the correct source files.
>
> Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state tracking marker')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/test/livepatch/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
> index c258ab0b5940..d987a8367f15 100644
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -118,12 +118,12 @@ xen_action_hooks_marker-objs := xen_action_hooks_marker.o xen_hello_world_func.o
>  $(obj)/xen_action_hooks_noapply.o: $(obj)/config.h
>
>  extra-y += xen_action_hooks_noapply.livepatch
> -xen_action_hooks_noapply-objs := xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
> +xen_action_hooks_noapply-objs := xen_action_hooks_noapply.o xen_hello_world_func.o note.o xen_note.o
>
>  $(obj)/xen_action_hooks_norevert.o: $(obj)/config.h
>
>  extra-y += xen_action_hooks_norevert.livepatch
> -xen_action_hooks_norevert-objs := xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
> +xen_action_hooks_norevert-objs := xen_action_hooks_norevert.o xen_hello_world_func.o note.o xen_note.o
>
>  EXPECT_BYTES_COUNT := 8
>  CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=1 $(1) | sed -n -e '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(EXPECT_BYTES_COUNT) | awk '{$$0=$$2; printf "%s", substr($$0,length-1)}' | sed 's/.\{2\}/0x&,/g' | sed 's/^/{/;s/,$$/}/g')
> --

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[Jan Beulich]

On 30.11.2023 15:29, Roger Pau Monne wrote:
> Hello,
> 
> The follow series contain a misc of fixes mostly related to the usage of
> the pre-apply / post-revert hooks.  The norevert test is also fixed to
> work as I think was expected.  Finally both the no{apply,revert}
> tests are fixed to build properly, as the files where previously
> unhooked from the build system completely.
> 
> I'm unsure how useful the apply and revert hooks really are, as without
> calling the internal apply/revert functions the state of the payload
> structure is quite likely inconsistent with the code expectations.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (5):
>   xen/livepatch: register livepatch regions when loaded
>   xen/livepatch: search for symbols in all loaded payloads
>   xen/livepatch: fix norevert test attempt to open-code revert
>   xen/livepatch: fix norevert test hook setup typo
>   xen/livepatch: properly build the noapply and norevert tests

With the R-b-s that have arrived, could you clarify in how far these
can be committed out of order?

Jan

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 09:45:15AM +0000, Ross Lagerwall wrote:
> On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > Currently livepatch regions are registered as virtual regions only after the
> > livepatch has been applied.
> >
> > This can lead to issues when using the pre-apply or post-revert hooks, as at
> > the point the livepatch is not in the virtual regions list.  If a livepatch
> > pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
> > code to handle the bug frame won't be able to find the instruction pointer that
> > triggered the #UD in any of the registered virtual regions, and hence crash.
> >
> > Fix this by adding the livepatch payloads as virtual regions as soon as loaded,
> > and only remove them once the payload is unloaded.  This requires some changes
> > to the virtual regions code, as the removal of the virtual regions is no longer
> > done in stop machine context, and hence an RCU barrier is added in order to
> > make sure there are no users of the virtual region after it's been removed from
> > the list.
> >
> > Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/common/livepatch.c      |  5 +++--
> >  xen/common/virtual_region.c | 40 +++++++++++--------------------------
> >  2 files changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 1209fea2566c..3199432f11f5 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
> >          }
> >      }
> >
> > +    register_virtual_region(region);
> > +
> >      return 0;
> >  }
> >
> 
> The region is registered in prepare_payload() but if e.g. the build id
> check in load_payload_data() fails, it won't unregister the region
> since the failure path calls free_payload_data(), not free_payload().
> Perhaps the call to register_virtual_region() could be done as the last
> thing in load_payload_data()?

I've moved it to livepatch_upload(), as I think it's more natural to
be done together with the addition to the list of livepatches.  Thanks
for spotting it, I guess I got confused between free_payload_data()
free_payload().

Roger.

Re: [PATCH 3/5] xen/livepatch: fix norevert test attempt to open-code revert

[Roger Pau Monné]

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.

Re: [PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 11:48:10AM +0100, Jan Beulich wrote:
> On 30.11.2023 15:29, Roger Pau Monne wrote:
> > Hello,
> > 
> > The follow series contain a misc of fixes mostly related to the usage of
> > the pre-apply / post-revert hooks.  The norevert test is also fixed to
> > work as I think was expected.  Finally both the no{apply,revert}
> > tests are fixed to build properly, as the files where previously
> > unhooked from the build system completely.
> > 
> > I'm unsure how useful the apply and revert hooks really are, as without
> > calling the internal apply/revert functions the state of the payload
> > structure is quite likely inconsistent with the code expectations.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (5):
> >   xen/livepatch: register livepatch regions when loaded
> >   xen/livepatch: search for symbols in all loaded payloads
> >   xen/livepatch: fix norevert test attempt to open-code revert
> >   xen/livepatch: fix norevert test hook setup typo
> >   xen/livepatch: properly build the noapply and norevert tests
> 
> With the R-b-s that have arrived, could you clarify in how far these
> can be committed out of order?

All can be applied out of order, except for the last one, that's
"xen/livepatch: properly build the noapply and norevert tests" as
applying that will enable the tests in osstest, and those will fail
without all the previous adjustments.

I was about to send v2 with the requested changes, but will wait for
you to commit 2/5 and 4/5.

Thanks, Roger.

Re: [PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[Jan Beulich]

On 26.02.2024 09:40, Roger Pau Monné wrote:
> On Fri, Feb 23, 2024 at 11:48:10AM +0100, Jan Beulich wrote:
>> On 30.11.2023 15:29, Roger Pau Monne wrote:
>>> Hello,
>>>
>>> The follow series contain a misc of fixes mostly related to the usage of
>>> the pre-apply / post-revert hooks.  The norevert test is also fixed to
>>> work as I think was expected.  Finally both the no{apply,revert}
>>> tests are fixed to build properly, as the files where previously
>>> unhooked from the build system completely.
>>>
>>> I'm unsure how useful the apply and revert hooks really are, as without
>>> calling the internal apply/revert functions the state of the payload
>>> structure is quite likely inconsistent with the code expectations.
>>>
>>> Thanks, Roger.
>>>
>>> Roger Pau Monne (5):
>>>   xen/livepatch: register livepatch regions when loaded
>>>   xen/livepatch: search for symbols in all loaded payloads
>>>   xen/livepatch: fix norevert test attempt to open-code revert
>>>   xen/livepatch: fix norevert test hook setup typo
>>>   xen/livepatch: properly build the noapply and norevert tests
>>
>> With the R-b-s that have arrived, could you clarify in how far these
>> can be committed out of order?
> 
> All can be applied out of order, except for the last one, that's
> "xen/livepatch: properly build the noapply and norevert tests" as
> applying that will enable the tests in osstest, and those will fail
> without all the previous adjustments.
> 
> I was about to send v2 with the requested changes, but will wait for
> you to commit 2/5 and 4/5.

Except that patch 2 had 3 failing hunks, and not just fuzz in any of them.
Which was too much for me to consider fixing up while committing.

Jan