Re: [PATCH v8 11/12] scsi: sd_zbc: define storage element management operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] scsi: sd_zbc: unbounded loop and missed error in
  sd_zbc_wait_for_se_removal()
- [Medium] scsi: sd_zbc: restore_allowed check masks error in
  sd_zbc_wait_for_se_restore()
- [Medium] scsi: sd_zbc: wrong loop bound on rep->elements in SE wait
  helpers
--

commit 9a8493bd583f59e5d376896ba0dad2854e23bbdd
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: sd_zbc: define storage element management operations

This commit implements block storage element management operations
(report, remove, and restore) for zoned SCSI disks using physical element
status and depopulation commands.

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index bf2e60071d577..3bad1392f0e25 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
[ ... ]
> +static int sd_zbc_report_se(struct scsi_disk *sdkp,
> +			    struct sd_zbc_se_report *rep)
> +{
[ ... ]
> +	desc = &rep->buf[32];
> +	for (i = 0; i < min(rep->nr_elements, nr_descs); i++, desc += 32)
> +		sd_zbc_parse_storage_element(sdkp, desc, &rep->elements[i]);
> +	rep->nr_reported_elements = i;
> +
> +	return 0;
> +}
[ ... ]
> +static int sd_zbc_wait_for_se_removal(struct scsi_disk *sdkp,
> +				      struct sd_zbc_se_report *rep,
> +				      unsigned int element_id)
> +{
> +	bool element_checked, element_removed;
> +	int ret, in_progress;
> +	unsigned int i;
> +
> +	/*
> +	 * Poll the storage elements health to see that we get the element we
> +	 * are interested in and that its removal completed. Since we do not
> +	 * know how long the operation may take, we cannot have a timeout on
> +	 * this loop. So check for fatal signals so that the user can terminate
> +	 * this loop if the device misbehaves.
> +	 */
> +	do {
> +		/* Let the operation proceed and check. */
> +		msleep(1000);
> +
> +		ret = sd_zbc_report_se(sdkp, rep);
> +		if (ret)
> +			return ret;
> +
> +		element_checked = false;
> +		element_removed = false;
> +		in_progress = 0;
> +		for (i = 0; i < rep->nr_elements; i++) {

[Severity: Medium]
Should this loop (and the matching loop in sd_zbc_wait_for_se_restore())
bound i by rep->nr_reported_elements instead of rep->nr_elements?

sd_zbc_report_se() only populates min(rep->nr_elements, nr_descs) entries
in rep->elements[] and stores that count in rep->nr_reported_elements.  If
the device returns fewer than rep->nr_elements descriptors, the remaining
entries in rep->elements[] are unpopulated on the first pass and retain
stale status values from earlier passes across the polling loop, which can
keep in_progress non-zero or trigger false error checks.

> +			if (rep->elements[i].id == element_id)
> +				element_checked = true;
> +			switch (rep->elements[i].status) {
> +			case BLK_SE_STS_REMOVED:
> +				if (rep->elements[i].id == element_id)
> +					element_removed = true;
> +				break;
> +			case BLK_SE_STS_REMOVE_IN_PROGRESS:
> +				in_progress++;
> +				break;
> +			case BLK_SE_STS_REMOVE_ERROR:
> +				if (rep->elements[i].id == element_id)
> +					return -EIO;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +
> +		if (!element_checked)
> +			return -EIO;
> +		if (element_removed && !in_progress)
> +			return 0;
> +	} while (!fatal_signal_pending(current));

[Severity: Medium]
Can sd_zbc_wait_for_se_removal() loop until a fatal signal if the removal
stops making progress without element_removed becoming true?

Unlike sd_zbc_wait_for_se_restore(), which returns as soon as !in_progress,
this loop keeps polling when in_progress is 0 and element_removed is false
(for example, if the operation aborts and leaves the element in its prior
health state, or if a paired fractional-access element fails removal while
the target element does not transition to removed or error).  While stuck
in this loop, the caller holds disk->zone_revalidate_mutex and blocks other
zone revalidation and storage element operations on the disk.

In addition, returning immediately from the removal-error case inside the
descriptor loop can return before a paired element finishes its in-progress
removal, and checking only rep->elements[i].id == element_id misses a
removal error reported on a paired element.

[ ... ]

> +static int sd_zbc_wait_for_se_restore(struct scsi_disk *sdkp,
> +				      struct sd_zbc_se_report *rep)
> +{
[ ... ]
> +		in_progress = 0;
> +		for (i = 0; i < rep->nr_elements; i++) {
> +			switch (rep->elements[i].status) {
> +			case BLK_SE_STS_RESTORE_IN_PROGRESS:
> +				in_progress++;
> +				break;
> +			case BLK_SE_STS_REMOVED:
> +			case BLK_SE_STS_RESTORE_ERROR:
> +				if (rep->elements[i].restore_allowed)
> +					ret = -EIO;
> +				break;

[Severity: Medium]
Does gating the restore-error case on rep->elements[i].restore_allowed mask
restoration failures when the device clears the restoration-allowed bit on
the failed element?

While a still-removed element is only an error if it was eligible for
restoration (rep->elements[i].restore_allowed), a restore-error status
means restoration was attempted and failed; if the device clears
restore_allowed when entering that error state, sd_zbc_wait_for_se_restore()
returns 0 instead of -EIO.

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