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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The BLIST_NO_LUN_1F flag fails to suppress the initial unpopulated LUN (usually LUN 0) from being added as a spurious 'No Device' entry.

Pre-existing issues:
- [High] Concurrent read-modify-write on `struct scsi_target` bitfields causes data races and lost updates.
--

commit 10038c8d1287bfd2d8ba061ac2b0a14d56b4076e
Author: Phil Pemberton <philpem@philpem.me.uk>
scsi: add BLIST_NO_LUN_1F blacklist flag

This commit introduces the BLIST_NO_LUN_1F blacklist flag to suppress spurious
"No Device" entries on certain multi-LUN devices. It modifies scsi_add_lun()
to set the pdt_1f_for_no_lun flag during LUN scan based on the new blacklist
flag.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ef22a4228b855..bfbbf9be05d21 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1069,6 +1069,8 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	transport_configure_device(&sdev->sdev_gendev);
>  
>  	sdev->sdev_bflags = *bflags;
> +	if (sdev->sdev_bflags & BLIST_NO_LUN_1F)
> +		sdev->sdev_target->pdt_1f_for_no_lun = 1;

[Severity: Medium]
Will this actually suppress the spurious entry for the first scanned LUN
(typically LUN 0)?

Looking at scsi_probe_and_add_lun(), the suppression check evaluates
starget->pdt_1f_for_no_lun before scsi_add_lun() is called:

scsi_probe_and_add_lun() {
    ...
    if (((result[0] >> 5) == 1 || starget->pdt_1f_for_no_lun) &&
        ...
        /* skip LUN */
        return SCSI_SCAN_TARGET_PRESENT;
    ...
    scsi_add_lun(...);
}

If LUN 0 is unpopulated, does it bypass the suppression check since the flag
is only set here in scsi_add_lun() afterward?

[Severity: High]
This is a pre-existing issue, but does adding another non-atomic bitfield
modification here expand a race window?

The fields pdt_1f_for_no_lun, single_lun, and expecting_lun_change are
adjacent bitfields in struct scsi_target that share the same memory word.
Modifying a bitfield performs a non-atomic read-modify-write.

Since scsi_add_lun() modifies pdt_1f_for_no_lun and single_lun from process
context (e.g., during a rescan), and an I/O completion on an already-active
LUN encountering a Unit Attention might call scsi_report_lun_change() which
modifies expecting_lun_change from softirq context, can this concurrent
read-modify-write cause updates to be lost?

If expecting_lun_change is lost, does it result in failing commands with
spurious I/O errors instead of retrying them?

If single_lun is lost, could it allow concurrent I/O to devices that do not
support it, potentially causing device hangs or data corruption?

>  
>  	if (scsi_device_is_pseudo_dev(sdev))
>  		return SCSI_SCAN_LUN_PRESENT;

[ ... ]

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