Re: [PATCH v2 3/7] 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:
- [Critical] block: silent data loss via full truncation without writeback
- [High] block: unkillable task and VFS deadlock in disk_wait_for_se_mgmt_completion()
--

commit 9851d611abe8fdc4d981018f157a08dd381fede2
Author: Damien Le Moal <dlemoal@kernel.org>
block: add storage element management ioctls

This commit introduces four new ioctls (BLKGETNRSTORELEMS, BLKREPORTSTORELEMS,
BLKREMOVESTORELEM, and BLKRESTORESTORELEMS) for managing storage elements on
zoned block devices. It allows querying, removing, and restoring degraded
storage elements from userspace.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index be57644ee2663..6ebb04e0a1593 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c

[ ... ]

> +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;
> +
> +	/*
> +	 * Flush the device volatile write cache and invalidate all cached data
> +	 * so that reads do not return old data for zones that went offline.
> +	 */
> +	inode_lock(bdev->bd_mapping->host);
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = truncate_bdev_range(bdev, mode, 0,
> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);

[Severity: Critical]
Does this truncate the full block device page cache without prior writeback?

The code issues a hardware cache flush via blkdev_issue_flush() but appears
to completely skip writing back dirty pages in the OS page cache (e.g., via
sync_blockdev()). The subsequent truncate_bdev_range() call unconditionally
drops all page cache entries, which could discard dirty data even for zones
that are completely unaffected by the storage element operation.

> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = bdev_remove_storage_element(bdev, element_id);

[Severity: High]
Could this lead to a VFS deadlock and an unkillable task if the device hangs?

VFS locks (inode_lock and filemap_invalidate_lock) are acquired earlier in
this function before calling bdev_remove_storage_element(). Inside that
function, it delegates to the driver and waits for completion. Looking at
disk_wait_for_se_mgmt_completion(), it uses an infinite loop:

    while (1) {
        ...
        /* Not done yet: wait and retry. */
        msleep(500);
    }

If the device takes an extended time to complete or stops responding, the
polling task can become unkillable because it lacks a timeout, max retry
bound, or fatal_signal_pending() check. Because the ioctl caller holds the
block device's i_rwsem and invalidate_lock during this wait, a hung device
could permanently lock out all other processes attempting to access the block
device.

> +
> +out_unlock:
> +	filemap_invalidate_unlock(bdev->bd_mapping);
> +	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;
> +
> +	/*
> +	 * Flush the device volatile write cache and invalidate all cached data
> +	 * so that reads do not return old data for zones that went offline.
> +	 */
> +	inode_lock(bdev->bd_mapping->host);
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = truncate_bdev_range(bdev, mode, 0,
> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);

[Severity: Critical]
Does this also drop dirty pages without prior writeback?

Similar to blkdev_remove_storage_element_ioctl(), this appears to truncate
the entire device's page cache unconditionally after issuing a hardware flush,
which skips writing back dirty pages in the OS page cache first.

> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = bdev_restore_storage_elements(bdev);

[Severity: High]
Does this code also suffer from the same VFS deadlock risk?

Similar to the issue above, bdev_restore_storage_elements() invokes
disk_wait_for_se_mgmt_completion() while holding both inode_lock and
filemap_invalidate_lock, which could permanently stall other processes
accessing the block device if the device becomes unresponsive.

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

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