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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] block/zloop: restore_elements strays elements in RESTORE_ERROR state
- [Medium] block/zloop: zloop_report_zones overwrites OFFLINE/READONLY zone conditions
--

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

This commit introduces storage element emulation to the zloop driver,
providing ways to emulate SMR disk depopulation features. It defines new
options and states to handle fractional access storage elements and their
associated zone conditions.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index 394dc2408ed73..d6a45f2148576 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c
[ ... ]
> @@ -353,6 +389,19 @@ static int zloop_update_seq_zone(struct zloop_device *zlo, unsigned int zone_no)
>  	return 0;
>  }
>  
> +static bool zloop_zone_is_offline_or_readonly(struct zloop_device *zlo,
> +					      struct zloop_zone *zone)
> +{
> +	bool ret;
> +
> +	spin_lock(&zone->wp_lock);
> +	ret = zone->cond == BLK_ZONE_COND_OFFLINE ||
> +		zone->cond == BLK_ZONE_COND_READONLY;
> +	spin_unlock(&zone->wp_lock);
> +
> +	return ret;
> +}
> +
>  static int zloop_open_zone(struct zloop_device *zlo, unsigned int zone_no)
>  {
>  	struct zloop_zone *zone = &zlo->zones[zone_no];
> @@ -363,6 +412,11 @@ static int zloop_open_zone(struct zloop_device *zlo, unsigned int zone_no)
>  
>  	mutex_lock(&zone->lock);
>  
> +	if (zloop_zone_is_offline_or_readonly(zlo, zone)) {
> +		ret = -EIO;
> +		goto unlock;
> +	}
> +
>  	if (test_and_clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags)) {
>  		ret = zloop_update_seq_zone(zlo, zone_no);

[Severity: Medium]
Should we also be checking zloop_zone_is_offline_or_readonly() in
zloop_report_zones()?

Currently, if a user depopulates a storage element, the zone transitions
to BLK_ZONE_COND_OFFLINE or BLK_ZONE_COND_READONLY. But if a report zones
command is executed, zloop_report_zones() checks for the
ZLOOP_ZONE_SEQ_ERROR bit and invokes zloop_update_seq_zone():

drivers/block/zloop.c:zloop_report_zones() {
...
    if (test_and_clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags)) {
        ret = zloop_update_seq_zone(zlo, zone_no);
...
}

Since zloop_report_zones() does not check if the zone is offline or
readonly first, zloop_update_seq_zone() could unconditionally overwrite
zone->cond to EMPTY, FULL, or CLOSED. Could this incorrectly revive the
zone and erase the status imposed by the storage element depopulation?

[ ... ]
> @@ -1002,11 +1105,252 @@ static int zloop_report_zones(struct gendisk *disk, sector_t sector,
[ ... ]
> +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]
Is it safe to unconditionally clear zlo->nr_removed_elements here when
elements might remain in the BLK_SE_STS_RESTORE_ERROR state?

If a zone reset fails (e.g. zloop_do_reset_zone() returns an error), the
affected elements transition to BLK_SE_STS_RESTORE_ERROR. However, the
code still sets zlo->nr_removed_elements = 0 despite these elements
remaining functionally disabled.

This breaks the accounting for the hardware limit check in
zloop_remove_element():

drivers/block/zloop.c:zloop_remove_element() {
...
    if (zlo->nr_removed_elements + nr_remove >
        zlo->max_nr_removed_elements) {
        ret = -EBUSY;
        goto unlock;
    }
...
}

This could allow a user to evade the zlo->max_nr_removed_elements limit
by deliberately failing restorations, potentially offlining all storage
elements.

Additionally, because nr_removed_elements becomes 0, elements in the
RESTORE_ERROR state might become stranded. In zloop_restore_elements():

> +static int zloop_restore_elements(struct gendisk *disk)
> +{
[ ... ]
> +	zlo->restore_in_progress = true;
> +	for (i = 0, se = zlo->elements; i < zlo->nr_elements; i++, se++) {
> +		if (se->status == BLK_SE_STS_REMOVED)
> +			se->status = BLK_SE_STS_RESTORE_IN_PROGRESS;
> +	}
> +
> +	schedule_delayed_work(&zlo->restore_elements_work,
> +			      msecs_to_jiffies(5000));
[ ... ]
> +}

Since zloop_restore_elements() strictly checks for BLK_SE_STS_REMOVED, it
will ignore elements in the BLK_SE_STS_RESTORE_ERROR state. Does this mean
those elements can never be restored again, stranding them in a
permanently broken state?

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