Re: [PATCH V2 5/6] null_blk: don't locklessly overwrite dev state after apply_fn

[Nilay Shroff]

On 7/7/26 8:25 AM, Zizhi Wo wrote:
> From: Zizhi Wo <wozizhi@xxxxxxxxxx>
>
> The NULLB_DEVICE_ATTR macro unconditionally writes dev->NAME = new_value
> after apply_fn() returns. For attributes with an apply_fn (submit_queues,
> poll_queues), apply_fn already sets dev->NAME under &nullb_list_lock.
>
> configfs serializes writes via a per-open-file mutex (buffer->mutex), so
> two threads writing to the same attribute through separate open file
> descriptions run the store callback concurrently. The macro's write is
> redundant and lockless, so a concurrent store's losing thread can overwrite
> the winner's value after apply_fn set it, making dev->submit_queues
> mismatch the hardware state. null_map_queues() then hits a WARN_ON_ONCE and
> falls back to a single queue.
>
> Restructure the macro so that apply_fn attributes return directly after
> apply_fn, and only non-apply_fn attributes write dev->NAME -- those are
> only changeable while not CONFIGURED and have no live hardware state to
> mismatch.
>
> Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
> Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
> ---
>   drivers/block/null_blk/main.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index cab51301560e..e7555c47b671 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -360,13 +360,15 @@ nullb_device_##NAME##_store(struct config_item *item, const char *page,	\
>   	ret = nullb_device_##TYPE##_attr_store(&new_value, page, count);\
>   	if (ret < 0)							\
>   		return ret;						\
> -	if (apply_fn)							\
> +	if (apply_fn) {							\
>   		ret = apply_fn(dev, new_value);				\
> -	else if (test_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags)) 	\
> -		ret = -EBUSY;						\
> -	if (ret < 0)							\
> -		return ret;						\
> -	dev->NAME = new_value;						\
> +		if (ret < 0)						\
> +			return ret;					\
> +	} else {							\
> +		if (test_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags))	\
> +			return -EBUSY;					\
> +		dev->NAME = new_value;					\
> +	}								\
>   	return count;							\
>   }									\
>   CONFIGFS_ATTR(nullb_device_, NAME);

Your patch correctly addresses the synchronization issue regarding the
apply_fn attributes under the nullb_list_lock. However, even with this
restructuring, the 'else' block remains vulnerable to concurrent
unmarked accesses to dev->NAME.

It seems when the device is not powered on, multiple threads can still
concurrently call the _show and _store callbacks on the same attribute
across separate open configfs file descriptions.

While this may be considered a benign data race, it can trigger KCSAN
splats and may allow the compiler to perform potentially unsafe optimization
heuristics (such as load/store tearing or merging). So I think we should
at-least mark those accesses using WRITE_ONCE() and READ_ONCE(). This also
help silence the KCSAN splat if it's configured.

Thanks,
--Nilay