Re: [PATCH v3 3/7] block: add storage element management ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] blk: missing partition check in device-wide storage element ioctls
- [High] blk: NULL pointer dereference if se_ops callbacks are not implemented
- [High] blk: TOCTOU race in blkdev_report_storage_elements_ioctl causes out-of-bounds read
- [High] blk: stale page cache due to premature filemap_invalidate_unlock
- [High] block: concurrent direct I/O bypasses inode_lock during storage element management
- [Medium] blk: kernel zone state desync on fatal signal interruption
--

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

block: add storage element management ioctls

This commit introduces the BLKGETNRSTORELEMS, BLKREPORTSTORELEMS,
BLKREMOVESTORELEM, and BLKRESTORESTORELEMS ioctls. These allow user space
to interact with the block layer's storage element management operations
for zoned block devices.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 6a4c6a3f2060b..de22f876b9928 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -2813,6 +2814,73 @@ int bdev_report_storage_elements(struct block_device *bdev,

[ ... ]

> +static int blkdev_report_storage_elements_ioctl(struct block_device *bdev,
> +						void __user *argp)
> +{
> +	struct blk_storage_elements_report rep;
> +	struct blk_storage_element *elements;
> +	unsigned int nr_elements = 0;
> +	unsigned long retc;
> +	int ret;
> +
> +	if (!argp)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&rep, argp,
> +			   sizeof(struct blk_storage_elements_report)))
> +		return -EFAULT;
> +
> +	ret = bdev_report_storage_elements(bdev, NULL, &nr_elements);
> +	if (ret)
> +		return ret;
> +
> +	nr_elements = min(rep.nr_elements, nr_elements);
> +	if (!nr_elements)
> +		return -EINVAL;
> +
> +	elements = kzalloc_objs(struct blk_storage_element, nr_elements);
> +	if (!elements)
> +		return -ENOMEM;
> +
> +	ret = bdev_report_storage_elements(bdev, elements, &nr_elements);
> +	if (ret)
> +		goto free_elements;
> +
> +	retc = copy_to_user(argp + sizeof(struct blk_storage_elements_report),
> +			    elements,
> +			    sizeof(struct blk_storage_element) * nr_elements);

[Severity: High]
Does this code have a TOCTOU race that could cause an out-of-bounds read?

In the second call to bdev_report_storage_elements(), the driver might
update nr_elements to a larger value if the hardware state has changed
and reports more elements.

If nr_elements is increased, the subsequent copy_to_user() will compute
a copy size larger than the allocated elements buffer, potentially
leaking kernel memory to userspace.

> +	if (retc) {
> +		ret = -EFAULT;
> +		goto free_elements;
> +	}

[ ... ]

> +static int blkdev_remove_storage_element_ioctl(struct block_device *bdev,
> +					blk_mode_t mode, void __user *argp)
> +{
> +	unsigned int element_id;
> +	int ret;
> +
> +	if (!(mode & BLK_OPEN_WRITE))
> +		return -EBADF;
> +	if (bdev_read_only(bdev))
> +		return -EPERM;
> +
> +	if (get_user(element_id, (unsigned int __user *)argp))
> +		return -EFAULT;
> +
> +	inode_lock(bdev->bd_mapping->host);

[Severity: High]
Does this incorrectly rely on inode_lock to serialize device access?

The request queue is explicitly unfrozen in bdev_remove_storage_element()
via blk_mq_unfreeze_queue() to allow polling. Concurrently, a thread could
issue an O_DIRECT write to the block device. In blkdev_write_iter(), the
IOCB_DIRECT path bypasses inode_lock and submits the BIO directly to the
unfrozen queue.

This allows hardware reconfiguration commands to execute concurrently with
regular I/O (reads, writes, zone resets), violating device requirements
during depopulation.

> +
> +	/*
> +	 * Flush the device volatile write cache and invalidate all cached data
> +	 * so that reads do not return old data for zones that went offline.
> +	 * Since a user can only write to zoned devices using direct IOs, we
> +	 * should never have any dirty page invalidated and so no data loss as
> +	 * long as the device cache flush succeeds, that is, as long as the
> +	 * flush does not use degraded storage elements.
> +	 */
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (!ret)
> +		ret = truncate_bdev_range(bdev, mode, 0,
> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);
> +	filemap_invalidate_unlock(bdev->bd_mapping);
> +
> +	if (!ret)
> +		ret = bdev_remove_storage_element(bdev, element_id);

[Severity: High]
Can dropping the invalidation lock before the element removal cause the
page cache to become stale?

The invalidation lock is dropped before the actual long-running device
operation (bdev_remove_storage_element) executes.

Because the lock is dropped before the physical device operation completes,
a concurrent buffered read or memory-mapped page fault can fetch the
soon-to-be-removed data from the hardware and cache it. Once the hardware
operation finishes, the page cache will be left containing stale data.

[Severity: Medium]
What happens if the element removal is interrupted by a fatal signal?

The bdev_remove_storage_element() function polls for completion via
disk_wait_for_se_mgmt_completion(). If the polling loop is interrupted by a
fatal signal, it returns -EINTR and bypasses the call to
blk_revalidate_disk_zones(disk).

The hardware operation may still complete asynchronously, leaving the kernel's
internal zone metadata permanently out of sync with the physical device state.

> +
> +	inode_unlock(bdev->bd_mapping->host);
> +
> +	return ret;
> +}

[ ... ]

> +static int blkdev_restore_storage_elements_ioctl(struct block_device *bdev,
> +						 blk_mode_t mode)
> +{
> +	int ret;
> +
> +	if (!(mode & BLK_OPEN_WRITE))
> +		return -EBADF;
> +	if (bdev_read_only(bdev))
> +		return -EPERM;
> +
> +	inode_lock(bdev->bd_mapping->host);
> +
> +	/*
> +	 * Storage element restoration is a destructive operation that will
> +	 * reset all zones. So fflush the device volatile write cache and
> +	 * invalidate all cached data that we may have.
> +	 */
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (!ret)
> +		ret = truncate_bdev_range(bdev, mode, 0,
> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);
> +	filemap_invalidate_unlock(bdev->bd_mapping);
> +
> +	if (!ret)
> +		ret = bdev_restore_storage_elements(bdev);

[Severity: High]
Similarly here, can dropping the invalidation lock before the element
restoration cause the page cache to become stale?

The invalidation lock is dropped before the physical device operation
(bdev_restore_storage_elements) executes, opening a race window for a
concurrent read to cache the soon-to-be-reset data.

> +
> +	inode_unlock(bdev->bd_mapping->host);
> +
> +	return ret;
> +}
> +
> +int blkdev_zone_storage_elements_ioctl(struct block_device *bdev,
> +				       blk_mode_t mode, unsigned int cmd,
> +				       void __user *argp)
> +{
> +	if (!bdev_is_zoned(bdev) || !bdev->bd_disk->fops->se_ops)
> +		return -ENOTTY;

[Severity: Critical]
Is a partition check missing for these device-wide operations?

BLKREMOVESTORELEM and BLKRESTORESTORELEMS execute whole-disk hardware
operations, but there is no bdev_is_partition(bdev) check in the ioctl
routing.

If executed on a partition:
- truncate_bdev_range() only truncates the page cache of that specific
  partition, leaving sibling partitions and the whole disk mapping with
  stale data.
- inode_lock() is taken only on the partition, meaning concurrent ioctls on
  different partitions can bypass serialization.

[Severity: High]
Can this result in a NULL pointer dereference if a driver omits some
callbacks in se_ops?

The functions bdev_report_storage_elements(), bdev_remove_storage_element(),
and bdev_restore_storage_elements() all check that disk->fops->se_ops is
non-NULL. However, they do not verify that the specific function pointers
(report_elements, remove_element, restore_elements) are actually implemented
before dereferencing them.

If a driver provides the se_ops structure but omits an optional callback,
invoking the corresponding ioctl will trigger a NULL pointer dereference.

> +
> +	switch (cmd) {
> +	case BLKGETNRSTORELEMS:
> +		return blkdev_get_nr_storage_elements_ioctl(bdev, argp);

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