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

[David Matlack]

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.