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

[Zizhi Wo]

From: Zizhi Wo <wozizhi@xxxxxxxxxx>

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@xxxxxxxxxxxxx>
Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
---
 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