Re: [PATCH v3 02/10] liveupdate: Synchronize lazy initialization of FLB private state

[Pasha Tatashin]

On Tue, Mar 31, 2026 at 3:22 PM Pratyush Yadav <pratyush@xxxxxxxxxx> wrote:
>
> On Tue, Mar 31 2026, Pasha Tatashin wrote:
>
> > On Tue, Mar 31, 2026 at 6:38 AM Pratyush Yadav <pratyush@xxxxxxxxxx> wrote:
> >>
> >> On Fri, Mar 27 2026, Pasha Tatashin wrote:
> >>
> >> > The luo_flb_get_private() function, which is responsible for lazily
> >> > initializing the private state of FLB objects, can be called
> >> > concurrently from multiple threads. This creates a data
> >> > race on the 'initialized' flag and can lead to multiple executions of
> >> > mutex_init() and INIT_LIST_HEAD() on the same memory.
> >> >
> >> > Introduce a static spinlock (luo_flb_init_lock) local to the function
> >> > to synchronize the initialization path. Use smp_load_acquire() and
> >> > smp_store_release() for memory ordering between the fast path and the
> >> > slow path.
> >> >
> >> > Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> >>
> >> Reviewed-by: Pratyush Yadav <pratyush@xxxxxxxxxx>
> >
> > Thank you.
> >
> >>
> >> But... wouldn't it be a whole lot simpler if we introduce a
> >> DEFINE_LUO_FLB() and get rid of luo_flb_get_private() entirely:
> >
> > We are planning some updates to this code in the near future:
> >
> > 1. David Matlack will send a patch in the next version of VFIO LUO
> > preservation series to add liveupdate_flb_put_incoming() so that
> > caller can protect from race with release of the last FD release that
> > will also release FLB.
> > This change will increment FLB 'count' not only when FH uses it, but
> > also when the object is being accessed.
> >
> > 2. After I plan to convert 'count' to use kref, and also decouble init
> > and liveupdate_flb_get_incoming(), this will allow to access FLB
> > object with interrupts disabled, as requested by Sami for the IOMMU
> > work. I think, that while working on this 2nd change, we can also do
> > some clean-ups if necessary.
>
> Okay, makes sense. I really think this initialization of private state
> on first use is ugly and it should be gotten rid of. So please, whenever
> you plan to do this refactor, include something like the static
> initialization I mentioned.

I am not sure if it is going to be possible without giving an access
to "private fields" to clients that declare FLBs, those sould really
be accessed internally by FLB itself. If that is doable than sure,
otherwise I prefer to keeping the logical separate that is enforced by
"__private"

Pasha

>
> >
> > Pasha
> >
> >>
> >> #define DEFINE_LUO_FLB(_name, _ops, _compatible)                        \
> >>         struct liveupdate_flb _name = {                                 \
> >>                 .ops = _ops,                                            \
> >>                 .compatible = _compatible,                              \
> >>                 .private = {                                            \
> >>                         .list = LIST_HEAD_INIT(_name.private.list),     \
> >>                         .list = LIST_HEAD_INIT(_name.private.list),     \
> >>                         / ...
> >>                 },                                                      \
> >>         }
> >>
> >> I can't get sparse to work so not sure if I need some special syntax to
> >> initialize stuff in .private, but I reckon we can get something working.
> >>
>
> --
> Regards,
> Pratyush Yadav