Re: [PATCH v3 7/7] scsi: sd_zbc: define storage element management operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: race in bdev_remove_storage_element allows DIO during zone removal
- [Medium] scsi: sd_zbc: runt zone omitted from element->nr_zones calculation
- [Medium] scsi: sd_zbc: data race on sdkp->zone_info in sd_zbc_parse_storage_element()
--

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

scsi: sd_zbc: define storage element management operations

Define the storage element management operations using struct
blk_storage_elements_ops. These operations are valid only on SMR disks
supporting the storage element depopulation feature.

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 1bf5f7ea0d9c5..80ed7a8e09a40 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -548,6 +548,239 @@ static void sd_zbc_print_zones(struct scsi_disk *sdkp)
>  			  sdkp->zone_info.zone_blocks);
>  }
>  
> +static void sd_zbc_parse_storage_element(struct scsi_disk *sdkp, u8 *desc,
> +					 struct blk_storage_element *element)
> +{
> +	struct scsi_device *sdp = sdkp->device;
> +	sector_t zone_sectors = sd_zbc_zone_sectors(sdkp);

[Severity: Medium]
Could this read of sdkp->zone_info.zone_blocks (via sd_zbc_zone_sectors) race
with concurrent disk revalidation?

It appears that sd_zbc_parse_storage_element() accesses zone_blocks without
acquiring disk->zone_revalidate_mutex or freezing the request queue via
blk_queue_enter(). Concurrently, disk revalidation (e.g., via the partition
scan ioctl or uevent rescan) can call sd_zbc_revalidate_zones() which modifies
sdkp->zone_info before acquiring zone_revalidate_mutex:

drivers/scsi/sd_zbc.c:sd_zbc_revalidate_zones() {
    ...
    sdkp->zone_info.zone_blocks = zone_blocks;
    sdkp->zone_info.nr_zones = nr_zones;
    ...
}

Can this lead to torn reads, or an ilog2(0) evaluation if the variable is
zeroed concurrently?

> +	u64 capacity;
> +
> +	memset(element, 0, sizeof(*element));
> +
> +	element->id = get_unaligned_be32(&desc[4]);
> +
> +	switch (desc[14]) {
> +	case SCSI_PHYS_ELEM_TYPE_ALL_ACCESS_STORAGE:
> +		element->type = BLK_SE_TYPE_RDWR;
> +		capacity = get_unaligned_be64(&desc[16]);
> +		if (!zone_sectors || capacity == ULLONG_MAX)
> +			element->nr_zones = 0;
> +		else
> +			element->nr_zones =
> +				logical_to_sectors(sdp, capacity) >>
> +					ilog2(zone_sectors);

[Severity: Medium]
Does this floor division drop the remainder of the capacity, improperly
omitting the runt zone from the element's zone count?

When the driver parses a BLK_SE_TYPE_RDWR storage element descriptor spanning
a capacity that does not divide perfectly by the zone size (e.g., reaching
the end of the disk where a runt zone resides), the bitwise right shift
silently truncates the remainder.

For comparison, sd_zbc_read_zones() calculates the total disk zones using
round_up(sdkp->capacity, zone_blocks) >> ilog2(zone_blocks) to correctly
account for the runt zone.

[ ... ]

> +static int sd_zbc_remove_storage_element(struct gendisk *disk,
> +					 unsigned int element_id)
> +{

[Severity: High]
Is it safe to rely on the caller for synchronization here when the block
layer queue freeze has been dropped?

The caller in the block layer actively drops the queue freeze before executing
the removal:

block/blk-zoned.c:bdev_remove_storage_element() {
    ...
    memflags = blk_mq_freeze_queue(disk->queue);
    blk_mq_unfreeze_queue(disk->queue, memflags);

    ret = disk->fops->se_ops->remove_element(disk, element_id);
    ...
}

And the ioctl only holds inode_lock:

block/blk-zoned.c:blkdev_remove_storage_element_ioctl() {
    ...
    filemap_invalidate_unlock(bdev->bd_mapping);
    if (!ret)
        ret = bdev_remove_storage_element(bdev, element_id);
    inode_unlock(bdev->bd_mapping->host);
    ...
}

Since direct I/O (via blkdev_write_iter) explicitly bypasses inode_lock for
block devices, this seems to allow direct I/O to be issued while the element
is actively being removed. Could this cause torn state between the hardware
and the block layer's in-memory zone write plug tracking?

> +	struct scsi_disk *sdkp = scsi_disk(disk);
> +	struct scsi_device *sdp = sdkp->device;
> +	const int timeout = sdp->request_queue->rq_timeout;
> +	struct scsi_sense_hdr sshdr;

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