Re: [RFC PATCH v2 3/4] liveupdate: Replace compatibility string with a 2-bit pair feature header

[Pasha Tatashin]

On Wed, Oct 21, 2026 at 11:53 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Wed, Oct 21, 2026 at 8:32 PM Pasha Tatashin
> <pasha.tatashin@xxxxxxxxxx> wrote:
> >
> > On Wed, Oct 21, 2026 at 10:57 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > >
> > > On 2026-10-21 09:26 PM, Pasha Tatashin wrote:
> > > > On Wed, Oct 21, 2026 at 7:28 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > > > >
> > > > > On 2026-10-21 02:50 PM, Pasha Tatashin wrote:
> > > > > > On Wed, Oct 21, 2026 at 2:47 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Wed, Oct 21, 2026 at 11:40 AM <tarunsahu@xxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> writes:
> > > > > > > >
> > > > > > > > > On Wed, Oct 21, 2026 at 11:25 AM Tarun Sahu <tarunsahu@xxxxxxxxxx> wrote:
> > > > > > > > >>
> > > > > > > > >> LUO and its file handlers validate handed-over state with monolithic
> > > > > > > > >> compatibility strings ("luo-v5", "memfd-v2"). Any change to a serialized
> > > > > > > > >> structure, even an optional additive one, must bump the string, which
> > > > > > > > >> breaks live update between adjacent kernels. Replace the string in
> > > > > > > > >> struct luo_ser with a feature header, so that the target kernel can
> > > > > > > > >> accept or reject the source kernel state per feature.
> > > > > > > > >>
> > > > > > > > >> struct luo_feature_hdr is a fixed 128-byte header: the serialized
> > > > > > > > >
> > > > > > > > > This patch is all wrong; 128 bytes is far too large. For every _ser
> > > > > > > > > struct, we only need 8 bytes instead.
> > > > > > > >
> > > > > > > > NOO, It is not. It is intended to avoid complex mess with unnecessary
> > > > > > > > over-engineering.
> > > > > >
> > > > > > 128-byte per serialized struct is not starter.
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > The first 4 bytes are the size, and the next 4 bytes are flags (i.e.,
> > > > > > > > > 30 feature flags + 2 reserved bits for flags extension). The last two
> > > > > > > > > bits of the flags indicate whether an extended flags field is present.
> > > > > > > > > Each extended flags field is 8 bytes, and its last two bits indicate
> > > > > > > > > that yet another extended flags field follows.
> > > > > > > >
> > > > > > > > If we will have dynamic extension of flags, Think about this.
> > > > > > >
> > > > > > > To support dynamic extensions in a backward compatible way you want
> > > > > > > something like this:
> > > > > > >
> > > > > > > struct luo_feature_hdr {
> > > > > > >    size
> > > > > > >    flags
> > > > > >
> > > > > > __u32 size
> > > > > > __u32 flags
> > > > > >
> > > > > > >    extensions  // offset of first luo_feature_extensions (or 0)
> > > > > >
> > > > > > __u64 extensions[0]  --> but the size should be specificed in
> > > > > > declaration macro that I showed in the next patch.
> > > > >
> > > > > I think you may have misunderstood what I meant. In my proposal,
> > > > > extensions is an offset to the first extension, not an array of
> > > > > extensions that immediately follow the header.
> > > > >
> > > > > If the extensions immediately follow the header, then the number of
> > > > > extensions is baked into the struct layout. Every field after them sits
> > > > > at an offset that depends on how many extensions there are. If we ever
> > > > > need one more extension, all the fields shift and old and new kernels
> > > > > can no longer read each other's struct. That is the problem Tarun
> > > >
> > > > We can structure it cleanly by nesting the payload: something like
> > > > struct { struct header {...}h; struct serialize_data { ... }p; };
> > > > That way, the header can expand dynamically at the front, while all
> > > > fields inside `serialize_data` stay at fixed offsets relative to the
> > > > start
> > > > of the payload.
> > > >
> > > > Since the header size is self-describing via continuation bits (e.g.,
> > > > the last two bits indicating whether another flag word follows), LUO
> > > > can
> > > > easily compute where the payload begins in the incoming blob. This
> > > > keeps the implementation simple, avoids linked lists of offsets.
> > >
> > > Oh interesting. I agree a dynamically sized header at the front can work
> > > and is simpler than a linked list.
> > >
> > > So the header and the payload need to be separate, and the payload is
> > > located with pointer arithmetic on the header. For example:
> > >
> > > struct luo_ser_hdr {
> > >     u32 size;    /* Size of the payload in bytes */
> > >     u32 flags;    /* Bits [31:30] indicate an extension word follows */
> > >     /* u64 extension words follow while the continuation bits are set */
> > > };
> > >
> > > /* Size of the header, including any extension words. */
> > > size_t luo_ser_hdr_size(const struct luo_ser_hdr *hdr);
> > >
> > > static inline void *luo_ser_payload(const struct luo_ser_hdr *hdr)
> > > {
> > >     return (void *)hdr + luo_ser_hdr_size(hdr);
> > > }
> > >
> > > On preserve:
> > >
> > >     hdr = kho_alloc_preserve(luo_ser_hdr_size_for(nr_ext) + sizeof(*ser));
> > >     luo_ser_hdr_init(hdr, sizeof(*ser), ...);
> > >     ser = luo_ser_payload(hdr);
> > >     ...
> > >     args->serialized_data = virt_to_phys(hdr);
> > >
> > > On retrieve:
> > >
> > >     hdr = phys_to_virt(args->serialized_data);
> > >     err = luo_ser_hdr_validate(hdr, ...);
> > >     if (err)
> > >         return err;
> > >
> > >     ser = luo_ser_payload(hdr);
> > >
> > > What would size mean in luo_ser_hdr? The size of the payload or the size
> > > of the combined luo_ser_hdr + payload. luo_ser_hdr is self describing so
> > > maybe it's just the size of the payload?
> >
> > > What would size mean in luo_ser_hdr? The size of the payload or the size
> > > of the combined luo_ser_hdr + payload. luo_ser_hdr is self describing so
> > > maybe it's just the size of the payload?
> >
> > Either works, but having size cover the combined hdr + payload
> > provides a few extra guarantees:
> >
> > 1. A single static_assert validates the entire serialized struct at
> > compile time, including the header and the number of ext_flags
> > elements.
> > 2. Bounds checking while traversing the header continuation bits on
> > retrieve: we can sanity-check that luo_ser_hdr_size(hdr) <= hdr->size
> > so we never walk past the end of the serialized blob if there is
> > memory corruption.
> >
> > If we declare the payload struct separately and pass it into the
> > macro, handlers also get a clean type for luo_ser_payload(hdr):
> >
> > #define DECLARE_LUO_SER(name, expected_size, ext_flags_count, payload_type) \
> >     struct name {                                                           \
> >         __u32 size;                                                         \
> >         __u32 flags;                                                        \
> >         __u64 ext_flags[ext_flags_count];                                   \
> >         payload_type payload;                                               \
> >     };                                                                      \
> >     static_assert(sizeof(struct name) == (expected_size),                   \
> >                   "LUO struct " #name " size mismatch!")
> >
> > For example:
> >
> > struct luo_memfd_payload {
> >     __u64 pos;
> >     __u64 size;
> > };
> >
> > DECLARE_LUO_SER(luo_memfd_ser, 24, 0, struct luo_memfd_payload);
>
> This looks pretty reasonable but I would name the payload
> luo_memfd_ser. The combined struct that DECLARE_LUO_SER() creates
> should never be used directly since the offset of the payload in that
> struct is only valid in the current kernel. So I would give that
> struct a special name that makes it obvious you shouldn't be using it.

Lets drop the fourth macro argument and generate the combined struct name:

#define DECLARE_LUO_SER(name, expected_size, ext_flags_count)              \
    struct name##_hdr {                                                    \
        __u32 size;                                                        \
        __u32 flags;                                                       \
        __u64 ext_flags[ext_flags_count];                                  \
        struct name payload;                                               \
    };                                                                     \
    static_assert(sizeof(struct name##_hdr) == (expected_size),            \
                  "LUO struct " #name " size mismatch!")

For example:

struct luo_memfd_ser {
    __u64 pos;
    __u64 size;
};

DECLARE_LUO_SER_HDR(luo_memfd_ser, 24, 0);