Re: [PATCH v4 8/9] scsi: sd_zbc: always revalidate zones for disks supporting head depopulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: sd: data race on struct scsi_disk bitfields can clobber WCE or modify_zones_supported
--

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

scsi: sd_zbc: always revalidate zones for disks supporting head depopulation

This commit introduces a new check for ZBC devices that support data preserving
head depopulation to ensure zone conditions are properly revalidated. It adds
a new modify_zones_supported bitfield to struct scsi_disk and sets it during
sd_zbc_read_zones(). The flag is then used to skip or perform zone revalidation
depending on the disk's support for this feature.

> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 574af82430169..6a72371fee786 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -156,6 +156,7 @@ struct scsi_disk {
>  	unsigned	ignore_medium_access_errors : 1;
>  	unsigned	rscs : 1; /* reduced stream control support */
>  	unsigned	use_atomic_write_boundary : 1;
> +	unsigned	modify_zones_supported : 1;
>  };
>  #define to_scsi_disk(obj) container_of(obj, struct scsi_disk, disk_dev)

[ ... ]

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 8cbf171e6fabf..1bf5f7ea0d9c5 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
[ ... ]
> @@ -620,6 +641,9 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, struct queue_limits *lim,
>  	if (ret != 0)
>  		goto err;
>  
> +	/* Check if REMOVE ELEMENT AND MODIFY ZONES is supported. */
> +	sdkp->modify_zones_supported = sd_zbc_check_modify_zones(sdkp, buf);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this bitfield assignment introduce a data race that can silently corrupt
neighboring bitfields like WCE?

Since C compilers implement bitfield assignments as non-atomic
read-modify-write operations on the underlying machine word, updating
modify_zones_supported here in sd_zbc_read_zones() during disk
revalidation might race with sysfs stores.

For example, if a user or udev script writes to
/sys/class/block/*/device/cache_type concurrently with a disk
revalidation event, cache_type_store() in drivers/scsi/sd.c updates
neighboring bits:

drivers/scsi/sd.c:cache_type_store() {
    ...
    sdkp->WCE = wce;
    sdkp->RCD = rcd;
    ...
}

If these operations interleave without holding a lock that synchronizes
the revalidation paths and sysfs writes, one update could silently
overwrite the other. This could potentially disable or enable write
caching unexpectedly or lose the modify_zones_supported flag.

> +
>  	nr_zones = round_up(sdkp->capacity, zone_blocks) >> ilog2(zone_blocks);
>  	if (nr_zones > INT_MAX) {
>  		sd_printk(KERN_ERR, sdkp, "Too many zones (%llu)\n",

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