Re: [PATCH v3 6/7] 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 including modify_zones_supported
--

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

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

This commit ensures that zone revalidation is always performed for ZBC disks
that support head depopulation, even if the zone size and number of zones
have not changed. It adds a new bitfield to track the disk's support for
the REMOVE ELEMENT AND MODIFY ZONES command.

> 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;

[Severity: High]
Does adding this bitfield here introduce a data race with other concurrently
accessed bitfields?

C compilers implement bitfield assignments as non-atomic read-modify-write
(RMW) operations on the underlying memory word. This bitfield shares a
memory word with WCE, cache_override, and ignore_medium_access_errors.

>  };
>  #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]
Can this assignment silently overwrite concurrent updates to other bitfields?

This assignment is executed in sd_zbc_read_zones() during disk revalidation.
Concurrently, a sysfs store via cache_type_store() can modify cache_override,
WCE, and RCD before acquiring queue_limits_start_update(), meaning it does
not synchronize with disk revalidation. Additionally, sd_eh_action() can
modify ignore_medium_access_errors from the asynchronous SCSI error handler.

If sd_revalidate_disk() runs concurrently with the sysfs cache updates or
SCSI EH, their non-atomic RMW operations on the shared bitfield word will
race.

Losing the WCE bit update could result in skipped cache flushes and
subsequent data corruption on power loss. Losing the modify_zones_supported
update could cause the block layer to skip necessary zone revalidation on ZBC
disks.

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