Re: [PATCH v3 4/7] zloop: add storage element emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: permanent device bricking due to uncleared restore_in_progress flag
- [High] block: restored storage elements' zones are permanently left offline or read-only
- [Medium] block: TOCTOU race allows I/O on OFFLINE/READONLY zones during removal
--

commit 20ddc2153af7f05d1115bfa8b2509e5c9f65e4f8
Author: Damien Le Moal <dlemoal@kernel.org>
Subject: zloop: add storage element emulation

This commit adds storage element emulation to the zloop driver,
allowing simulation of SMR disk fractional access storage elements.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index 394dc2408ed73..e113ecd63c0b2 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c

[ ... ]

> @@ -430,20 +511,19 @@ static int zloop_reset_zone(struct zloop_device *zlo, unsigned int zone_no)
>  
>  	mutex_lock(&zone->lock);
>  
> +	if (zloop_zone_is_offline_or_readonly(zlo, zone)) {
> +		if (!all_zones)
> +			ret = -EIO;
> +		goto unlock;
> +	}
> +
>  	if (!test_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags) &&
>  	    zone->cond == BLK_ZONE_COND_EMPTY)
>  		goto unlock;

[Severity: High]
In zloop_restore_elements_work(), the code calls zloop_reset_all_zones()
to reset the zones of restored elements, which in turn calls this function
as zloop_reset_zone(..., all_zones=true). 

Since zloop_zone_is_offline_or_readonly() evaluates to true for these
removed elements' zones, the function jumps to unlock and skips 
zloop_do_reset_zone(). 

Does this permanently leave the restored elements' zones offline or
read-only, preventing the restoration from completing successfully?

[ ... ]

> +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);
> +
> +	if (zlo->nr_elements) {
> +		if (zone->cond == BLK_ZONE_COND_OFFLINE ||
> +		    (zone->cond == BLK_ZONE_COND_READONLY && is_write)) {
> +			ret = false;
> +			goto unlock;
> +		}
> +	}

[Severity: Medium]
Is there a time-of-check to time-of-use race condition here?

This validates the zone condition against offline and read-only states
before adding the I/O request to the workqueue. If a concurrently
executing zloop_remove_element_work() transitions the zone condition to
offline while the request is pending in the queue, the request could still
be processed.

Once popped from the queue, zloop_handle_cmd() calls zloop_rw(), which
executes reads and conventional zone writes without re-acquiring the zone
lock or re-validating the zone condition. Could this allow reads and writes
to execute against offlined zones?

[ ... ]

> +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 blk_storage_element *se;
> +	unsigned int i;
> +	int ret;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Reset all zones. */
> +	ret = zloop_reset_all_zones(zlo);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* Restore all removed elements. */
> +	for (i = 0, se = zlo->elements; i < zlo->nr_elements; i++, se++) {
> +		if (se->status == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +			se->status = BLK_SE_STS_OK;
> +	}
> +
> +	zlo->nr_removed_elements = 0;
> +	WRITE_ONCE(zlo->restore_in_progress, false);
> +
> +out_unlock:
> +	mutex_unlock(&zlo->stor_elements_lock);
> +}

[Severity: High]
If zloop_reset_all_zones() fails (for example, if vfs_truncate encounters
an I/O error), this code branches to out_unlock and skips clearing the
restore_in_progress flag. 

Since zlo->restore_in_progress would remain true, zloop_queue_rq() appears
to unconditionally reject all incoming block I/O with BLK_STS_IOERR
afterward. Does this leave the device permanently bricked on a reset
failure?

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