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

[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