Re: [PATCH v4 4/9] block: add storage element management ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block: concurrent direct I/O races with storage element reconfiguration
- [High] block: Unbounded polling loop holds inode_lock and zone_revalidate_mutex causing Hung Task panics
--

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

block: add storage element management ioctls

Define the new ioctl commands BLKGETNRSTORELEMS, BLKREPORTSTORELEMS,
BLKREMOVESTORELEM, and BLKRESTORESTORELEMS to provide users accessing
zoned block devices directly with an interface to the storage elements
management operations of the block layer.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 8ca5470d65173..10fb3ca5adcd6 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c

[ ... ]

> @@ -2895,6 +2946,54 @@ int bdev_remove_storage_element(struct block_device *bdev,
>  }
>  EXPORT_SYMBOL_GPL(bdev_remove_storage_element);
>  
> +static int blkdev_flush_and_truncate(struct block_device *bdev,
> +				     blk_mode_t mode)
> +{
> +	int ret;
> +
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (!ret)
> +		ret = truncate_bdev_range(bdev, mode, 0,
> +					  bdev_nr_bytes(bdev) - 1);
> +	filemap_invalidate_unlock(bdev->bd_mapping);
> +
> +	return ret;
> +}
> +
> +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);
> +
> +	/*
> +	 * 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.
> +	 */
> +	ret = blkdev_flush_and_truncate(bdev, mode);
> +	if (!ret)
> +		ret = bdev_remove_storage_element(bdev, element_id);

[Severity: High]
Does this actually prevent concurrent direct I/O?

The comment implies that inode_lock() will prevent concurrent direct I/O
during the hardware operation. However, blkdev_write_iter() and
blkdev_read_iter() explicitly bypass inode_lock() for block devices when
IOCB_DIRECT is set.

Since bdev_remove_storage_element() explicitly unfreezes the block queue
to allow the management command to execute, couldn't concurrent O_DIRECT reads
and writes be issued to the device, bypassing the VFS lock and racing with the
destructive hardware reconfiguration?

[Severity: High]
Can holding these locks during the operation cause a Hung Task panic?

bdev_remove_storage_element() acquires zone_revalidate_mutex and then waits
for the hardware operation to complete via disk_wait_for_se_mgmt_completion(),
which contains an unbounded polling loop using msleep(1000).

Because this loop runs while both inode_lock() and zone_revalidate_mutex are
held, any concurrent task attempting to acquire either lock—such as
blk_revalidate_disk_zones() or a buffered I/O operation—will be blocked in
TASK_UNINTERRUPTIBLE. If the hardware reconfiguration takes longer than 120
seconds, won't this trigger the kernel's Hung Task watchdog and panic the
system?

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

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