From: Zizhi Wo 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, once one store's apply_fn has reconfigured the hardware, a second (losing) store can still overwrite dev->NAME afterwards. This leaves dev->submit_queues out of sync with the live queue count, which is later caught by the WARN_ON_ONCE() in null_map_queues(). For !apply_fn attributes, power_store()'s null_add_dev() validates and builds the device under "lock" but only sets CONFIGURED afterwards. A store slipping in during this window can change a field mid-setup -- for example, zone_nr_conv can be pushed above nr_zones after it has already been clamped, leading to an 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. Also reset ret to 0 after the input parsing so that within the locked section ret is purely a status code, rather than carrying the byte count returned by nullb_device_##TYPE##_attr_store(). The field is then written only on success via if (!ret), giving a single consistent rule for both the apply_fn and the APPLY=NULL paths. Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") Signed-off-by: Zizhi Wo --- drivers/block/null_blk/main.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 340ecc0a331e..a411db6a8c40 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -358,17 +358,21 @@ nullb_device_##NAME##_store(struct config_item *item, const char *page, \ int ret; \ \ ret = nullb_device_##TYPE##_attr_store(&new_value, page, count);\ if (ret < 0) \ return ret; \ + ret = 0; \ + 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) \ + dev->NAME = new_value; \ + mutex_unlock(&lock); \ if (ret < 0) \ return ret; \ - dev->NAME = new_value; \ return count; \ } \ CONFIGFS_ATTR(nullb_device_, NAME); static int nullb_update_nr_hw_queues(struct nullb_device *dev, @@ -428,29 +432,17 @@ 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); NULLB_DEVICE_ATTR(completion_nsec, ulong, NULL); NULLB_DEVICE_ATTR(submit_queues, uint, nullb_apply_submit_queues); -- 2.52.0