From: Zizhi Wo 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 Signed-off-by: Zizhi Wo --- 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