[PATCH V4 0/9] null_blk: fix init/exit races and memleaks

[PATCH V4 0/9] null_blk: fix init/exit races and memleaks

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

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

Patches 1-4 are unchanged from v3.
Patch 5 fixes another memory leak of the zones array, and patch 6 is a
related cleanup.
Patch 7 fixes a NULL-ptr-deref of hctx when shrinking submit_queues on a
device that shares the global tag_set.
Patch 8 fixes a data race, similar to patch 5 in v3, but adjusts the
return value handling.
Patch 9 serializes configfs attribute shows to close the remaining
show-vs-write races.

See the individual patch descriptions for details.

Changes since v3:
- Added patch 5 (zones array memleak) and patch 6 (cleanup).
- Added patch 7 (NULL-ptr-deref on shared tag_set queue shrink).
- Patch 9: reworked the fix from v3's patch 6 to take the file-scope
  lock in _store instead of scattering READ_ONCE/WRITE_ONCE.
https://lore.kernel.org/all/20260708073917.2172392-1-wozizhi@huaweicloud.com/

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 4: also update dev->NAME in the "!dev->nullb" path, which was
  previously lost.
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 (9):
  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: free zones array on device power-off
  null_blk: clean up null_del_dev() to use cached dev pointer
  null_blk: reject per-device queue resize for shared tag set
  null_blk: serialize configfs attribute stores with device setup
  null_blk: serialize configfs attribute shows with the file-scope lock

 drivers/block/null_blk/main.c | 79 +++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 35 deletions(-)

-- 
2.52.0

[PATCH V4 1/9] 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
@@ -64,11 +64,11 @@ struct nullb_page {
 };
 #define NULLB_PAGE_LOCK (MAP_SZ - 1)
 #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;
 
 enum {
@@ -2164,12 +2164,10 @@ static int __init null_init(void)
 
 	ret = configfs_register_subsystem(&nullb_subsys);
 	if (ret)
 		return ret;
 
-	mutex_init(&lock);
-
 	null_major = register_blkdev(0, "nullb");
 	if (null_major < 0) {
 		ret = null_major;
 		goto err_conf;
 	}
-- 
2.52.0

Re: [PATCH V4 1/9] null_blk: use DEFINE_MUTEX for the file-scope mutex

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> 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:

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 2/9] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.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
@@ -2160,37 +2160,33 @@ 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();
 		if (ret)
 			goto err_dev;
 	}
 
+	ret = configfs_register_subsystem(&nullb_subsys);
+	if (ret)
+		goto err_dev;
+
 	pr_info("module loaded\n");
 	return 0;
 
 err_dev:
 	while (!list_empty(&nullb_list)) {
 		nullb = list_entry(nullb_list.next, struct nullb, list);
 		null_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
-err_conf:
-	configfs_unregister_subsystem(&nullb_subsys);
 	return ret;
 }
 
 static void __exit null_exit(void)
 {
-- 
2.52.0

Re: [PATCH V4 2/9] null_blk: register configfs subsystem after creating default devices

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> 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:

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 3/9] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.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
@@ -2192,19 +2192,19 @@ static void __exit null_exit(void)
 {
 	struct nullb *nullb;
 
 	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);
 		null_destroy_dev(nullb);
 	}
 	mutex_unlock(&lock);
 
+	unregister_blkdev(null_major, "nullb");
+
 	if (tag_set.ops)
 		blk_mq_free_tag_set(&tag_set);
 
 	mutex_destroy(&lock);
 }
-- 
2.52.0

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

[Nilay Shroff]

On 7/9/26 3:34 PM, 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.
> 
> No functional change intended.

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 4/9] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.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
@@ -2183,10 +2183,12 @@ static int __init null_init(void)
 	while (!list_empty(&nullb_list)) {
 		nullb = list_entry(nullb_list.next, struct nullb, list);
 		null_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
+	if (tag_set.ops)
+		blk_mq_free_tag_set(&tag_set);
 	return ret;
 }
 
 static void __exit null_exit(void)
 {
-- 
2.52.0

Re: [PATCH V4 4/9] null_blk: free global tag_set on init error path

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> 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).

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 5/9] null_blk: free zones array on device power-off

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

null_init_zoned_dev() allocates dev->zones when a zoned device is powered
on, but null_del_dev() never frees it on power-off; dev->zones is only
freed later in null_free_dev(), when the configfs directory is removed. If
the device is powered off and then on again, null_init_zoned_dev()
allocates a new array and overwrites the dev->zones pointer, leaking the
previous allocation each power cycle.

Free dev->zones in null_del_dev() via null_free_zoned_dev() to solve it.
And calling null_del_dev() in null_free_dev() is no longer necessary
because every caller already invokes null_del_dev() first: via
nullb_group_drop_item() before nullb_device_release(), in the
null_add_dev() error path of null_create_dev(), and in null_destroy_dev().
Remove the redundant call.

Fixes: ca4b2a011948 ("null_blk: add zone support")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 drivers/block/null_blk/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index df85189f0b69..a75989220a2f 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -834,11 +834,10 @@ static struct nullb_device *null_alloc_dev(void)
 static void null_free_dev(struct nullb_device *dev)
 {
 	if (!dev)
 		return;
 
-	null_free_zoned_dev(dev);
 	badblocks_exit(&dev->badblocks);
 	kfree(dev);
 }
 
 static enum hrtimer_restart null_cmd_timer_expired(struct hrtimer *timer)
@@ -1775,10 +1774,11 @@ static void null_del_dev(struct nullb *nullb)
 		atomic_long_set(&nullb->cur_bytes, LONG_MAX);
 		blk_mq_start_stopped_hw_queues(nullb->q, true);
 	}
 
 	put_disk(nullb->disk);
+	null_free_zoned_dev(dev);
 	if (nullb->tag_set == &nullb->__tag_set)
 		blk_mq_free_tag_set(nullb->tag_set);
 	kfree(nullb->queues);
 	if (null_cache_active(nullb))
 		null_free_device_storage(nullb->dev, true);
-- 
2.52.0

Re: [PATCH V4 5/9] null_blk: free zones array on device power-off

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> null_init_zoned_dev() allocates dev->zones when a zoned device is powered
> on, but null_del_dev() never frees it on power-off; dev->zones is only
> freed later in null_free_dev(), when the configfs directory is removed. If
> the device is powered off and then on again, null_init_zoned_dev()
> allocates a new array and overwrites the dev->zones pointer, leaking the
> previous allocation each power cycle.
> 
> Free dev->zones in null_del_dev() via null_free_zoned_dev() to solve it.
> And calling null_del_dev() in null_free_dev() is no longer necessary
> because every caller already invokes null_del_dev() first: via
> nullb_group_drop_item() before nullb_device_release(), in the
> null_add_dev() error path of null_create_dev(), and in null_destroy_dev().
> Remove the redundant call.

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 6/9] null_blk: clean up null_del_dev() to use cached dev pointer

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

Replace remaining nullb->dev dereferences with the already-cached
local dev variable. No functional change.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 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 a75989220a2f..6d30591abb28 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1767,11 +1767,11 @@ static void null_del_dev(struct nullb *nullb)
 
 	list_del_init(&nullb->list);
 
 	del_gendisk(nullb->disk);
 
-	if (test_bit(NULLB_DEV_FL_THROTTLED, &nullb->dev->flags)) {
+	if (test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags)) {
 		hrtimer_cancel(&nullb->bw_timer);
 		atomic_long_set(&nullb->cur_bytes, LONG_MAX);
 		blk_mq_start_stopped_hw_queues(nullb->q, true);
 	}
 
@@ -1779,11 +1779,11 @@ static void null_del_dev(struct nullb *nullb)
 	null_free_zoned_dev(dev);
 	if (nullb->tag_set == &nullb->__tag_set)
 		blk_mq_free_tag_set(nullb->tag_set);
 	kfree(nullb->queues);
 	if (null_cache_active(nullb))
-		null_free_device_storage(nullb->dev, true);
+		null_free_device_storage(dev, true);
 	kfree(nullb);
 	dev->nullb = NULL;
 }
 
 static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
-- 
2.52.0

Re: [PATCH V4 6/9] null_blk: clean up null_del_dev() to use cached dev pointer

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> Replace remaining nullb->dev dereferences with the already-cached
> local dev variable. No functional change.
> 
> Signed-off-by: Zizhi Wo<wozizhi@huawei.com>

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 7/9] null_blk: reject per-device queue resize for shared tag set

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huawei.com>

When shared_tags is enabled, null_setup_tagset() makes the device use the
global tag_set, whose driver_data stays NULL. null_map_queues() therefore
falls back to the module-wide g_submit_queues/g_poll_queues instead of any
per-device value.

Resizing submit_queues or poll_queues via configfs on such a device calls
blk_mq_update_nr_hw_queues() on the shared set, shrinking
set->nr_hw_queues.  __blk_mq_realloc_hw_ctxs() only grows the
q->queue_hw_ctx[] allocation, so on shrink it merely exits and NULLs the
now-excess hctx slots. null_map_queues(), however, keeps mapping CPUs with
the unchanged g_submit_queues/g_poll_queues, so mq_map[] ends up pointing
at those NULLed hctx slots. blk_mq_map_swqueue() then dereferences the NULL
hctx (hctx->cpumask), crashing the kernel:

[  460.218374] KASAN: null-ptr-deref in range [0x0000000000000098-0x000000000000009f]
[  460.219003] CPU: 24 UID: 0 PID: 1492 Comm: sh Not tainted 7.2.0-rc2+ #67 PREEMPT(full)
[  460.219792] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-4.fc41 04/01/2014
[  460.220452] RIP: 0010:blk_mq_map_swqueue+0x4db/0x1430
......
[  460.228977] Call Trace:
[  460.229175]  <TASK>
[  460.229354]  blk_mq_update_nr_hw_queues+0xd49/0x11c0
[  460.229779]  ? __pfx_blk_mq_update_nr_hw_queues+0x10/0x10
[  460.230200]  nullb_update_nr_hw_queues+0x1a9/0x370 [null_blk]
[  460.230694]  nullb_device_submit_queues_store+0xd9/0x170 [null_blk]
[  460.231190]  ? __pfx_nullb_device_submit_queues_store+0x10/0x10 [null_blk]
[  460.231776]  ? configfs_write_iter+0x35c/0x4e0
[  460.232122]  configfs_write_iter+0x286/0x4e0
[  460.232460]  vfs_write+0x52d/0xd00
[  460.232779]  ? __x64_sys_openat+0x108/0x1d0
[  460.233106]  ? __pfx_vfs_write+0x10/0x10
[  460.233413]  ? fdget_pos+0x1cf/0x4c0
[  460.233745]  ? fput_close+0x133/0x190
[  460.234038]  ? __pfx_expand_files+0x10/0x10
[  460.234368]  ksys_write+0xfc/0x1d0

Reproducer:
modprobe null_blk shared_tags=1 submit_queues=64 poll_queues=1
mkdir /sys/kernel/config/nullb/dev
echo 1 > /sys/kernel/config/nullb/dev/power
echo 1 > /sys/kernel/config/nullb/dev/submit_queues

A per-device resize of a shared tag set is meaningless anyway, so reject it
with -EINVAL in nullb_update_nr_hw_queues() when the device is bound to the
global tag_set.

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 6d30591abb28..340ecc0a331e 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -380,10 +380,19 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
 	int ret, nr_hw_queues;
 
 	if (!dev->nullb)
 		return 0;
 
+	/*
+	 * A shared tag_set is mapped via the module-wide queue counts, so a
+	 * per-device resize is meaningless. On shrink it would also leave
+	 * mq_map[] pointing at NULLed hctx slots, causing a NULL deref in
+	 * blk_mq_map_swqueue(). Reject it.
+	 */
+	if (dev->nullb->tag_set == &tag_set)
+		return -EINVAL;
+
 	/*
 	 * Make sure at least one submit queue exists.
 	 */
 	if (!submit_queues)
 		return -EINVAL;
-- 
2.52.0

Re: [PATCH V4 7/9] null_blk: reject per-device queue resize for shared tag set

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> From: Zizhi Wo <wozizhi@huawei.com>
> 
> When shared_tags is enabled, null_setup_tagset() makes the device use the
> global tag_set, whose driver_data stays NULL. null_map_queues() therefore
> falls back to the module-wide g_submit_queues/g_poll_queues instead of any
> per-device value.
> 
> Resizing submit_queues or poll_queues via configfs on such a device calls
> blk_mq_update_nr_hw_queues() on the shared set, shrinking
> set->nr_hw_queues.  __blk_mq_realloc_hw_ctxs() only grows the
> q->queue_hw_ctx[] allocation, so on shrink it merely exits and NULLs the
> now-excess hctx slots. null_map_queues(), however, keeps mapping CPUs with
> the unchanged g_submit_queues/g_poll_queues, so mq_map[] ends up pointing
> at those NULLed hctx slots. blk_mq_map_swqueue() then dereferences the NULL
> hctx (hctx->cpumask), crashing the kernel:
> 
> [  460.218374] KASAN: null-ptr-deref in range [0x0000000000000098-0x000000000000009f]
> [  460.219003] CPU: 24 UID: 0 PID: 1492 Comm: sh Not tainted 7.2.0-rc2+ #67 PREEMPT(full)
> [  460.219792] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-4.fc41 04/01/2014
> [  460.220452] RIP: 0010:blk_mq_map_swqueue+0x4db/0x1430
> ......
> [  460.228977] Call Trace:
> [  460.229175]  <TASK>
> [  460.229354]  blk_mq_update_nr_hw_queues+0xd49/0x11c0
> [  460.229779]  ? __pfx_blk_mq_update_nr_hw_queues+0x10/0x10
> [  460.230200]  nullb_update_nr_hw_queues+0x1a9/0x370 [null_blk]
> [  460.230694]  nullb_device_submit_queues_store+0xd9/0x170 [null_blk]
> [  460.231190]  ? __pfx_nullb_device_submit_queues_store+0x10/0x10 [null_blk]
> [  460.231776]  ? configfs_write_iter+0x35c/0x4e0
> [  460.232122]  configfs_write_iter+0x286/0x4e0
> [  460.232460]  vfs_write+0x52d/0xd00
> [  460.232779]  ? __x64_sys_openat+0x108/0x1d0
> [  460.233106]  ? __pfx_vfs_write+0x10/0x10
> [  460.233413]  ? fdget_pos+0x1cf/0x4c0
> [  460.233745]  ? fput_close+0x133/0x190
> [  460.234038]  ? __pfx_expand_files+0x10/0x10
> [  460.234368]  ksys_write+0xfc/0x1d0
> 
> Reproducer:
> modprobe null_blk shared_tags=1 submit_queues=64 poll_queues=1
> mkdir /sys/kernel/config/nullb/dev
> echo 1 > /sys/kernel/config/nullb/dev/power
> echo 1 > /sys/kernel/config/nullb/dev/submit_queues
> 
> A per-device resize of a shared tag set is meaningless anyway, so reject it
> with -EINVAL in nullb_update_nr_hw_queues() when the device is bound to the
> global tag_set.
> 
> 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 | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 6d30591abb28..340ecc0a331e 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -380,10 +380,19 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
>   	int ret, nr_hw_queues;
>   
>   	if (!dev->nullb)
>   		return 0;
>   
> +	/*
> +	 * A shared tag_set is mapped via the module-wide queue counts, so a
> +	 * per-device resize is meaningless. On shrink it would also leave
> +	 * mq_map[] pointing at NULLed hctx slots, causing a NULL deref in
> +	 * blk_mq_map_swqueue(). Reject it.
> +	 */
> +	if (dev->nullb->tag_set == &tag_set)
> +		return -EINVAL;
> +

Wouldn't it be simpler to check dev->shared_tags here instead? Since the restriction
is specifically for devices configured with shared tags, that seems a bit easier to
reason about than checking whether dev->nullb->tag_set points to the global tag_set.
Something like this:

if (dev->shared_tags)
         return -EINVAL;
Thanks,
--Nilay

[PATCH V4 8/9] 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, 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 <wozizhi@huawei.com>
---
 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

Re: [PATCH V4 8/9] null_blk: serialize configfs attribute stores with device setup

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> 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, 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 <wozizhi@huawei.com>

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V4 9/9] null_blk: serialize configfs attribute shows with the file-scope lock

[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 it. 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. The
_show readers also race against writes to these fields that run after the
configfs item becomes visible, e.g. in nullb_update_nr_hw_queues().

All of those writers now run under the file-scope lock: _store takes it
unconditionally, and the setup-side writers run under power_store() which
holds the same lock. The only remaining unsynchronized accesses are the
plain reads in _show. Rather than annotating every field with
READ_ONCE()/WRITE_ONCE() across files, simply take the file-scope lock in
_show (and in power_show) as well. This closes the remaining _show-vs-write
data races with a single lock and keeps the writers as plain assignments.

configfs attribute access is not on the I/O hot path, so taking the mutex
in _show is acceptable from a performance standpoint. The dev fields
written in null_alloc_dev() and dev->power in nullb_group_drop_item() need
no locking: the former runs from .make_group before the item is published,
and the latter is serialized by configfs frag_sem/frag_dead against
attribute show/store.

Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 drivers/block/null_blk/main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index a411db6a8c40..e829b502981b 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -343,12 +343,18 @@ static ssize_t nullb_device_bool_attr_store(bool *val, const char *page,
 /* The following macro should only be used with TYPE = {uint, ulong, bool}. */
 #define NULLB_DEVICE_ATTR(NAME, TYPE, APPLY)				\
 static ssize_t								\
 nullb_device_##NAME##_show(struct config_item *item, char *page)	\
 {									\
-	return nullb_device_##TYPE##_attr_show(				\
+	ssize_t ret;							\
+									\
+	mutex_lock(&lock);						\
+	ret = nullb_device_##TYPE##_attr_show(				\
 				to_nullb_device(item)->NAME, page);	\
+	mutex_unlock(&lock);						\
+									\
+	return ret;							\
 }									\
 static ssize_t								\
 nullb_device_##NAME##_store(struct config_item *item, const char *page,	\
 			    size_t count)				\
 {									\
@@ -477,11 +483,17 @@ NULLB_DEVICE_ATTR(rotational, bool, NULL);
 NULLB_DEVICE_ATTR(badblocks_once, bool, NULL);
 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);
+	ssize_t ret;
+
+	mutex_lock(&lock);
+	ret = nullb_device_bool_attr_show(to_nullb_device(item)->power, page);
+	mutex_unlock(&lock);
+
+	return ret;
 }
 
 static ssize_t nullb_device_power_store(struct config_item *item,
 				     const char *page, size_t count)
 {
-- 
2.52.0

Re: [PATCH V4 9/9] null_blk: serialize configfs attribute shows with the file-scope lock

[Nilay Shroff]

On 7/9/26 3:34 PM, Zizhi Wo wrote:
> The _show callback in the NULLB_DEVICE_ATTR macro reads dev->NAME and the
> _store path writes it. 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. The
> _show readers also race against writes to these fields that run after the
> configfs item becomes visible, e.g. in nullb_update_nr_hw_queues().
> 
> All of those writers now run under the file-scope lock: _store takes it
> unconditionally, and the setup-side writers run under power_store() which
> holds the same lock. The only remaining unsynchronized accesses are the
> plain reads in _show. Rather than annotating every field with
> READ_ONCE()/WRITE_ONCE() across files, simply take the file-scope lock in
> _show (and in power_show) as well. This closes the remaining _show-vs-write
> data races with a single lock and keeps the writers as plain assignments.
> 
> configfs attribute access is not on the I/O hot path, so taking the mutex
> in _show is acceptable from a performance standpoint. The dev fields
> written in null_alloc_dev() and dev->power in nullb_group_drop_item() need
> no locking: the former runs from .make_group before the item is published,
> and the latter is serialized by configfs frag_sem/frag_dead against
> attribute show/store.
> 
> Suggested-by: Nilay Shroff<nilay@linux.ibm.com>
> Signed-off-by: Zizhi Wo<wozizhi@huawei.com>

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>