Re: [PATCH v5 5/9] zloop: add storage element emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] block: array underflow in zloop_remove_element() via element_id == 0
- [High] block: UAF of disk->queue in delayed works during zloop_free_disk()
- [High] block: double-free of elements and other structures in zloop_ctl_add() error path
- [High] block/zloop: softirq deadlock on wp_lock in process context
- [Medium] block: emulation state inconsistency on partial restore failure
- [Medium] block: zone management commands bypass element removal lock-out
--

commit 1027b8e5f9403129fc0dca5c5db75877129cfc64
Author: Damien Le Moal <dlemoal@kernel.org>

    zloop: add storage element emulation

This commit adds storage element emulation to the zloop driver, simulating
the depopulation feature of SMR disks. It allows mapping zones to specific
storage elements to test offline behaviors and transitions.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index 394dc2408ed73..11481577b9cb2 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c
> @@ -985,6 +1011,41 @@ static bool zloop_prep_rq(struct zloop_device *zlo, struct request *rq)
>  {
>  	struct zloop_zone *zone = &zlo->zones[rq_zone_no(rq)];
>  	bool is_write = op_is_write(req_op(rq));
>  	bool ret = true;
>  
>  	spin_lock(&zone->wp_lock);

[Severity: High]
Can this cause a softirq deadlock? The zloop_prep_rq() function is called from
zloop_queue_rq(), which runs in softirq context. It acquires zone->wp_lock
using a plain spin_lock(). Later in this file, several new process-context
functions (such as zloop_remove_element_work and zloop_restore_conv_zone) also
acquire zone->wp_lock without disabling softirqs. If a process holding the
lock is preempted by a softirq attempting to queue I/O to the same zone, it
could result in a deadlock.

>  
> +	if (zlo->nr_elements) {
> +		struct blk_se *se;
> +
> +		/*
> +		 * Check the zone condition and the health state of the storage
> +		 * element serving the zone: do not allow any operation to
> +		 * offline zones or if the storage element serving the zone is
> +		 * being removed, and allow only reads to read-only zones.
> +		 */
> +		if (is_write)
> +			se = &zlo->elements[zone->wr_se_id - 1];
> +		else
> +			se = &zlo->elements[zone->rd_se_id - 1];
> +		if (READ_ONCE(se->status) == BLK_SE_STS_REMOVE_IN_PROGRESS) {
> +			ret = false;
> +			goto unlock;
> +		}
> +
> +		if (zone->cond == BLK_ZONE_COND_OFFLINE ||
> +		    (zone->cond == BLK_ZONE_COND_READONLY && is_write)) {
> +			ret = false;
> +			goto unlock;
> +		}
> +	}
> +
[ ... ]
> @@ -1040,11 +1188,19 @@ static blk_status_t zloop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	}
>  
>  	/*
> -	 * If we need to strongly order zone append operations, set the request
> -	 * sector to the zone write pointer location now instead of when the
> -	 * command work runs.
> +	 * Do not allow commands while the restoration of storage elements is
> +	 * in progress.
>  	 */
> -	if (zlo->ordered_zone_append && req_op(rq) == REQ_OP_ZONE_APPEND) {
> -		if (!zloop_set_zone_append_sector(rq))
> +	if (READ_ONCE(zlo->restore_in_progress))
> +		return BLK_STS_IOERR;
> +
> +	switch (req_op(rq)) {
> +	case REQ_OP_READ:
> +	case REQ_OP_WRITE:
> +	case REQ_OP_ZONE_APPEND:
> +		if (!zloop_prep_rq(zlo, rq))
>  			return BLK_STS_IOERR;
> +		break;
> +	default:
> +		break;
>  	}

[Severity: Medium]
Do zone management commands bypass the element removal lock-out? Operations
like REQ_OP_ZONE_RESET are not handled by zloop_prep_rq() here, meaning they
do not check if the storage element is in the BLK_SE_STS_REMOVE_IN_PROGRESS
state. Could this allow them to proceed on zones belonging to a storage
element that is actively undergoing depopulation?

[ ... ]
> +static void zloop_remove_element_work(struct work_struct *work)
> +{
> +	struct zloop_device *zlo = container_of(work, struct zloop_device,
> +						remove_element_work.work);
> +	struct blk_se *se, *paired_se = NULL;
> +	enum blk_zone_cond cond;
> +	struct zloop_zone *zone;
> +	unsigned int memflags, i;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Wait for all IOs to complete. */
> +	memflags = blk_mq_freeze_queue(zlo->disk->queue);
> +	blk_mq_unfreeze_queue(zlo->disk->queue, memflags);

[Severity: High]
Does this worker access the queue after it may have been freed? When the disk
is removed, disk_release() drops the queue reference and schedules it for RCU
free, followed by a call to zloop_free_disk() which calls
cancel_delayed_work_sync(). If this worker executes during that time, could it
result in a use-after-free on zlo->disk->queue here?

[ ... ]
> +static int zloop_remove_element(struct gendisk *disk, unsigned int element_id)
> +{
> +	struct zloop_device *zlo = disk->private_data;
> +	struct blk_se *se, *paired_se = NULL;
> +	unsigned int nr_remove = 1;
> +	int ret = 0;
> +
> +	if (zlo->stor_elements == ZLOOP_STOR_ELEMENTS_NONE)
> +		return -EOPNOTSUPP;
> +
> +	if (element_id > zlo->nr_elements)
> +		return -EINVAL;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	if (zlo->remove_element_id || READ_ONCE(zlo->restore_in_progress)) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	/*
> +	 * Get the element to remove. If it is already removed, we have nothing
> +	 * to do.
> +	 */
> +	se = &zlo->elements[element_id - 1];

[Severity: High]
Is it possible for element_id to be 0 here? The bounds check above prevents
element_id > zlo->nr_elements, but does not check for element_id == 0.
If element_id is 0, element_id - 1 could underflow and result in an
out-of-bounds array access on zlo->elements.

[ ... ]
> +static void zloop_restore_elements_work(struct work_struct *work)
> +{
> +	struct zloop_device *zlo = container_of(work, struct zloop_device,
> +						restore_elements_work.work);
> +	struct zloop_zone *zone;
> +	struct blk_se *se;
> +	unsigned int memflags, i;
> +	enum blk_se_status sts;
> +	int ret = 0;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Wait for all IOs to complete. */
> +	memflags = blk_mq_freeze_queue(zlo->disk->queue);
> +	blk_mq_unfreeze_queue(zlo->disk->queue, memflags);
> +
> +	/* Reset all zones. */
> +	for (i = 0, zone = zlo->zones; i < zlo->nr_zones; i++, zone++) {
> +		if (test_bit(ZLOOP_ZONE_CONV, &zone->flags)) {
> +			zloop_restore_conv_zone(zlo, zone);
> +			continue;
> +		}
> +		ret = zloop_restore_seq_zone(zlo, zone);
> +		if (ret)
> +			break;
> +	}
> +
> +	/*
> +	 * If we had an error, maintain all elements as removed instead of
> +	 * using the restore error status.
> +	 */
> +	if (ret)
> +		sts = BLK_SE_STS_REMOVED;
> +	else
> +		sts = BLK_SE_STS_OK;
> +
> +	/* Restore all removed elements. */
> +	for (i = 0, se = zlo->elements; i < zlo->nr_elements; i++, se++) {
> +		if (READ_ONCE(se->status) == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +			WRITE_ONCE(se->status, sts);
> +	}

[Severity: Medium]
If zloop_restore_seq_zone() fails and breaks the loop, does this leave the
already-processed zones in the BLK_ZONE_COND_EMPTY state? Since the element
status reverts to BLK_SE_STS_REMOVED on failure, this mismatch might trick
zloop_prep_rq() into allowing I/O to those EMPTY zones, even though their
storage element is officially removed.

[ ... ]
>  static void zloop_free_disk(struct gendisk *disk)
>  {
>  	struct zloop_device *zlo = disk->private_data;
>  	unsigned int i;
>  
> +	cancel_delayed_work_sync(&zlo->remove_element_work);
> +	cancel_delayed_work_sync(&zlo->restore_elements_work);
> +
>  	blk_mq_free_tag_set(&zlo->tag_set);
>  
>  	for (i = 0; i < zlo->nr_zones; i++) {
> @@ -1019,15 +1434,24 @@ static void zloop_free_disk(struct gendisk *disk)
>  
>  	fput(zlo->data_dir);
>  	destroy_workqueue(zlo->workqueue);
> +	kfree(zlo->elements);
>  	kfree(zlo->base_dir);
>  	kvfree(zlo);
>  }
[ ... ]
> @@ -1384,6 +1915,8 @@ static int zloop_ctl_add(struct zloop_options *opts)
>  	kfree(zlo->base_dir);
>  out_destroy_workqueue:
>  	destroy_workqueue(zlo->workqueue);
> +out_destroy_storage_elements:
> +	kfree(zlo->elements);
>  out_free_idr:
>  	mutex_lock(&zloop_ctl_mutex);
>  	idr_remove(&zloop_index_idr, zlo->id);
>  	mutex_unlock(&zloop_ctl_mutex);
>  out_free_dev:
>  	kvfree(zlo);

[Severity: High]
Does this error path cause a double-free? If add_disk() fails, the code jumps
to out_cleanup_disk, which calls put_disk(zlo->disk). This drops the final
reference and invokes zloop_free_disk(), successfully freeing zlo->elements,
the workqueue, and zlo itself. Continuing the fall-through then explicitly
calls kfree(zlo->elements) and kvfree(zlo) again on the already-freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012130121.1902634-1-dlemoal@kernel.org?part=5