[PATCH V3 0/6] null_blk: fix init/exit races and memleaks

[PATCH V3 0/6] null_blk: fix init/exit races and memleaks

[Zizhi Wo]

This series fixes several issues in null_blk around lock initialization,
concurrent configfs access, and module init/exit.

Patch 1 fixes the uninitialized mutex. Following Bart's suggestion, the
fix now uses DEFINE_MUTEX(). This series no longer renames the lock;
following Damien Le Moal's suggestion, the rename and locking rework will
be sent as a separate series.

Patch 2 fixes configfs registration concurrency.

Patch 3 reorders resource release in null_exit() to match null_init().

Patch 4 fixes a global tag_set leak on the null_init() error path.

Patch 5 fixes a mid-setup race where a store changes a field before
CONFIGURED is set, causing out-of-bounds dev->zones[] access

Patch 6 adds READ_ONCE()/WRITE_ONCE() to the configfs attribute
reads/writes that race with each other.

Changes since v2:
- Dropped the lock rename patch; the rename and locking rework will be
  sent as a separate series (per Damien's suggestion).
- Patch 3: fixed the tense in the commit message.
- Patch 5: Mofify the lock position in v2 patch 4 to fix a mid-setup race.
- Added new patch 6.
https://lore.kernel.org/all/20260707025542.1299859-1-wozizhi@huaweicloud.com/

Changes since v1:
- Added patches 4-6, and modify the lock name in patch 2.
https://lore.kernel.org/all/20260706123507.3809871-1-wozizhi@huaweicloud.com/

Zizhi Wo (6):
  null_blk: use DEFINE_MUTEX for the file-scope mutex
  null_blk: register configfs subsystem after creating default devices
  null_blk: move unregister_blkdev() after destroying dev in null_exit()
  null_blk: free global tag_set on init error path
  null_blk: serialize configfs attribute stores with device setup
  null_blk: mark racy configfs attribute accesses with
    READ_ONCE/WRITE_ONCE

 drivers/block/null_blk/main.c  | 92 +++++++++++++++-------------------
 drivers/block/null_blk/zoned.c | 18 +++----
 2 files changed, 49 insertions(+), 61 deletions(-)

-- 
2.52.0

[PATCH V3 1/6] null_blk: use DEFINE_MUTEX for the file-scope mutex

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

In null_init(), mutex_init(&lock) currently happens after
configfs_register_subsystem(), which exposes the nullb subsystem to
userspace. A racing mkdir() into /sys/kernel/config/nullb/ can reach
null_find_dev_by_name() -> mutex_lock(&lock) before the mutex is
initialized, trigger warning:

[  123.137788] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[  123.137796] WARNING: kernel/locking/mutex.c:159 at mutex_lock+0x171/0x1c0, CPU#13: mkdir/1301
[  123.140090] Modules linked in: null_blk(+) nft_fib_inet nft_fib_ipv4
......
[  123.154926] Call Trace:
[  123.155172]  <TASK>
[  123.155419]  ? __pfx_mutex_lock+0x10/0x10
[  123.156181]  ? __pfx__raw_spin_lock+0x10/0x10
[  123.156571]  nullb_group_make_group+0x20/0x100 [null_blk]
[  123.157011]  configfs_mkdir+0x47b/0xc70
[  123.157337]  ? __pfx_configfs_mkdir+0x10/0x10
[  123.157719]  ? may_create_dentry+0x242/0x2e0
[  123.158061]  vfs_mkdir+0x2a9/0x6c0
[  123.158352]  filename_mkdirat+0x3dc/0x500
[  123.158710]  ? __pfx_filename_mkdirat+0x10/0x10
[  123.159070]  ? strncpy_from_user+0x3a/0x1d0
[  123.159413]  __x64_sys_mkdir+0x6b/0x90
[  123.159760]  do_syscall_64+0xea/0x600

Replace the runtime mutex_init(&lock) with a static DEFINE_MUTEX(lock)
declaration to fix this issue.

Fixes: 49c3b9266a71 ("block: null_blk: Improve device creation with configfs")
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/null_blk/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index f8c0fd57e041..eba204b27785 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -66,7 +66,7 @@ struct nullb_page {
 #define NULLB_PAGE_FREE (MAP_SZ - 2)
 
 static LIST_HEAD(nullb_list);
-static struct mutex lock;
+static DEFINE_MUTEX(lock);
 static int null_major;
 static DEFINE_IDA(nullb_indexes);
 static struct blk_mq_tag_set tag_set;
@@ -2166,8 +2166,6 @@ static int __init null_init(void)
 	if (ret)
 		return ret;
 
-	mutex_init(&lock);
-
 	null_major = register_blkdev(0, "nullb");
 	if (null_major < 0) {
 		ret = null_major;
-- 
2.52.0

[PATCH V3 2/6] null_blk: register configfs subsystem after creating default devices

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

In null_init(), configfs_register_subsystem() currently runs before
register_blkdev(), so when null_blk is built as a module, a racing mkdir()
+ poweron from userspace can reach null_add_dev() while null_major is still
0. __add_disk() then hits WARN_ON(disk->minors) (major=0 with minors!=0)
and fails:

[root@fedora ~]# [ 2366.521436] WARNING: block/genhd.c:476 at __add_disk+0x8a7/0xde0,
[ 2366.523552] Modules linked in: null_blk(+) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib
[ 2366.529081] CPU: 26 UID: 0 PID: 1600 Comm: sh Not tainted 7.2.0-rc1+ #66 PREEMPT(full)
......
[ 2366.547251] Call Trace:
[ 2366.547575]  <TASK>
[ 2366.547831]  ? _raw_spin_lock+0x84/0xe0
[ 2366.548260]  add_disk_fwnode+0x114/0x560
[ 2366.548739]  null_add_dev+0x102d/0x1b80 [null_blk]
[ 2366.549310]  ? __pfx_null_add_dev+0x10/0x10 [null_blk]
[ 2366.549906]  ? mutex_lock+0xde/0x1c0
[ 2366.550361]  ? __pfx_mutex_lock+0x10/0x10
[ 2366.550827]  nullb_device_power_store+0x1e7/0x280 [null_blk]
[ 2366.551499]  ? __pfx_nullb_device_power_store+0x10/0x10 [null_blk]
[ 2366.552177]  ? __kmalloc_cache_noprof+0x1f5/0x470
[ 2366.552748]  ? configfs_write_iter+0x35c/0x4e0
[ 2366.553242]  configfs_write_iter+0x286/0x4e0
[ 2366.553787]  vfs_write+0x52d/0xd00
[ 2366.554169]  ? __pfx_vfs_write+0x10/0x10
[ 2366.554679]  ? __pfx___css_rstat_updated+0x10/0x10
[ 2366.555196]  ? fdget_pos+0x1cf/0x4c0
[ 2366.555649]  ksys_write+0xfc/0x1d0
......

Additionally, the err_dev path destroys all devices on nullb_list while
configfs is still registered. If a racing mkdir() + poweron puts a user
device on the list, null_destroy_dev()->null_free_dev() kfrees the user
device's nullb_device but /sys/kernel/config/nullb/<name> is still
reachable. Any userspace access to the item will trigger a UAF.

For simplicity, move configfs_register_subsystem() to the end to solve
the problems above.

Fixes: 3bf2bd20734e ("nullb: add configfs interface")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/null_blk/main.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index eba204b27785..4613035222cd 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -2162,15 +2162,9 @@ static int __init null_init(void)
 	config_group_init(&nullb_subsys.su_group);
 	mutex_init(&nullb_subsys.su_mutex);
 
-	ret = configfs_register_subsystem(&nullb_subsys);
-	if (ret)
-		return ret;
-
 	null_major = register_blkdev(0, "nullb");
-	if (null_major < 0) {
-		ret = null_major;
-		goto err_conf;
-	}
+	if (null_major < 0)
+		return null_major;
 
 	for (i = 0; i < nr_devices; i++) {
 		ret = null_create_dev();
@@ -2178,6 +2172,10 @@ static int __init null_init(void)
 			goto err_dev;
 	}
 
+	ret = configfs_register_subsystem(&nullb_subsys);
+	if (ret)
+		goto err_dev;
+
 	pr_info("module loaded\n");
 	return 0;
 
@@ -2187,8 +2185,6 @@ static int __init null_init(void)
 		null_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
-err_conf:
-	configfs_unregister_subsystem(&nullb_subsys);
 	return ret;
 }
 
-- 
2.52.0

[PATCH V3 3/6] null_blk: move unregister_blkdev() after destroying dev in null_exit()

[Zizhi Wo]

In null_exit(), unregister_blkdev() is called before the null_blk instances
are destroyed, which is inconsistent with the cleanup order in null_init().
Move it after null_destroy_dev() so that teardown happens in the reverse
order of initialization.

No functional change intended.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/null_blk/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 4613035222cd..6cb213779cc5 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -2194,8 +2194,6 @@ static void __exit null_exit(void)
 
 	configfs_unregister_subsystem(&nullb_subsys);
 
-	unregister_blkdev(null_major, "nullb");
-
 	mutex_lock(&lock);
 	while (!list_empty(&nullb_list)) {
 		nullb = list_entry(nullb_list.next, struct nullb, list);
@@ -2203,6 +2201,8 @@ static void __exit null_exit(void)
 	}
 	mutex_unlock(&lock);
 
+	unregister_blkdev(null_major, "nullb");
+
 	if (tag_set.ops)
 		blk_mq_free_tag_set(&tag_set);
 
-- 
2.52.0

[PATCH V3 4/6] null_blk: free global tag_set on init error path

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

If shared_tags is enabled, null_setup_tagset() allocates the global tag_set
via null_init_global_tag_set(). If device creation later fails, err_dev
destroys the default devices and calls unregister_blkdev(), but never frees
the global tag_set. Since module init failed, null_exit() is never invoked,
so the global tag_set's tags and maps are permanently leaked.

Free the global tag_set in err_dev, matching null_exit() which does
if (tag_set.ops) blk_mq_free_tag_set(&tag_set).

Fixes: 82f402fefa50 ("null_blk: add support for shared tags")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/null_blk/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 6cb213779cc5..df85189f0b69 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -2185,6 +2185,8 @@ static int __init null_init(void)
 		null_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
+	if (tag_set.ops)
+		blk_mq_free_tag_set(&tag_set);
 	return ret;
 }
 
-- 
2.52.0

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

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

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@huawei.com>
---
 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)							\
+		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);
-- 
2.52.0

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

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

The _show callback in the NULLB_DEVICE_ATTR macro reads dev->NAME and the
_store path writes dev->NAME.  configfs does not serialize accesses across
separate open file descriptions (buffer->mutex is per-fd), and _show takes
no lock, so a concurrent read and write on the same attribute is a data
race. Mark the _show read with READ_ONCE() and the _store write with
WRITE_ONCE() in the macro. The non-macro "power" attribute has the same
issue between power_show and power_store and is marked the same way.

The same _show readers also race against writes to these fields outside
_store that run after the configfs item becomes visible:

1. nullb_update_nr_hw_queues() (submit_queues, poll_queues),
2. null_validate_conf() (queue_mode, submit_queues, poll_queues, irqmode,
blocking, cache_size, mbps),
3. null_config_discard() (discard),
4. null_init_zoned_dev() (zone_capacity, zone_nr_conv,
zone_append_max_sectors, zone_max_active, zone_max_open),
5. null_add_dev() (index).

These run under the file-scope lock (taken in the macro for _store, and in
power_store for setup), but _show does not take that lock, so a plain write
still races with the READ_ONCE() read. Mark all of them with WRITE_ONCE().

Writes in null_alloc_dev() are intentionally left as plain assignments: it
runs from the .make_group callback before the configfs item is published,
so _show cannot run concurrently with it. The dev->power write in
nullb_group_drop_item() is also left plain: configfs takes frag_sem for
write and sets frag_dead during rmdir, and drop_item runs only after that,
so attribute show/store (frag_sem readers) cannot be concurrent with it.

The setup-side reads in null_validate_conf()/null_config_discard()/etc.
no longer race with _store now that _store takes the file-scope lock; those
reads are therefore left plain. This patch closes the remaining
_show-vs-write data races.

Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 drivers/block/null_blk/main.c  | 47 +++++++++++++++++-----------------
 drivers/block/null_blk/zoned.c | 18 ++++++-------
 2 files changed, 33 insertions(+), 32 deletions(-)

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;						\
@@ -404,8 +404,8 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
 	 */
 	dev->prev_submit_queues = dev->submit_queues;
 	dev->prev_poll_queues = dev->poll_queues;
-	dev->submit_queues = submit_queues;
-	dev->poll_queues = poll_queues;
+	WRITE_ONCE(dev->submit_queues, submit_queues);
+	WRITE_ONCE(dev->poll_queues, poll_queues);
 
 	set = dev->nullb->tag_set;
 	nr_hw_queues = submit_queues + poll_queues;
@@ -414,8 +414,8 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
 
 	if (ret) {
 		/* on error, revert the queue numbers */
-		dev->submit_queues = dev->prev_submit_queues;
-		dev->poll_queues = dev->prev_poll_queues;
+		WRITE_ONCE(dev->submit_queues, dev->prev_submit_queues);
+		WRITE_ONCE(dev->poll_queues, dev->prev_poll_queues);
 	}
 
 	return ret;
@@ -469,7 +469,8 @@ NULLB_DEVICE_ATTR(badblocks_partial_io, bool, NULL);
 
 static ssize_t nullb_device_power_show(struct config_item *item, char *page)
 {
-	return nullb_device_bool_attr_show(to_nullb_device(item)->power, page);
+	return nullb_device_bool_attr_show(
+			READ_ONCE(to_nullb_device(item)->power), page);
 }
 
 static ssize_t nullb_device_power_store(struct config_item *item,
@@ -496,11 +497,11 @@ static ssize_t nullb_device_power_store(struct config_item *item,
 		}
 
 		set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
-		dev->power = newp;
+		WRITE_ONCE(dev->power, newp);
 		ret = count;
 	} else if (dev->power && !newp) {
 		if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) {
-			dev->power = newp;
+			WRITE_ONCE(dev->power, newp);
 			null_del_dev(dev->nullb);
 		}
 		clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
@@ -1783,13 +1784,13 @@ static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
 		return;
 
 	if (!nullb->dev->memory_backed) {
-		nullb->dev->discard = false;
+		WRITE_ONCE(nullb->dev->discard, false);
 		pr_info("discard option is ignored without memory backing\n");
 		return;
 	}
 
 	if (nullb->dev->zoned) {
-		nullb->dev->discard = false;
+		WRITE_ONCE(nullb->dev->discard, false);
 		pr_info("discard option is ignored in zoned mode\n");
 		return;
 	}
@@ -1881,31 +1882,31 @@ static int null_validate_conf(struct nullb_device *dev)
 	}
 	if (dev->queue_mode == NULL_Q_BIO) {
 		pr_err("BIO-based IO path is no longer available, using blk-mq instead.\n");
-		dev->queue_mode = NULL_Q_MQ;
+		WRITE_ONCE(dev->queue_mode, NULL_Q_MQ);
 	}
 
 	if (dev->use_per_node_hctx) {
 		if (dev->submit_queues != nr_online_nodes)
-			dev->submit_queues = nr_online_nodes;
+			WRITE_ONCE(dev->submit_queues, nr_online_nodes);
 	} else if (dev->submit_queues > nr_cpu_ids)
-		dev->submit_queues = nr_cpu_ids;
+		WRITE_ONCE(dev->submit_queues, nr_cpu_ids);
 	else if (dev->submit_queues == 0)
-		dev->submit_queues = 1;
+		WRITE_ONCE(dev->submit_queues, 1);
 	dev->prev_submit_queues = dev->submit_queues;
 
 	if (dev->poll_queues > g_poll_queues)
-		dev->poll_queues = g_poll_queues;
+		WRITE_ONCE(dev->poll_queues, g_poll_queues);
 	dev->prev_poll_queues = dev->poll_queues;
-	dev->irqmode = min_t(unsigned int, dev->irqmode, NULL_IRQ_TIMER);
+	WRITE_ONCE(dev->irqmode, min_t(unsigned int, dev->irqmode, NULL_IRQ_TIMER));
 
 	/* Do memory allocation, so set blocking */
 	if (dev->memory_backed)
-		dev->blocking = true;
+		WRITE_ONCE(dev->blocking, true);
 	else /* cache is meaningless */
-		dev->cache_size = 0;
-	dev->cache_size = min_t(unsigned long, ULONG_MAX / 1024 / 1024,
-						dev->cache_size);
-	dev->mbps = min_t(unsigned int, 1024 * 40, dev->mbps);
+		WRITE_ONCE(dev->cache_size, 0);
+	WRITE_ONCE(dev->cache_size, min_t(unsigned long, ULONG_MAX / 1024 / 1024,
+					  dev->cache_size));
+	WRITE_ONCE(dev->mbps, min_t(unsigned int, 1024 * 40, dev->mbps));
 
 	if (dev->zoned &&
 	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
@@ -2015,7 +2016,7 @@ static int null_add_dev(struct nullb_device *dev)
 		goto out_cleanup_disk;
 
 	nullb->index = rv;
-	dev->index = rv;
+	WRITE_ONCE(dev->index, rv);
 
 	if (config_item_name(&dev->group.cg_item)) {
 		/* Use configfs dir name as the device name */
diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c
index 384bdce6a9b7..75613eaf3ea9 100644
--- a/drivers/block/null_blk/zoned.c
+++ b/drivers/block/null_blk/zoned.c
@@ -66,7 +66,7 @@ int null_init_zoned_dev(struct nullb_device *dev,
 	}
 
 	if (!dev->zone_capacity)
-		dev->zone_capacity = dev->zone_size;
+		WRITE_ONCE(dev->zone_capacity, dev->zone_size);
 
 	if (dev->zone_capacity > dev->zone_size) {
 		pr_err("zone capacity (%lu MB) larger than zone size (%lu MB)\n",
@@ -99,29 +99,29 @@ int null_init_zoned_dev(struct nullb_device *dev,
 	spin_lock_init(&dev->zone_res_lock);
 
 	if (dev->zone_nr_conv >= dev->nr_zones) {
-		dev->zone_nr_conv = dev->nr_zones - 1;
+		WRITE_ONCE(dev->zone_nr_conv, dev->nr_zones - 1);
 		pr_info("changed the number of conventional zones to %u",
 			dev->zone_nr_conv);
 	}
 
-	dev->zone_append_max_sectors =
-		min(ALIGN_DOWN(dev->zone_append_max_sectors,
-			       dev->blocksize >> SECTOR_SHIFT),
-		    zone_capacity_sects);
+	WRITE_ONCE(dev->zone_append_max_sectors,
+		   min(ALIGN_DOWN(dev->zone_append_max_sectors,
+				  dev->blocksize >> SECTOR_SHIFT),
+		       zone_capacity_sects));
 
 	/* Max active zones has to be < nbr of seq zones in order to be enforceable */
 	if (dev->zone_max_active >= dev->nr_zones - dev->zone_nr_conv) {
-		dev->zone_max_active = 0;
+		WRITE_ONCE(dev->zone_max_active, 0);
 		pr_info("zone_max_active limit disabled, limit >= zone count\n");
 	}
 
 	/* Max open zones has to be <= max active zones */
 	if (dev->zone_max_active && dev->zone_max_open > dev->zone_max_active) {
-		dev->zone_max_open = dev->zone_max_active;
+		WRITE_ONCE(dev->zone_max_open, dev->zone_max_active);
 		pr_info("changed the maximum number of open zones to %u\n",
 			dev->zone_max_open);
 	} else if (dev->zone_max_open >= dev->nr_zones - dev->zone_nr_conv) {
-		dev->zone_max_open = 0;
+		WRITE_ONCE(dev->zone_max_open, 0);
 		pr_info("zone_max_open limit disabled, limit >= zone count\n");
 	}
 	dev->need_zone_res_mgmt = dev->zone_max_active || dev->zone_max_open;
-- 
2.52.0

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@huawei.com>
> 
> 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@huawei.com>
> ---
>   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);

Re: [PATCH V3 2/6] null_blk: register configfs subsystem after creating default devices

[Bart Van Assche]

On 7/8/26 12:39 AM, Zizhi Wo wrote:
> For simplicity, move configfs_register_subsystem() to the end to solve
> the problems above.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH V3 3/6] null_blk: move unregister_blkdev() after destroying dev in null_exit()

[Bart Van Assche]

On 7/8/26 12:39 AM, Zizhi Wo wrote:
> In null_exit(), unregister_blkdev() is called before the null_blk instances
> are destroyed, which is inconsistent with the cleanup order in null_init().
> Move it after null_destroy_dev() so that teardown happens in the reverse
> order of initialization.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH V3 4/6] null_blk: free global tag_set on init error path

[Bart Van Assche]

On 7/8/26 12:39 AM, Zizhi Wo wrote:
> Free the global tag_set in err_dev, matching null_exit() which does
> if (tag_set.ops) blk_mq_free_tag_set(&tag_set).
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

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

[Zizhi Wo]

在 2026/7/9 1:49, 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

Thanks for the suggestion.

Indeed, _store() now takes the lock unconditionally. Otherwise the
else-branch of _store() could run concurrently with
nullb_device_power_store() -> null_add_dev() during power-on and cause
other problems. So taking the lock in _show() directly avoids the races,
without scattering ~a dozen WRITE_ONCE() across two files.

Since this is not a hot path, and I/O hot paths such as
null_complete_cmd() are rejected from writing during power-on anyway,
this is fine.

I'll drop this patch and instead take "lock" in _show() (and in
power_show()) in the next revision.

Thanks,
Zizhi Wo