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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] block: page index vs byte count mismatch in invalidate_inode_pages2_range
- [High] block: heap buffer overflow in disk_wait_for_se_mgmt_completion array reuse
- [High] block: dirty page writeback omission before storage element removal
- [Medium] block: silent error abort in disk_wait_for_se_mgmt_completion causes premature zone revalidation
- [Low] block: blk_storage_element_status typos and duplicate fields in UAPI header
--

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

block: introduce storage element management

This patch introduces storage element management features for managing hard
disk heads. It defines generic block device driver operations for reporting,
removing, and restoring storage elements, allowing file systems to use the
data-preserving flavor of the depopulation feature.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 131c9f50b3da5..b713512f7d48f 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
[ ... ]
> +static void 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, &nr_elements, NULL);
> +	if (ret) {
> +		pr_err("Failed to get number of storage elements\n");
> +		return;
> +	}
> +
> +	elements = kzalloc_objs(struct blk_storage_element, nr_elements);
> +	if (!elements)
> +		return;

[Severity: Medium]
Will this silently abort on failure instead of returning an error or retrying?

If report_elements() or the memory allocation fails, the function simply
returns. This means callers like bdev_remove_storage_element() will
immediately proceed to invoke blk_revalidate_disk_zones() while the hardware
is potentially still actively removing or restoring an element.

> +
> +	while (1) {
> +		/*
> +		 * Check if we have storage elements being removed or restored.
> +		 */
> +		nr_se = nr_elements;
> +		ret = (disk->fops->se_ops->report_elements)(disk,
> +						&nr_elements, elements);

[Severity: High]
Does this code risk a heap buffer overflow if the hardware dynamically
changes the number of reported elements?

The driver's report_elements() callback modifies nr_elements to the actual
number of descriptors returned. In the next iteration, nr_se is set to this
updated count and passed back as the buffer capacity. If the hardware reports
a higher count during polling, nr_elements is treated as a larger capacity
than what was originally allocated via kzalloc_objs().

> +		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;
> +		}

[Severity: High]
Could this read uninitialized or stale memory if the driver returns fewer
elements than nr_se?

The loop iterates up to the pre-call capacity nr_se instead of the actual
returned count nr_elements.

[ ... ]
> +int bdev_remove_storage_element(struct block_device *bdev,
> +				unsigned int element_id)
> +{
[ ... ]
> +	/*
> +	 * Flush the volatile write cache if there is any. Note that this may
> +	 * lead to errors if degraded storage elements are used. So ignore
> +	 * errors here.
> +	 */
> +	blkdev_issue_flush(disk->part0);
> +
> +	/* Invalidate all cached data. */
> +	invalidate_inode_pages2_range(bdev->bd_mapping, 0,
> +				      get_capacity(disk) << SECTOR_SHIFT);

[Severity: High]
Does this miss dirty pages in the block device's page cache?

invalidate_inode_pages2_range() skips dirty pages and returns -EBUSY when it
encounters them. Since sync_blockdev() or filemap_write_and_wait() is not
called before this invalidation, dirty pages remain in memory and will
eventually be written back by background threads to obsolete or offline zones
after the storage element is removed.

[Severity: High]
Does this pass a byte size instead of a page index, leading to incomplete
cache invalidation?

The invalidate_inode_pages2_range() function expects a page index. The
code here passes get_capacity(disk) << SECTOR_SHIFT, which is the capacity
in bytes. On 32-bit platforms where pgoff_t is a 32-bit unsigned integer,
passing a large byte count causes the value to be silently truncated modulo
2^32. For example, a 1TB disk truncates to 0, meaning only page 0 is
invalidated.

> +
> +	ret = (disk->fops->se_ops->remove_element)(disk, element_id);
> +	if (ret)
> +		return ret;
[ ... ]
> +int bdev_restore_storage_elements(struct block_device *bdev)
> +{
[ ... ]
> +	/* Invalidate all cached data. */
> +	invalidate_inode_pages2_range(bdev->bd_mapping, 0,
> +				      get_capacity(disk) << SECTOR_SHIFT);

[Severity: High]
Does this suffer from the same page index vs byte count mismatch as above,
causing truncation on 32-bit systems?

> +
> +	ret = (disk->fops->se_ops->restore_elements)(disk);
> +	if (ret)
> +		return ret;
[ ... ]
> diff --git a/include/uapi/linux/blkzoned.h b/include/uapi/linux/blkzoned.h
> index 6638361209667..3a4bacfe79797 100644
> --- a/include/uapi/linux/blkzoned.h
> +++ b/include/uapi/linux/blkzoned.h
> @@ -208,4 +208,71 @@ struct blk_zone_range {
>  #define BLKFINISHZONE	_IOW(0x12, 136, struct blk_zone_range)
>  #define BLKREPORTZONEV2	_IOWR(0x12, 142, struct blk_zone_report)
>  
> +/**
> + * enum blk_storage_element_status - Statuc of a zoned device storage elements.
> + *
> + * @BLK_SE_RDWR: The storage element handles both reads and writes.
> + * @BLK_SE_READ: The storage element handles reads only.
> + * @BLK_SE_WRITE: The storage element handles writes only.
> + * @BLK_SE_WRITE: The storage element type is unknown.
> + */
> +enum blk_storage_element_type {

[Severity: Low]
This isn't a bug, but there are some typos in this UAPI header.

The documentation has a typo in "Statuc" and names the wrong enum in the
description. Additionally, @BLK_SE_WRITE is duplicated in the docstring
instead of documenting @BLK_SE_TYPE_UNKNOWN.

[ ... ]
> +enum blk_storage_element_status {
> +	BLK_SE_STS_OK			= 0x01,
> +	BLK_SE_STS_DEGRADED		= 0x02,
> +	BLK_SE_STS_REMOVE_IN_PROGRESS	= 0x03,
> +	BLK_SE_STS_REMOVE_ERROR		= 0x04,
> +	BLK_SE_STS_RESTORE_IN_PROGRESS	= 0x05,
> +	BLK_SE_STS_RESTORE_ERROR	= 0x06,
> +	BLK_SE_STS_REMOVED		= 0x07,
> +	BLK_SE_STS_UNKOWN		= 0xFF,
> +};

[Severity: Low]
This isn't a bug, but the UAPI constant for the unknown state is misspelled
as BLK_SE_STS_UNKOWN. Correcting this before the UAPI freezes could prevent
compatibility issues later.

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