Re: [PATCH 4/7] 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/zloop: integer overflow in ordered zone append causes wp corruption on OFFLINE zones
- [Medium] block/zloop: failed storage element restore permanently bricks device
- [Medium] block/zloop: storage element emulation reports incorrect zone counts due to division truncation
- [Medium] block/zloop: lockless TOCTOU race allows I/O on OFFLINE and READONLY storage elements
- [Low] block/zloop: double counting of nr_removed_elements when removing READ elements
- [Low] block/zloop: misleading error message for invalid stor_elements option
--

commit 2ec54a3b571357c5ded6a5a4c9a7aea473cb335d
Author: Damien Le Moal <dlemoal@kernel.org>

zloop: add storage element emulation

This commit introduces storage element emulation to the zloop driver. It
allows for emulating SMR disks' storage element depopulation feature,
controlled by a new stor_elements option. The operations to manage storage
elements are specified via zloop_se_ops.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index f0ca221524db6..7e6b5cc8017d2 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c
> @@ -664,6 +733,11 @@ static void zloop_rw(struct zloop_cmd *cmd)
>  			 zone->start + zlo->zone_size))
>  		goto out;
>  
> +	if (zone->cond == BLK_ZONE_COND_OFFLINE)
> +		goto out;
> +	if (zone->cond == BLK_ZONE_COND_READONLY && is_write)
> +		goto out;
> +

[Severity: Medium]
Could this lockless check introduce a TOCTOU race window?

Concurrently, zloop_remove_element_work() can acquire zone->lock and change
the state to OFFLINE or READONLY.

If the work function runs between this lockless check and the dispatch of the
I/O, zloop_do_rw() could be executed on a disabled storage element, as it
bypasses taking zone->lock for reads and conventional zone writes.

[ ... ]

> @@ -877,7 +951,7 @@ static bool zloop_set_zone_append_sector(struct request *rq)
>  	rq->__sector = zone->wp;
>  	zone->wp += blk_rq_sectors(rq);
>  	if (zone->wp >= zone_end)
> -		zloop_mark_full(zlo, zone);
> +		zloop_set_zone_full(zlo, zone);
>  
>  	spin_unlock(&zone->wp_lock);

[Severity: High]
Does this bounds check properly handle OFFLINE/READONLY zones?

When a zone becomes OFFLINE or READONLY, its zone->wp is set to ULLONG_MAX.

If an ordered ZONE_APPEND arrives for this zone, the bounds check in
zloop_set_zone_append_sector():

	if (zone->cond == BLK_ZONE_COND_FULL ||
	    zone->wp + nr_sectors > zone_end) {

could evaluate to false because ULLONG_MAX + nr_sectors can overflow to a
small sector number. This would bypass the bounds check and update zone->wp
to the overflowed value, corrupting the write pointer state.

[ ... ]

> @@ -985,11 +1059,251 @@ static int zloop_report_zones(struct gendisk *disk, sector_t sector,
[ ... ]
> +static int zloop_remove_element(struct gendisk *disk, unsigned int element_id)
> +{
[ ... ]
> +	/*
> +	 * If the element to remove is a read element, the associated write
> +	 * element must also be removed.
> +	 */
> +	if (se->type == BLK_SE_TYPE_READ) {
> +		paired_se = &zlo->elements[se->paired_id - 1];
> +		nr_remove = 2;
> +	}

[Severity: Low]
Does this code double-count nr_removed_elements if the paired WRITE element
was already manually removed?

zloop_remove_element() allows targeting a paired WRITE element and overwriting
paired_se->status to BLK_SE_STS_REMOVE_IN_PROGRESS without validating if it
was already removed.

Then zloop_remove_element_work() unconditionally increments
zlo->nr_removed_elements for both elements:

	zlo->nr_removed_elements++;
	if (paired_se) {
		paired_se->status = BLK_SE_STS_REMOVED;
		zlo->nr_removed_elements++;
	}

This could artificially inflate nr_removed_elements and prematurely trigger
the max_nr_removed_elements limit.

[ ... ]

> +static void zloop_restore_elements_work(struct work_struct *work)
> +{
[ ... ]
> +	/* 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)
> +			continue;
> +		if (!ret)
> +			se->status = BLK_SE_STS_OK;
> +		else
> +			se->status = BLK_SE_STS_RESTORE_ERROR;
> +	}
> +
> +	zlo->nr_removed_elements = 0;
> +	zlo->restore_in_progress = false;
> +
> +	mutex_unlock(&zlo->stor_elements_lock);
> +}

[Severity: Medium]
Will this permanently brick the device if a restore fails?

If zloop_do_reset_zone() fails, elements are put into
BLK_SE_STS_RESTORE_ERROR, but the code still unconditionally clears
zlo->nr_removed_elements.

A subsequent call to zloop_restore_elements() checks:

	if (!zlo->nr_removed_elements)
		goto unlock;

This prevents retrying the restore operation. Even if the check is bypassed,
the loop only targets BLK_SE_STS_REMOVED elements, leaving RESTORE_ERROR
elements permanently stuck.

[ ... ]

> +static int zloop_create_storage_elements(struct zloop_device *zlo)
> +{
[ ... ]
> +	nr_zones_per_element = zlo->nr_zones / nr_elements;
> +	capacity_per_element =
> +		(sector_t)nr_zones_per_element << zlo->zone_shift;
[ ... ]
> +	for (i = 0, se = zlo->elements; i < nr_elements; i++, se++) {
> +		se->id = i + 1;
> +		if (nrz + nr_zones_per_element > zlo->nr_zones)
> +			se->nr_zones = zlo->nr_zones - nrz;
> +		else
> +			se->nr_zones = nr_zones_per_element;

[Severity: Medium]
Does this code lose the remainder zones in its calculation?

Because nr_zones_per_element is the integer floor of
zlo->nr_zones / nr_elements, the condition
(nrz + nr_zones_per_element > zlo->nr_zones) will never evaluate to true.

Furthermore, elements are assigned to zones in a round-robin fashion (e.g.,
zone_no % nr_elems), meaning the number of zones per element should be
computed based on modulo arithmetic, not simple contiguous division. This
causes se->nr_zones to under-report the actual zones managed by the element.

[ ... ]

> @@ -1619,6 +2058,23 @@ static int zloop_parse_options(struct zloop_options *opts, const char *buf)
[ ... ]
> +		case ZLOOP_OPT_STOR_ELEMENTS:
> +			if (match_uint(args, &token)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			switch (token) {
> +			case ZLOOP_STOR_ELEMENTS_NONE:
> +			case ZLOOP_STOR_ELEMENTS_RDWR:
> +			case ZLOOP_STOR_ELEMENTS_PAIRS:
> +				break;
> +			default:
> +				pr_err("Invalid zone_append value\n");
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			opts->stor_elements = token;
> +			break;

[Severity: Low]
This isn't a bug, but the error message here says "Invalid zone_append value\n"
for the stor_elements option. This looks like a copy-paste error.

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