Currently the livepatch logic inside of Xen will use fields of struct
livepatch_func in order to cache internal state of patched functions.  Note
this is a field that is part of the payload, and is loaded as an ELF section
(.livepatch.funcs), taking into account the SHF_* flags in the section
header.

The flags for the .livepatch.funcs section, as set by livepatch-build-tools,
are SHF_ALLOC, which leads to its contents (the array of livepatch_func
structures) being placed in read-only memory:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
[...]
  [ 4] .livepatch.funcs  PROGBITS         0000000000000000  00000080
       0000000000000068  0000000000000000   A       0     0     8

This previously went unnoticed, as all writes to the fields of livepatch_func
happen in the critical region that had WP disabled in CR0.  After 8676092a0f16
however WP is no longer toggled in CR0 for patch application, and only the
hypervisor .text mappings are made write-accessible.  That leads to the
following page fault when attempting to apply a livepatch:

----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
CPU:    4
RIP:    e008:[<ffff82d040221e81>] common/livepatch.c#apply_payload+0x45/0x1e1
[...]
Xen call trace:
   [<ffff82d040221e81>] R common/livepatch.c#apply_payload+0x45/0x1e1
   [<ffff82d0402235b2>] F check_for_livepatch_work+0x385/0xaa5
   [<ffff82d04032508f>] F arch/x86/domain.c#idle_loop+0x92/0xee

Pagetable walk from ffff82d040625079:
 L4[0x105] = 000000008c6c9063 ffffffffffffffff
 L3[0x141] = 000000008c6c6063 ffffffffffffffff
 L2[0x003] = 000000086a1e7063 ffffffffffffffff
 L1[0x025] = 800000086ca5d121 ffffffffffffffff

****************************************
Panic on CPU 4:
FATAL PAGE FAULT
[error_code=0003]
Faulting linear address: ffff82d040625079
****************************************

Fix this by moving the internal Xen function patching state out of
livepatch_func into an area not allocated as part of the ELF payload.  While
there also constify the array of livepatch_func structures in order to prevent
further surprises.

Note there's still one field (old_addr) that gets set during livepatch load.  I
consider this fine since the field is read-only after load, and at the point
the field gets set the underlying mapping hasn't been made read-only yet.

Fixes: 8676092a0f16 ('x86/livepatch: Fix livepatch application when CET is active')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The selection of the Fixes tag is kind of wonky.  While it's true that
8676092a0f16 broke livepatching, that's only due to the assumptions made by
livepatch about read-only areas becoming read-write during the patching
window.

Another way to fix it would be to add the SHF_WRITE flag to .livepatch.funcs
section flags in livepatch-build-tools, but I find such approach fragile.
---
 xen/arch/arm/arm32/livepatch.c      |  9 ++++---
 xen/arch/arm/arm64/livepatch.c      |  9 ++++---
 xen/arch/arm/livepatch.c            |  9 ++++---
 xen/arch/x86/livepatch.c            | 26 +++++++++++---------
 xen/common/livepatch.c              | 25 +++++++++++++------
 xen/include/public/sysctl.h         |  5 +---
 xen/include/xen/livepatch.h         | 38 ++++++++++++++++++++---------
 xen/include/xen/livepatch_payload.h |  3 ++-
 8 files changed, 76 insertions(+), 48 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 3c50283b2ab7..5dd877a5c625 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -11,23 +11,24 @@
 #include <asm/page.h>
 #include <asm/livepatch.h>
 
-void arch_livepatch_apply(struct livepatch_func *func)
+void arch_livepatch_apply(const struct livepatch_func *func,
+                          struct livepatch_fstate *state)
 {
     uint32_t insn;
     uint32_t *new_ptr;
     unsigned int i, len;
 
-    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
+    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(state->opaque));
     BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
 
     ASSERT(vmap_of_xen_text);
 
-    len = livepatch_insn_len(func);
+    len = livepatch_insn_len(func, state);
     if ( !len )
         return;
 
     /* Save old ones. */
-    memcpy(func->opaque, func->old_addr, len);
+    memcpy(state->opaque, func->old_addr, len);
 
     if ( func->new_addr )
     {
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 62d2ef373a0e..9017e6b0c2bc 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -15,23 +15,24 @@
 #include <asm/insn.h>
 #include <asm/livepatch.h>
 
-void arch_livepatch_apply(struct livepatch_func *func)
+void arch_livepatch_apply(const struct livepatch_func *func,
+                          struct livepatch_fstate *state)
 {
     uint32_t insn;
     uint32_t *new_ptr;
     unsigned int i, len;
 
-    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
+    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(state->opaque));
     BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
 
     ASSERT(vmap_of_xen_text);
 
-    len = livepatch_insn_len(func);
+    len = livepatch_insn_len(func, state);
     if ( !len )
         return;
 
     /* Save old ones. */
-    memcpy(func->opaque, func->old_addr, len);
+    memcpy(state->opaque, func->old_addr, len);
 
     if ( func->new_addr )
         insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index d646379c8c5e..38f6b335a50e 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -69,7 +69,7 @@ void arch_livepatch_revive(void)
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
     /* If NOPing only do up to maximum amount we can put in the ->opaque. */
-    if ( !func->new_addr && (func->new_size > sizeof(func->opaque) ||
+    if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE ||
          func->new_size % ARCH_PATCH_INSN_SIZE) )
         return -EOPNOTSUPP;
 
@@ -79,15 +79,16 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     return 0;
 }
 
-void arch_livepatch_revert(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func,
+                           struct livepatch_fstate *state)
 {
     uint32_t *new_ptr;
     unsigned int len;
 
     new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
 
-    len = livepatch_insn_len(func);
-    memcpy(new_ptr, func->opaque, len);
+    len = livepatch_insn_len(func, state);
+    memcpy(new_ptr, state->opaque, len);
 
     clean_and_invalidate_dcache_va_range(new_ptr, len);
 }
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index a54d991c5f0f..97e54b18a912 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -95,7 +95,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     if ( !func->new_addr )
     {
         /* Only do up to maximum amount we can put in the ->opaque. */
-        if ( func->new_size > sizeof(func->opaque) )
+        if ( func->new_size > LIVEPATCH_OPAQUE_SIZE )
             return -EOPNOTSUPP;
 
         if ( func->old_size < func->new_size )
@@ -123,13 +123,14 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
  * "noinline" to cause control flow change and thus invalidate I$ and
  * cause refetch after modification.
  */
-void noinline arch_livepatch_apply(struct livepatch_func *func)
+void noinline arch_livepatch_apply(const struct livepatch_func *func,
+                                   struct livepatch_fstate *state)
 {
     uint8_t *old_ptr;
-    uint8_t insn[sizeof(func->opaque)];
+    uint8_t insn[sizeof(state->opaque)];
     unsigned int len;
 
-    func->patch_offset = 0;
+    state->patch_offset = 0;
     old_ptr = func->old_addr;
 
     /*
@@ -141,14 +142,14 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
      * ENDBR64 or similar instructions).
      */
     if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
-        func->patch_offset += ENDBR64_LEN;
+        state->patch_offset += ENDBR64_LEN;
 
     /* This call must be done with ->patch_offset already set. */
-    len = livepatch_insn_len(func);
+    len = livepatch_insn_len(func, state);
     if ( !len )
         return;
 
-    memcpy(func->opaque, old_ptr + func->patch_offset, len);
+    memcpy(state->opaque, old_ptr + state->patch_offset, len);
     if ( func->new_addr )
     {
         int32_t val;
@@ -156,7 +157,7 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
         BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
 
         insn[0] = 0xe9; /* Relative jump. */
-        val = func->new_addr - (func->old_addr + func->patch_offset +
+        val = func->new_addr - (func->old_addr + state->patch_offset +
                                 ARCH_PATCH_INSN_SIZE);
 
         memcpy(&insn[1], &val, sizeof(val));
@@ -164,17 +165,18 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
     else
         add_nops(insn, len);
 
-    memcpy(old_ptr + func->patch_offset, insn, len);
+    memcpy(old_ptr + state->patch_offset, insn, len);
 }
 
 /*
  * "noinline" to cause control flow change and thus invalidate I$ and
  * cause refetch after modification.
  */
-void noinline arch_livepatch_revert(const struct livepatch_func *func)
+void noinline arch_livepatch_revert(const struct livepatch_func *func,
+                                    struct livepatch_fstate *state)
 {
-    memcpy(func->old_addr + func->patch_offset, func->opaque,
-           livepatch_insn_len(func));
+    memcpy(func->old_addr + state->patch_offset, state->opaque,
+           livepatch_insn_len(func, state));
 }
 
 /*
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d89a904bd4e1..e635606c10fb 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -260,6 +260,9 @@ static void free_payload_data(struct payload *payload)
     vfree((void *)payload->text_addr);
 
     payload->pages = 0;
+
+    /* fstate gets allocated strictly after move_payload. */
+    XFREE(payload->fstate);
 }
 
 /*
@@ -656,6 +659,7 @@ static int prepare_payload(struct payload *payload,
 {
     const struct livepatch_elf_sec *sec;
     unsigned int i;
+    struct livepatch_func *funcs;
     struct livepatch_func *f;
     struct virtual_region *region;
     const Elf_Note *n;
@@ -666,14 +670,19 @@ static int prepare_payload(struct payload *payload,
         if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
             return -EINVAL;
 
-        payload->funcs = sec->load_addr;
+        payload->funcs = funcs = sec->load_addr;
         payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
 
+        payload->fstate = xzalloc_array(typeof(*payload->fstate),
+                                        payload->nfuncs);
+        if ( !payload->fstate )
+            return -ENOMEM;
+
         for ( i = 0; i < payload->nfuncs; i++ )
         {
             int rc;
 
-            f = &(payload->funcs[i]);
+            f = &(funcs[i]);
 
             if ( f->version != LIVEPATCH_PAYLOAD_VERSION )
             {
@@ -1361,7 +1370,7 @@ 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]);
+        common_livepatch_apply(&data->funcs[i], &data->fstate[i]);
 
     arch_livepatch_revive();
 
@@ -1397,7 +1406,7 @@ static int revert_payload(struct payload *data)
     }
 
     for ( i = 0; i < data->nfuncs; i++ )
-        common_livepatch_revert(&data->funcs[i]);
+        common_livepatch_revert(&data->funcs[i], &data->fstate[i]);
 
     /*
      * Since we are running with IRQs disabled and the hooks may call common
@@ -1438,9 +1447,10 @@ static inline bool was_action_consistent(const struct payload *data, livepatch_f
 
     for ( i = 0; i < data->nfuncs; i++ )
     {
-        struct livepatch_func *f = &(data->funcs[i]);
+        const struct livepatch_func *f = &(data->funcs[i]);
+        const struct livepatch_fstate *s = &(data->fstate[i]);
 
-        if ( f->applied != expected_state )
+        if ( s->applied != expected_state )
         {
             printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n",
                    data->name, f->name ?: "noname");
@@ -2157,7 +2167,8 @@ static void cf_check livepatch_printall(unsigned char key)
 
         for ( i = 0; i < data->nfuncs; i++ )
         {
-            struct livepatch_func *f = &(data->funcs[i]);
+            const struct livepatch_func *f = &(data->funcs[i]);
+
             printk("    %s patch %p(%u) with %p (%u)\n",
                    f->name, f->old_addr, f->old_size, f->new_addr, f->new_size);
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index f1eba784059a..9b19679caeb1 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -991,10 +991,7 @@ struct livepatch_func {
     uint32_t new_size;
     uint32_t old_size;
     uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
-    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
-    uint8_t applied;
-    uint8_t patch_offset;
-    uint8_t _pad[6];
+    uint8_t _pad[39];
     livepatch_expectation_t expect;
 };
 typedef struct livepatch_func livepatch_func_t;
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 9fdb29c382b6..a44141a82ef2 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -13,6 +13,9 @@ struct xen_sysctl_livepatch_op;
 
 #include <xen/elfstructs.h>
 #include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
+
+#include <public/sysctl.h> /* For LIVEPATCH_OPAQUE_SIZE */
+
 #ifdef CONFIG_LIVEPATCH
 
 /*
@@ -51,6 +54,12 @@ struct livepatch_symbol {
     bool_t new_symbol;
 };
 
+struct livepatch_fstate {
+    unsigned int patch_offset;
+    enum livepatch_func_state applied;
+    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
+};
+
 int livepatch_op(struct xen_sysctl_livepatch_op *);
 void check_for_livepatch_work(void);
 unsigned long livepatch_symbols_lookup_by_name(const char *symname);
@@ -87,10 +96,11 @@ void arch_livepatch_init(void);
 int arch_livepatch_verify_func(const struct livepatch_func *func);
 
 static inline
-unsigned int livepatch_insn_len(const struct livepatch_func *func)
+unsigned int livepatch_insn_len(const struct livepatch_func *func,
+                                const struct livepatch_fstate *state)
 {
     if ( !func->new_addr )
-        return func->new_size - func->patch_offset;
+        return func->new_size - state->patch_offset;
 
     return ARCH_PATCH_INSN_SIZE;
 }
@@ -117,39 +127,43 @@ int arch_livepatch_safety_check(void);
 int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
 
-void arch_livepatch_apply(struct livepatch_func *func);
-void arch_livepatch_revert(const struct livepatch_func *func);
+void arch_livepatch_apply(const struct livepatch_func *func,
+                          struct livepatch_fstate *state);
+void arch_livepatch_revert(const struct livepatch_func *func,
+                           struct livepatch_fstate *state);
 void arch_livepatch_post_action(void);
 
 void arch_livepatch_mask(void);
 void arch_livepatch_unmask(void);
 
-static inline void common_livepatch_apply(struct livepatch_func *func)
+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 ( func->applied == LIVEPATCH_FUNC_APPLIED )
+    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);
-    func->applied = LIVEPATCH_FUNC_APPLIED;
+    arch_livepatch_apply(func, state);
+    state->applied = LIVEPATCH_FUNC_APPLIED;
 }
 
-static inline void common_livepatch_revert(struct livepatch_func *func)
+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 || func->applied == LIVEPATCH_FUNC_NOT_APPLIED )
+    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);
-    func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
+    arch_livepatch_revert(func, state);
+    state->applied = LIVEPATCH_FUNC_NOT_APPLIED;
 }
 #else
 
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 9f5f0642057c..b9cd4f209670 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -52,7 +52,8 @@ struct payload {
     size_t ro_size;                      /* .. and its size (if any). */
     unsigned int pages;                  /* Total pages for [text,rw,ro]_addr */
     struct list_head applied_list;       /* Linked to 'applied_list'. */
-    struct livepatch_func *funcs;        /* The array of functions to patch. */
+    const struct livepatch_func *funcs;  /* The array of functions to patch. */
+    struct livepatch_fstate *fstate;     /* State of patched functions. */
     unsigned int nfuncs;                 /* Nr of functions to patch. */
     const struct livepatch_symbol *symtab; /* All symbols. */
     const char *strtab;                  /* Pointer to .strtab. */
-- 
2.42.0

On 16.11.2023 12:58, Roger Pau Monne wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -991,10 +991,7 @@ struct livepatch_func {
>      uint32_t new_size;
>      uint32_t old_size;
>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> -    uint8_t applied;
> -    uint8_t patch_offset;
> -    uint8_t _pad[6];
> +    uint8_t _pad[39];

Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...

> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -13,6 +13,9 @@ struct xen_sysctl_livepatch_op;
>  
>  #include <xen/elfstructs.h>
>  #include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
> +
> +#include <public/sysctl.h> /* For LIVEPATCH_OPAQUE_SIZE */
> +
>  #ifdef CONFIG_LIVEPATCH
>  
>  /*
> @@ -51,6 +54,12 @@ struct livepatch_symbol {
>      bool_t new_symbol;
>  };
>  
> +struct livepatch_fstate {
> +    unsigned int patch_offset;
> +    enum livepatch_func_state applied;
> +    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];

... this use a separate, new (and internal only) constant? Thus also
elimiating the need to include public/sysctl.h above?

Jan

On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> On 16.11.2023 12:58, Roger Pau Monne wrote:
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -991,10 +991,7 @@ struct livepatch_func {
> >      uint32_t new_size;
> >      uint32_t old_size;
> >      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> > -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> > -    uint8_t applied;
> > -    uint8_t patch_offset;
> > -    uint8_t _pad[6];
> > +    uint8_t _pad[39];
> 
> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...

Hm, I'm not sure that's any clearer.  In fact I think
LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
place.

If we later want to add more fields and bump the version, isn't it
easier to have the padding size clearly specified as a number?

> 
> > --- a/xen/include/xen/livepatch.h
> > +++ b/xen/include/xen/livepatch.h
> > @@ -13,6 +13,9 @@ struct xen_sysctl_livepatch_op;
> >  
> >  #include <xen/elfstructs.h>
> >  #include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
> > +
> > +#include <public/sysctl.h> /* For LIVEPATCH_OPAQUE_SIZE */
> > +
> >  #ifdef CONFIG_LIVEPATCH
> >  
> >  /*
> > @@ -51,6 +54,12 @@ struct livepatch_symbol {
> >      bool_t new_symbol;
> >  };
> >  
> > +struct livepatch_fstate {
> > +    unsigned int patch_offset;
> > +    enum livepatch_func_state applied;
> > +    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> 
> ... this use a separate, new (and internal only) constant? Thus also
> elimiating the need to include public/sysctl.h above?

The size of the buffer here is tied to the buffer size in
livepatch_expectation data field, so if using a different size
internally we would still need to ensure that the internal size is >=
LIVEPATCH_OPAQUE_SIZE, not sure there's much benefit in it.

In any case, I would suggest to do this in a followup patch, the
content in this patch is IMO enough.

Thanks, Roger.

On Thu, Nov 16, 2023 at 1:54 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> > On 16.11.2023 12:58, Roger Pau Monne wrote:
> > > --- a/xen/include/public/sysctl.h
> > > +++ b/xen/include/public/sysctl.h
> > > @@ -991,10 +991,7 @@ struct livepatch_func {
> > >      uint32_t new_size;
> > >      uint32_t old_size;
> > >      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> > > -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> > > -    uint8_t applied;
> > > -    uint8_t patch_offset;
> > > -    uint8_t _pad[6];
> > > +    uint8_t _pad[39];
> >
> > Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
>
> Hm, I'm not sure that's any clearer.  In fact I think
> LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> place.
>
> If we later want to add more fields and bump the version, isn't it
> easier to have the padding size clearly specified as a number?

I prefer using the number as it makes it clear that this padding is not
(anymore) related to the size of the instruction buffer in livepatch_fstate.

Do you think it would be better to call livepatch_fstate.opaque
something like livepatch_fstate.insn_buffer instead (and rename
the constant accordingly) since it is internal to Xen and is not
hiding something from tools building live patches?

(Otherwise this patch looks generally fine to me.)

Ross

On Wed, Nov 22, 2023 at 04:31:07PM +0000, Ross Lagerwall wrote:
> On Thu, Nov 16, 2023 at 1:54 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> > > On 16.11.2023 12:58, Roger Pau Monne wrote:
> > > > --- a/xen/include/public/sysctl.h
> > > > +++ b/xen/include/public/sysctl.h
> > > > @@ -991,10 +991,7 @@ struct livepatch_func {
> > > >      uint32_t new_size;
> > > >      uint32_t old_size;
> > > >      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> > > > -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> > > > -    uint8_t applied;
> > > > -    uint8_t patch_offset;
> > > > -    uint8_t _pad[6];
> > > > +    uint8_t _pad[39];
> > >
> > > Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
> >
> > Hm, I'm not sure that's any clearer.  In fact I think
> > LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> > place.
> >
> > If we later want to add more fields and bump the version, isn't it
> > easier to have the padding size clearly specified as a number?
> 
> I prefer using the number as it makes it clear that this padding is not
> (anymore) related to the size of the instruction buffer in livepatch_fstate.
> 
> Do you think it would be better to call livepatch_fstate.opaque
> something like livepatch_fstate.insn_buffer instead (and rename

That would be fine.

> the constant accordingly) since it is internal to Xen and is not
> hiding something from tools building live patches?

The constant would be needed anyway due to the usage in the livepatch
tests, see the layout of livepatch_expectation_t which is public and
part of livepatch_func.

I don't mind changing fstate->opaque to insn_buffer, but I would avoid
touching anything in livepatch_expectation_t as part of this patch.

Thanks, Roger.

On 16.11.2023 14:54, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
>> On 16.11.2023 12:58, Roger Pau Monne wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -991,10 +991,7 @@ struct livepatch_func {
>>>      uint32_t new_size;
>>>      uint32_t old_size;
>>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
>>> -    uint8_t applied;
>>> -    uint8_t patch_offset;
>>> -    uint8_t _pad[6];
>>> +    uint8_t _pad[39];
>>
>> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
> 
> Hm, I'm not sure that's any clearer.  In fact I think
> LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> place.
> 
> If we later want to add more fields and bump the version, isn't it
> easier to have the padding size clearly specified as a number?

If new fields (beyond the present padding size) would need adding,
that would constitute an incompatible change anyway. Until then imo
it would be clearer to keep the reference to the original constant.
But thinking about it again, the difference is perhaps indeed only
marginal. Anyway, I'll leave this to the livepatch maintainers.

One further related remark though: Now that you pointed me at the
other use of the constant in the public header, don't you want to
update the comment there, for it to not become stale (in referring
to struct livepatch_func)?

Jan

On Thu, Nov 16, 2023 at 03:05:20PM +0100, Jan Beulich wrote:
> On 16.11.2023 14:54, Roger Pau Monné wrote:
> > On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> >> On 16.11.2023 12:58, Roger Pau Monne wrote:
> >>> --- a/xen/include/public/sysctl.h
> >>> +++ b/xen/include/public/sysctl.h
> >>> @@ -991,10 +991,7 @@ struct livepatch_func {
> >>>      uint32_t new_size;
> >>>      uint32_t old_size;
> >>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> >>> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> >>> -    uint8_t applied;
> >>> -    uint8_t patch_offset;
> >>> -    uint8_t _pad[6];
> >>> +    uint8_t _pad[39];
> >>
> >> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
> > 
> > Hm, I'm not sure that's any clearer.  In fact I think
> > LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> > place.
> > 
> > If we later want to add more fields and bump the version, isn't it
> > easier to have the padding size clearly specified as a number?
> 
> If new fields (beyond the present padding size) would need adding,
> that would constitute an incompatible change anyway.

Not if we bump the version field I think?

As the version is a strict match, bumping the version allows for a
completely new layout to be implemented past the 'version' field.

> Until then imo
> it would be clearer to keep the reference to the original constant.
> But thinking about it again, the difference is perhaps indeed only
> marginal. Anyway, I'll leave this to the livepatch maintainers.
> 
> One further related remark though: Now that you pointed me at the
> other use of the constant in the public header, don't you want to
> update the comment there, for it to not become stale (in referring
> to struct livepatch_func)?

Hm, yes, indeed.  I will wait for further comments before sending just
that comment fix.  I would add:

uint8_t data[LIVEPATCH_OPAQUE_SIZE]; /* Max number of bytes to be patched */

Thanks, Roger.

On 16.11.2023 15:48, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 03:05:20PM +0100, Jan Beulich wrote:
>> On 16.11.2023 14:54, Roger Pau Monné wrote:
>>> On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
>>>> On 16.11.2023 12:58, Roger Pau Monne wrote:
>>>>> --- a/xen/include/public/sysctl.h
>>>>> +++ b/xen/include/public/sysctl.h
>>>>> @@ -991,10 +991,7 @@ struct livepatch_func {
>>>>>      uint32_t new_size;
>>>>>      uint32_t old_size;
>>>>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>>>> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
>>>>> -    uint8_t applied;
>>>>> -    uint8_t patch_offset;
>>>>> -    uint8_t _pad[6];
>>>>> +    uint8_t _pad[39];
>>>>
>>>> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
>>>
>>> Hm, I'm not sure that's any clearer.  In fact I think
>>> LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
>>> place.
>>>
>>> If we later want to add more fields and bump the version, isn't it
>>> easier to have the padding size clearly specified as a number?
>>
>> If new fields (beyond the present padding size) would need adding,
>> that would constitute an incompatible change anyway.
> 
> Not if we bump the version field I think?
> 
> As the version is a strict match, bumping the version allows for a
> completely new layout to be implemented past the 'version' field.

Of course.

Jan