Re: [PATCH v2 2/7] block: introduce storage element management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] block: memory allocation deadlock in disk_wait_for_se_mgmt_completion()
- [High] block: NULL pointer dereference on missing se_ops callbacks
- [High] block: ignored asynchronous hardware error statuses in disk_wait_for_se_mgmt_completion()
- [High] block: unkillable task hang due to unbounded hardware polling loop
- [High] block: TOCTOU data race from unfrozen queue during blk_revalidate_disk_zones()
- [Low] block: UAPI copy-paste error in blk_storage_element_type docstring
--

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

block: introduce storage element management

Recent SCSI (SBC) and ATA (ACS) standards define the storage element
depopulation feature, which allows keeping disks with defective heads
longer in production by removing them. This introduces generic operations
and functions to block device drivers for managing storage elements.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 131c9f50b3da5..be57644ee2663 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c

[ ... ]

> @@ -2718,5 +2719,162 @@ int queue_zone_wplugs_show(void *data, struct seq_file *m)
>  
>  	return 0;
>  }
> -
>  #endif
> +
> +static int disk_wait_for_se_mgmt_completion(struct gendisk *disk)
> +{
> +	struct blk_storage_element *elements, *e;
> +	unsigned int i, nr_se, nr_elements = 0;
> +	int ret;
> +
> +	ret = disk->fops->se_ops->report_elements(disk, NULL, &nr_elements);

[Severity: High]
Should there be a NULL check for the specific se_ops function pointers
(report_elements, remove_element, and restore_elements) before invoking them
to prevent a kernel panic?

The commit message notes that some operations like repopulating defective
elements are generally not allowed by devices, which suggests drivers might
legitimately leave restore_elements or other operations as NULL.

> +	if (ret) {
> +		pr_err("Failed to get number of storage elements\n");
> +		return ret;
> +	}
> +
> +	elements = kzalloc_objs(struct blk_storage_element, nr_elements);

[Severity: High]
Should this allocation be protected by memalloc_noio_save()?

This function is called by storage element management APIs like
bdev_remove_storage_element() where I/O might be paused or frozen. An
allocation using the default GFP_KERNEL could enter direct reclaim. 

If the reclaim attempts to write back dirty pages to the frozen filesystem,
could this cause an indefinite block and deadlock?

> +	if (!elements)
> +		return -ENOMEM;
> +
> +	while (1) {

[Severity: High]
Does this unbounded loop risk an unkillable task hang?

If a device malfunctions, firmware stalls, or the completion event drops, the
task will block indefinitely in the D state because msleep() below uses
TASK_UNINTERRUPTIBLE.

There are no bounds, timeout limits, or checks for fatal_signal_pending().

> +		/*
> +		 * Check if we have storage elements being removed or restored.
> +		 */
> +		nr_se = nr_elements;
> +		ret = disk->fops->se_ops->report_elements(disk, elements,
> +							  &nr_se);
> +		if (ret) {
> +			pr_err("Failed to get storage elements\n");
> +			break;
> +		}
> +
> +		e = elements;
> +		for (i = 0; i < nr_se; i++, e++) {
> +			if (e->status == BLK_SE_STS_REMOVE_IN_PROGRESS ||
> +			    e->status == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +				break;
> +		}
> +		if (i >= nr_se)
> +			break;

[Severity: High]
This polling loop treats ERROR states like BLK_SE_STS_REMOVE_ERROR and
BLK_SE_STS_RESTORE_ERROR as not in progress, causing the loop to break
normally and falsely return success (0).

Could this mislead a caller into assuming offlined zones are safe to format or
ignore, potentially leading to data corruption?

> +
> +		/* Not done yet: wait and retry. */
> +		msleep(500);
> +	}
> +
> +	kfree(elements);
> +
> +	return ret;
> +}

[ ... ]

> +int bdev_remove_storage_element(struct block_device *bdev,
> +				unsigned int element_id)
> +{
> +	struct gendisk *disk = bdev->bd_disk;
> +	unsigned int memflags;
> +	int ret;
> +
> +	if (!bdev_is_zoned(bdev) || !disk->fops->se_ops)
> +		return -EOPNOTSUPP;
> +
> +	/* Zero is not a valid storage element ID. */
> +	if (!element_id)
> +		return -EINVAL;
> +
> +	/*
> +	 * Freeze and unfreeze the queue to flush any outstanding command.
> +	 * The caller is responsible for not queuing up more I/Os by higher
> +	 * level means.
> +	 */
> +	memflags = blk_mq_freeze_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue, memflags);
> +
> +	ret = disk->fops->se_ops->remove_element(disk, element_id);

[Severity: High]
By deliberately unfreezing the queue before the hardware removal command and
the subsequent call to blk_revalidate_disk_zones(), does this introduce a
TOCTOU race?

The queue remains unfrozen during the first half of
blk_revalidate_disk_zones(), which could allow concurrent write I/Os to bypass
validation and allocate new blk_zone_wplug entries based on stale state.

When the queue is finally frozen to install the new zone array, these freshly
created stale write plugs aren't purged, and bios might proceed to write to
the offlined device zones.

> +	if (ret)
> +		return ret;
> +
> +	/* Revalidate the device zones once the opration completes. */
> +	ret = disk_wait_for_se_mgmt_completion(disk);
> +	if (ret)
> +		return ret;
> +
> +	return blk_revalidate_disk_zones(disk);
> +}

[ ... ]

> diff --git a/include/uapi/linux/blkzoned.h b/include/uapi/linux/blkzoned.h
> index 6638361209667..f8f2c2651dffa 100644
> --- a/include/uapi/linux/blkzoned.h
> +++ b/include/uapi/linux/blkzoned.h

[ ... ]

> +/**
> + * enum blk_storage_element_status - Status of a zoned device storage elements.
> + *
> + * @BLK_SE_TYPE_RDWR: The storage element handles both reads and writes.
> + * @BLK_SE_TYPE_READ: The storage element handles reads only.
> + * @BLK_SE_TYPE_WRITE: The storage element handles writes only.
> + * @BLK_SE_TYPE_UNKNOWN: The storage element type is not known.
> + */
> +enum blk_storage_element_type {

[Severity: Low]
this isn't a bug, but the docstring incorrectly identifies the enum as
enum blk_storage_element_status.

Could this copy-paste error be fixed to enum blk_storage_element_type to avoid
confusion in userspace?

> +	BLK_SE_TYPE_RDWR		= 0x01,
> +	BLK_SE_TYPE_READ		= 0x02,

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