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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: NULL pointer dereference if optional se_ops methods are not implemented
- [Medium] block: disk_wait_for_se_mgmt_completion() ignores hardware errors and falsely reports success
- [Medium] block: disk_wait_for_se_mgmt_completion() overwrites successful completion with -EINTR
--

commit d3d300e29a3d6ef944841ecbfddc83f5a8fabe94
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. This feature is intended for managing hard-disks
heads, either combined read-write heads or pairs of read and write heads.

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

[ ... ]

> +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;
> +	unsigned int noio_flag;
> +	int ret;
> +
> +	/* The callers already checked that disk->fops->se_ops is set. */
> +	ret = disk->fops->se_ops->report_elements(disk, NULL, &nr_elements);

[Severity: High]
Does this cause a NULL pointer dereference if the device driver provides
se_ops but leaves the optional report_elements method as NULL?

[ ... ]

> +		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: Medium]
Will this loop falsely report success if a hardware operation fails?

If the hardware encounters an error and updates the element's status to
BLK_SE_STS_REMOVE_ERROR or BLK_SE_STS_RESTORE_ERROR, the status is no
longer IN_PROGRESS. As a result, the loop terminates successfully
(i >= nr_se) and the function returns 0. Will the caller then assume the
element was successfully depopulated or restored, masking the hardware
failure?

> +
> +		/* Not done yet: wait and retry. */
> +		msleep(1000);
> +	}
> +
> +	if (!ret && fatal_signal_pending(current))
> +		ret = -EINTR;

[Severity: Medium]
Does this logic mistakenly overwrite a successful completion with an
error if a signal arrives concurrently?

If the polling loop detects the operation finished successfully and breaks
out, but a fatal signal is delivered at the exact same time, this will set
ret = -EINTR.

Since the callers bdev_remove_storage_element() and
bdev_restore_storage_elements() abort on an error return, won't this
cause them to skip the mandatory blk_revalidate_disk_zones() step, leaving
the kernel's zone state permanently out-of-sync with the physical hardware
geometry?

[ ... ]

> +int bdev_report_storage_elements(struct block_device *bdev,
> +				 struct blk_storage_element *elements,
> +				 unsigned int *nr_elements)
> +{
> +	struct gendisk *disk = bdev->bd_disk;
> +
> +	if (!bdev_is_zoned(bdev) || !disk->fops->se_ops)
> +		return -EOPNOTSUPP;
> +
> +	if (!nr_elements)
> +		return -EINVAL;
> +
> +	if (*nr_elements && !elements)
> +		return -EINVAL;
> +
> +	return disk->fops->se_ops->report_elements(disk, elements, nr_elements);

[Severity: High]
Can this code cause a NULL pointer dereference if report_elements is not
implemented by the driver?

> +}
> +EXPORT_SYMBOL_GPL(bdev_report_storage_elements);

[ ... ]

> +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]
Can this trigger a NULL pointer dereference if the driver provides se_ops
but omits the remove_element method?

[ ... ]

> +int bdev_restore_storage_elements(struct block_device *bdev)
> +{
> +	struct gendisk *disk = bdev->bd_disk;
> +	unsigned int memflags;
> +	int ret;
> +
> +	if (!bdev_is_zoned(bdev) || !disk->fops->se_ops)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Freeze and unfreeze the queue to flush any outstanding commands.
> +	 * The caller is responsible for not queuing up more I/O by higher
> +	 * level means.
> +	 */
> +	memflags = blk_mq_freeze_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue, memflags);
> +
> +	ret = disk->fops->se_ops->restore_elements(disk);

[Severity: High]
Does this code trigger a NULL pointer dereference if the driver provides
se_ops but leaves restore_elements as NULL?

The commit message notes that "Repopulating defective storage elements
is generally not allowed by devices", suggesting drivers will commonly
omit restore_elements.

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