Re: [PATCH 5/7] zloop: add degrade_element control command

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: logic inversion in zloop_zone_healthy() aborts I/O on healthy zones
- [High] block: Use-After-Free in zloop_ctl_degrade_element() due to missing refcount
- [Medium] block: data race on se->status without READ_ONCE/WRITE_ONCE wrappers
--

commit 003661eebb0c9d5db84ec05ab948b11a2ca35348
Author: Damien Le Moal <dlemoal@kernel.org>

zloop: add degrade_element control command

This patch adds a "degrade_element" control command to zloop devices, allowing
users to mark specific storage elements as degraded. Depending on the element's
access type, read or write operations targeting its zones will be failed.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index 7e6b5cc8017d2..1d51060b832bc 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c

[ ... ]

> @@ -201,6 +205,29 @@ static unsigned int rq_zone_no(struct request *rq)
>  	return blk_rq_pos(rq) >> zlo->zone_shift;
>  }
>  
> +static bool zloop_zone_healthy(struct zloop_device *zlo,
> +			       struct zloop_zone *zone, bool write)
> +{
> +	struct blk_storage_element *se;
> +
> +	if (!zlo->nr_elements)
> +		return true;
> +
> +	/* Check the zone condition first. */
> +	if (zone->cond == BLK_ZONE_COND_OFFLINE)
> +		return false;
> +	if (zone->cond == BLK_ZONE_COND_READONLY && write)
> +		return false;
> +
> +	/* Check the health state of the storage element serving the zone. */
> +	if (write)
> +		se = &zlo->elements[zone->wr_se_id - 1];
> +	else
> +		se = &zlo->elements[zone->rd_se_id - 1];
> +
> +	return se->status == BLK_SE_STS_DEGRADED;

[Severity: Medium]
Since this status field is read locklessly in the fast I/O path while it can
be concurrently updated by zloop_degrade_element(), should this use
READ_ONCE() to prevent compiler optimizations from tearing the read or
caching the variable?

[Severity: High]
Does this evaluate the health condition inversely? It returns true when the
element is degraded and false when it is healthy.

> +}
> +

[ ... ]

> @@ -733,9 +760,7 @@ 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)
> +	if (!zloop_zone_healthy(zlo, zone, is_write))
>  		goto out;

[Severity: High]
Because zloop_zone_healthy() returns false for healthy zones, does this
inverted check systematically abort valid I/Os to operating zones?

>  
>  	if (test_and_clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags)) {

[ ... ]

> @@ -1095,6 +1120,31 @@ static int zloop_report_elements(struct gendisk *disk,
>  	return 0;
>  }
>  
> +static int zloop_degrade_element(struct zloop_device *zlo,
> +				 unsigned int element_id)
> +{
> +	struct blk_storage_element *se;
> +	int ret = 0;
> +
> +	if (zlo->stor_elements == ZLOOP_STOR_ELEMENTS_NONE)
> +		return -EOPNOTSUPP;
> +
> +	if (!element_id || element_id > zlo->nr_elements)
> +		return -EINVAL;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	se = &zlo->elements[element_id - 1];
> +	if (se->status == BLK_SE_STS_OK)
> +		se->status = BLK_SE_STS_DEGRADED;

[Severity: Medium]
To match the lockless read in zloop_zone_healthy(), should this update be
wrapped in WRITE_ONCE() to ensure the memory access ordering?

> +	else
> +		ret = -EINVAL;
> +
> +	mutex_unlock(&zlo->stor_elements_lock);
> +
> +	return ret;
> +}
> +

[ ... ]

> @@ -1904,6 +1954,39 @@ static int zloop_ctl_remove(struct zloop_options *opts)
>  	return 0;
>  }
>  
> +static int zloop_ctl_degrade_element(struct zloop_options *opts)
> +{
> +	struct zloop_device *zlo;
> +	int ret = 0;
> +
> +	if (!(opts->mask & ZLOOP_OPT_ID)) {
> +		pr_err("No ID specified for degrade_element\n");
> +		return -EINVAL;
> +	}
> +
> +	if (opts->mask & ~(ZLOOP_OPT_ID | ZLOOP_OPT_ELEMENT_ID)) {
> +		pr_err("Invalid option specified for degrade_element\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&zloop_ctl_mutex);
> +	zlo = idr_find(&zloop_index_idr, opts->id);
> +	if (!zlo || zlo->state == Zlo_creating)
> +		ret = -ENODEV;
> +	else if (zlo->state == Zlo_deleting)
> +		ret = -EINVAL;
> +	mutex_unlock(&zloop_ctl_mutex);
> +	if (ret)
> +		return ret;
> +
> +	ret = zloop_degrade_element(zlo, opts->element_id);

[Severity: High]
Is zlo exposed to a concurrent free here? The mutex is unlocked without holding
a reference to zlo->disk via get_disk().

This could allow a concurrent zloop_ctl_remove() operation to remove the IDR
entry, unlock the same mutex, drop the final disk reference, and free the
zlo structure right before it is used in zloop_degrade_element().

> +	if (!ret)
> +		pr_info("Degraded element %u of device %u\n",
> +			opts->id, opts->element_id);
> +
> +	return ret;
> +}

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