Re: [PATCH V3 5/6] null_blk: serialize configfs attribute stores with device setup

[Zizhi Wo]

在 2026/7/8 15:39, Zizhi Wo 写道:
> From: Zizhi Wo <wozizhi@xxxxxxxxxx>
>
> The NULLB_DEVICE_ATTR _store takes no lock: apply_fn attributes
> (submit_queues, poll_queues) get dev->NAME written again after apply_fn
> returns, outside its lock; APPLY=NULL attributes are entirely lockless.
> configfs only serializes stores per-open-file, so concurrent stores on
> separate fds race.
>
> For apply_fn attributes the loser can overwrite dev->NAME after the
> winner's apply_fn reconfigured hardware, mismatching dev->submit_queues
> with the live queue count (null_map_queues WARN_ON_ONCE). For APPLY=NULL
> attributes, a store during power_store's null_add_dev() (validates and
> builds the device under "lock" but sets CONFIGURED only afterwards) can
> change a field mid-setup -- e.g. zone_nr_conv pushed above nr_zones after
> clamping, causing out-of-bounds dev->zones[] access.
>
> Take "lock" in the macro around the apply_fn call, the CONFIGURED test and
> the field write, and move it out of nullb_apply_submit_queues()/
> nullb_apply_poll_queues() so both paths are covered once. This serializes
> stores with power_store's setup and with each other.
>
> _show still takes no lock; its data races are handled by READ_ONCE/
> WRITE_ONCE in a follow-up.
>
> 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 | 21 ++++++---------------
>   1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index df85189f0b69..9e0002e4aeec 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -360,13 +360,16 @@ 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;						\
> +	mutex_lock(&lock);						\
>   	if (apply_fn)							\
>   		ret = apply_fn(dev, new_value);				\
>   	else if (test_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags)) 	\
>   		ret = -EBUSY;						\
> +	if (!ret)							\

Sorry for the noise. The condition here should be ret >= 0; I'll fix
this in the next version. :(

> +		dev->NAME = new_value;					\
> +	mutex_unlock(&lock);						\
>   	if (ret < 0)							\
>   		return ret;						\
> -	dev->NAME = new_value;						\
>   	return count;							\
>   }									\
>   CONFIGFS_ATTR(nullb_device_, NAME);
> @@ -421,25 +424,13 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
>   static int nullb_apply_submit_queues(struct nullb_device *dev,
>   				     unsigned int submit_queues)
>   {
> -	int ret;
> -
> -	mutex_lock(&lock);
> -	ret = nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues);
> -	mutex_unlock(&lock);
> -
> -	return ret;
> +	return nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues);
>   }
>   
>   static int nullb_apply_poll_queues(struct nullb_device *dev,
>   				   unsigned int poll_queues)
>   {
> -	int ret;
> -
> -	mutex_lock(&lock);
> -	ret = nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues);
> -	mutex_unlock(&lock);
> -
> -	return ret;
> +	return nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues);
>   }
>   
>   NULLB_DEVICE_ATTR(size, ulong, NULL);