Re: [PATCH v8 4/6] scsi: add BLIST_NO_LUN_1F blacklist flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent modification of `struct scsi_target` bitfields introduces a data race, potentially causing updates to `pdt_1f_for_no_lun` or `expecting_lun_change` to be lost.
--

--- Patch [4]: [PATCH v8 4/6] scsi: add BLIST_NO_LUN_1F blacklist flag ---
commit 90f0f3c59ea5fbec1b6c95c81d334666e21cd3a9
Author: Phil Pemberton <philpem@philpem.me.uk>

scsi: add BLIST_NO_LUN_1F blacklist flag

This commit adds the BLIST_NO_LUN_1F blacklist flag to allow devices to
suppress spurious "No Device" entries for unpopulated LUNs returning
PQ=0/PDT=0x1f. The flag is wired up in scsi_probe_and_add_lun() to set
starget->pdt_1f_for_no_lun.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c

[ ... ]

> @@ -1296,6 +1296,9 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  	 * PDT=00h Direct-access device (floppy)
>  	 * PDT=1Fh none (no FDD connected to the requested logical unit)
>  	 */
> +	if (bflags & BLIST_NO_LUN_1F)
> +		starget->pdt_1f_for_no_lun = 1;
> +
[Severity: High]
This isn't a bug introduced by this patch, but could this bitfield assignment
introduce a data race with other bitfields in struct scsi_target?

Looking at include/scsi/scsi_device.h, pdt_1f_for_no_lun shares the same
memory word with other bitfields:

    unsigned int            single_lun:1;
    unsigned int            pdt_1f_for_no_lun:1;
    unsigned int            no_report_luns:1;
    unsigned int            expecting_lun_change:1;

If a multi-LUN device reports a LUN change (REPORT LUNS DATA HAS CHANGED) on
an active LUN, scsi_report_lun_change() will set expecting_lun_change = 1 in
softirq context.

Simultaneously, if the system handles the LUN change by rescanning the
target, scsi_probe_and_add_lun() executes in process context and sets
pdt_1f_for_no_lun = 1.

Because these contexts perform read-modify-write operations on the same
memory word without mutual exclusion (the softirq does not hold
shost->scan_mutex), one of the updates might be lost.

If expecting_lun_change is lost, the SCSI layer fails to handle a LUN change
UNIT ATTENTION correctly, which on removable devices triggers a spurious
media-change event (cmd->device->changed = 1 and ACTION_FAIL), failing
in-flight I/O. If pdt_1f_for_no_lun is lost, spurious LUNs may be added.

>  	if (((result[0] >> 5) == 1 || starget->pdt_1f_for_no_lun) &&
>  	    (result[0] & 0x1f) == 0x1f &&
>  	    !scsi_is_wlun(lun)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731213503.2379771-1-philpem@philpem.me.uk?part=4