Re: [PATCH v4 3/9] block: introduce storage element management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block: permanent zone state desync on signal or memory allocation failure
- [Medium] block: ignored storage element management error status
--

commit 47105e9f6e8f70ce8710089fec4d093f5ff51fde
Author: Damien Le Moal <dlemoal@kernel.org>
Subject: block: introduce storage element management

This commit introduces storage element management for zoned block devices. It
defines generic operations to report, remove, and restore storage elements
so that disks with defective heads can remain in production by depopulating
the defective elements.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
[ ... ]
> +static int disk_se_mgmt_in_progress(struct gendisk *disk,
> +				     struct blk_se *elements,
> +				     bool *in_progress)
> +{
[ ... ]
> +	for (i = 0; i < nr_se; i++) {
> +		switch (elements[i].status) {
> +		case BLK_SE_STS_REMOVE_IN_PROGRESS:
> +		case BLK_SE_STS_RESTORE_IN_PROGRESS:
> +			*in_progress = true;
> +			return 0;
> +		default:
> +			break;

[Severity: Medium]
Do error statuses like BLK_SE_STS_REMOVE_ERROR fall through to the default 
case here?

If the device encounters an error during the removal, it changes the element's 
status to an error state. The polling loop then sees no elements are in 
progress, sets in_progress to false, and returns 0 instead of bubbling up 
the error.

> +		}
> +	}
> +
> +	*in_progress = false;
> +
> +	return 0;
> +}
> +
> +static int disk_wait_for_se_mgmt_completion(struct gendisk *disk)
> +{
> +	struct blk_se *elements;
> +	unsigned int noio_flag;
> +	bool in_progress;
> +	int ret;
> +
> +	noio_flag = memalloc_noio_save();
> +	elements = kzalloc_objs(struct blk_se, disk->nr_storage_elements);
> +	memalloc_noio_restore(noio_flag);
> +	if (!elements)
> +		return -ENOMEM;

[Severity: High]
Does attempting to allocate the polling buffer after the hardware operation 
has already been initiated introduce a memory failure path that aborts 
synchronization?

If kzalloc_objs fails and returns -ENOMEM, the wait loop is skipped, which 
later bypasses the critical disk_revalidate_zones step in 
bdev_remove_storage_element. Could this leave the kernel's zone state 
permanently out of sync with the physical device conditions?

> +
> +	/*
> +	 * Poll the storage elements heath state to detect the end of a
> +	 * removal or restoration operation. 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 retry loop if
> +	 * the device misbehaves.
> +	 */
> +	while (1) {
> +		ret = disk_se_mgmt_in_progress(disk, elements, &in_progress);
> +		if (ret || !in_progress)
> +			break;
> +
> +		if (fatal_signal_pending(current)) {
> +			ret = -EINTR;
> +			break;

[Severity: High]
Does permitting fatal signals to abandon the wait loop leave the hardware 
executing in the background with no mechanism to trigger a revalidation 
upon actual completion?

This error path propagates the aborted wait and also skips the vital zone 
revalidation step that keeps the kernel's view consistent with the device.

> +		}
> +
> +		/* Not done yet: wait and retry. */
> +		msleep(1000);
> +	}
> +
> +	kfree(elements);
> +
> +	return ret;
> +}
[ ... ]
> +int bdev_remove_storage_element(struct block_device *bdev,
> +				unsigned int element_id)
> +{
[ ... ]
> +	ret = disk->fops->se_ops->remove_element(disk, element_id);
> +	if (ret)
> +		goto unlock;
> +
> +	/* Revalidate the device zones once the opration completes. */
> +	ret = disk_wait_for_se_mgmt_completion(disk);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = disk_revalidate_zones(disk);
> +
> +unlock:
> +	mutex_unlock(&disk->zone_revalidate_mutex);
> +
> +	return ret;

[Severity: Medium]
Because disk_se_mgmt_in_progress returns 0 on error states, does this 
unconditionally return 0 without verifying if the target element actually 
achieved the BLK_SE_STS_REMOVED status?

The caller might be incorrectly informed that the removal succeeded when it 
actually failed. Should the status of the specific element_id be explicitly 
verified for success after the polling wait completes?

> +}

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