Re: [PATCH V3 6/6] null_blk: mark racy configfs attribute accesses with READ_ONCE/WRITE_ONCE

[Zizhi Wo]

在 2026/7/9 1:49, Nilay Shroff 写道:
> On 7/8/26 1:09 PM, Zizhi Wo wrote:
> > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/ 
> > main.c
> > index 9e0002e4aeec..fd3c993a67b2 100644
> > --- a/drivers/block/null_blk/main.c
> > +++ b/drivers/block/null_blk/main.c
> > @@ -346,7 +346,7 @@ static ssize_t                                \
> >   nullb_device_##NAME##_show(struct config_item *item, char *page)    \
> >   {                                    \
> >       return nullb_device_##TYPE##_attr_show(                \
> > -                to_nullb_device(item)->NAME, page);    \
> > +            READ_ONCE(to_nullb_device(item)->NAME), page);    \
> >   }                                    \
> >   static ssize_t                                \
> >   nullb_device_##NAME##_store(struct config_item *item, const char 
> > *page,    \
> > @@ -366,7 +366,7 @@ nullb_device_##NAME##_store(struct config_item 
> > *item, const char *page,    \
> >       else if (test_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags))     \
> >           ret = -EBUSY;                        \
> >       if (!ret)                            \
> > -        dev->NAME = new_value;                    \
> > +        WRITE_ONCE(dev->NAME, new_value);            \
> >       mutex_unlock(&lock);                        \
> >       if (ret < 0)                            \
> >           return ret;                        \
>
> So in this series I see that all attribute _store() methods are now 
> protected
> by the file-scoped mutex, which wasn't the case before. If this is 
> intentional,
> have you considered protecting _show() with the same mutex as well? If 
> yes, then
> that would avoid the lockless read/write races without requiring the 
> additional
> READ_ONCE()/WRITE_ONCE() annotations throughout the code.
>
> Since these configfs attribute accesses are not on the I/O hot path, 
> taking the
> mutex in _show() also seems acceptable from a performance perspective. 
> Earlier
> in the previous series, I suggested using READ_ONCE/WRITE_ONCE because only
> _store() methods using apply_fn attributes were using the lock but that's
> not the case now.
>
> Thanks,
> --Nilay

Thanks for the suggestion.

Indeed, _store() now takes the lock unconditionally. Otherwise the
else-branch of _store() could run concurrently with
nullb_device_power_store() -> null_add_dev() during power-on and cause
other problems. So taking the lock in _show() directly avoids the races,
without scattering ~a dozen WRITE_ONCE() across two files.

Since this is not a hot path, and I/O hot paths such as
null_complete_cmd() are rejected from writing during power-on anyway,
this is fine.

I'll drop this patch and instead take "lock" in _show() (and in
power_show()) in the next revision.

Thanks,
Zizhi Wo