Re: [PATCH V2 5/6] null_blk: don't locklessly overwrite dev state after apply_fn
From: Zizhi Wo
Date: Tue Jul 07 2026 - 04:30:03 EST
在 2026/7/7 15:33, Nilay Shroff 写道:
On 7/7/26 8:25 AM, Zizhi Wo wrote:
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);
Your patch correctly addresses the synchronization issue regarding the
apply_fn attributes under the nullb_list_lock. However, even with this
restructuring, the 'else' block remains vulnerable to concurrent
unmarked accesses to dev->NAME.
It seems when the device is not powered on, multiple threads can still
concurrently call the _show and _store callbacks on the same attribute
across separate open configfs file descriptions.
While this may be considered a benign data race, it can trigger KCSAN
splats and may allow the compiler to perform potentially unsafe optimization
heuristics (such as load/store tearing or merging). So I think we should
at-least mark those accesses using WRITE_ONCE() and READ_ONCE(). This also
help silence the KCSAN splat if it's configured.
Thanks,
--Nilay
Thanks for catching this! The 'else' path can indeed be accessed
concurrently; I'd left it alone earlier assuming there was no real
impact, but from the angle you described, adding READ_ONCE()/
WRITE_ONCE() does make sense. I'll fix this in the next version.
Thanks,
Zizhi Wo