Re: [PATCH v7 21/22] liveupdate: luo_flb: Introduce File-Lifecycle-Bound global state

[Pasha Tatashin]

On Mon, Nov 24, 2025 at 6:45 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Sat, Nov 22, 2025 at 2:24 PM Pasha Tatashin
> <pasha.tatashin@xxxxxxxxxx> wrote:
>
> > +int liveupdate_flb_incoming_locked(struct liveupdate_flb *flb, void **objp);
> > +void liveupdate_flb_incoming_unlock(struct liveupdate_flb *flb, void *obj);
> > +int liveupdate_flb_outgoing_locked(struct liveupdate_flb *flb, void **objp);
> > +void liveupdate_flb_outgoing_unlock(struct liveupdate_flb *flb, void *obj);
>
> nit: "locked" should be "lock". "locked" is used for situations where
> the lock must already be held by the caller.

I am going to clean-up this API, and remove locked/unlocked; just
return the object directly.

>
> > @@ -633,6 +639,7 @@ static void luo_file_finish_one(struct luo_file_set *file_set,
> >         args.file = luo_file->file;
> >         args.serialized_data = luo_file->serialized_data;
> >         args.retrieved = luo_file->retrieved;
> > +       luo_flb_file_finish(luo_file->fh);
> >
> >         luo_file->fh->ops->finish(&args);
>
> I think luo_flb_file_finish() should be called after the file's
> finish() callback. Otherwise the FLB data will be cleaned just before
> the last file's finish() callback.
>
> i.e. The order should be
>
>   file1->finish()
>   file2->finish()
>   file3->finish() // last file
>   flb->finish()
>
> rather than
>
>   file1->finish()
>   file2->finish()
>   flb->finish()
>   file3->finish() // last file

Yes, I will make this change in the next version of FLB patch
(currently FLB has been dropped from LUO and will be sent separately
since there currently no in-kernel users beside the self-test)

>
> > +static void luo_flb_unlock(struct liveupdate_flb *flb, bool incoming,
> > +                          void *obj)
> > +{
> > +       struct luo_flb_private *private = luo_flb_get_private(flb);
> > +       struct luo_flb_private_state *state;
> > +
> > +       state = incoming ? &private->incoming : &private->outgoing;
> > +
> > +       lockdep_assert_held(&state->lock);
> > +       state->obj = obj;
>
> I tripped over this when developing the PCI FLB state. The following
> compiles fine and looks innocent enough:
>
>   liveupdate_flb_incoming_locked(&pci_liveupdate_flb, &ser);
>   ...
>   liveupdate_flb_incoming_unlock(&pci_liveupdate_flb, &ser);
>
> But this ends up corrupting state->obj.
>
> Do we have a use-case for replacing obj on unlock? If not I'd suggest
> dropping it.

I can remove internal obj updates.