Re: [PATCH v10 13/15] 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:
- [High] scsi: single_lun bitfield data race with expecting_lun_change
--

commit 15aed878e542873ad45c1f172fe5d87a9c1380de
Author: Phil Pemberton <philpem@philpem.me.uk>

scsi: add BLIST_NO_LUN_1F blacklist flag

This commit adds a new BLIST_NO_LUN_1F blacklist flag to prevent SCSI
scanning from adding spurious "No Device" entries for unpopulated LUNs
reporting PQ=0/PDT=0x1f. It also safely moves pdt_1f_for_no_lun to a
dedicated boolean to prevent scan-time writes from overwriting concurrent
I/O updates in shared bitfields.

> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 8694eeadd753e..a81a7c8cce18e 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -360,13 +360,13 @@ struct scsi_target {
>  	unsigned int		single_lun:1;	/* Indicates we should only
>  						 * allow I/O to one of the luns
>  						 * for the device at a time. */
> -	unsigned int		pdt_1f_for_no_lun:1;	/* PDT = 0x1f
> -						 * means no lun present. */
>  	unsigned int		no_report_luns:1;	/* Don't use
>  						 * REPORT LUNS for scanning. */
>  	unsigned int		expecting_lun_change:1;	/* A device has reported
>  						 * a 3F/0E UA, other devices on
>  						 * the same target will also. */

[Severity: High]
Does leaving single_lun in the same bitfield block as expecting_lun_change
still leave us vulnerable to read-modify-write tearing?

While this patch correctly identifies the tearing risk and moves
pdt_1f_for_no_lun to its own bool, single_lun is also updated at scan time
in scsi_add_lun() under shost->scan_mutex:

drivers/scsi/scsi_scan.c:scsi_add_lun() {
    ...
    if (*bflags & BLIST_SINGLELUN)
        scsi_target(sdev)->single_lun = 1;
    ...
}

While LUN 1 (or later) is being scanned, LUN 0 is already visible and can
process I/O. If an I/O on LUN 0 triggers a Unit Attention or completes a
REPORT LUNS command, softirq block completion context updates
expecting_lun_change without holding scan_mutex:

drivers/scsi/scsi_error.c:scsi_report_lun_change() {
    ...
    sdev->sdev_target->expecting_lun_change = 1;
    ...
}

Because both fields share the same memory word, the scan-time update to
single_lun can silently overwrite and erase the concurrent I/O update to
expecting_lun_change, leading to dropped I/O retries, spurious I/O errors,
or loss of single_lun serialization.

This race is explicitly triggered by follow-up patch 15 in this series,
which introduces a quirk setting BLIST_SINGLELUN for a multi-LUN device
(COMPAQ PD-1).

Should single_lun also be moved to a separate bool to completely resolve
this bitfield race?

> +	/* Keep scan-time writes separate from I/O-updated bitfields. */
> +	bool			pdt_1f_for_no_lun; /* PDT = 0x1f means no LUN. */
>  	/* commands actually active on LLD. */
>  	atomic_t		target_busy;
>  	atomic_t		target_blocked;

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