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

[Zizhi Wo]

在 2026/7/7 10:55, Zizhi Wo 写道:
> 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);

Sorry for the noise. I missed that nullb_update_nr_hw_queues() returns
early without storing the value when !dev->nullb, so dropping the
macro's trailing write would silently discard pre-power-on
configuration. I'll add the update logic on the !dev->nullb path in v3.

Any further comments are welcome.

Thanks,
Zizhi Wo