Re: [PATCH V2 5/6] null_blk: don't locklessly overwrite dev state after apply_fn

[Zizhi Wo <wozizhi@huaweicloud.com>]

在 2026/7/7 15:33, Nilay Shroff 写道:
> On 7/7/26 8:25 AM, Zizhi Wo wrote:
>> From: Zizhi Wo <wozizhi@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   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