Re: [PATCH] scsi: core: Make SCSI host scan interruptible

[sashiko-bot@kernel.org]

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

New issues:
- [High] Non-fatal signals and TIF_NOTIFY_SIGNAL silently abort SCSI scans due to incorrect use of signal_pending() and mutex_lock_interruptible().

Pre-existing issues:
- [High] Transport-backed sysfs scans still sleep in TASK_UNINTERRUPTIBLE on shost->scan_mutex.
--

commit 78930d642b0da5b5ffc8a1e28c1f7cf1e6e1e5b5
Author: Bart Van Assche <bvanassche@acm.org>

scsi: core: Make SCSI host scan interruptible

This commit modifies SCSI host scanning to use interruptible locking and
check for pending signals during scans. It aims to prevent hung task
warnings when tasks waiting on shost->scan_mutex receive fatal signals
like SIGKILL.

Link: https://syzkaller.appspot.com/bug?extid=1f678bd2872aefb11d8a

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 4a4f11d2c7798..3161aa1bc8dcd 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c

[ ... ]

> @@ -1591,6 +1595,9 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  	 * the header, so start at 1 and go up to and including num_luns.
>  	 */
>  	for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
> +		if (signal_pending(current))
> +			break;
> +

[Severity: High]
Can this silently abort LUN scanning on non-fatal signals?

If a non-fatal signal (like SIGCHLD) or TIF_NOTIFY_SIGNAL (used by
io_uring or task work) is delivered to the thread performing the scan,
signal_pending() returns true. This causes the loop in
scsi_report_lun_scan() to break and return success, leaving devices
undiscovered without reporting an error.

>  		lun = scsilun_to_int(lunp);
>  
>  		if (lun > sdev->host->max_lun) {

[ ... ]

> @@ -1913,23 +1926,30 @@ int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
>  	    ((lun != SCAN_WILD_CARD) && (lun >= shost->max_lun)))
>  		return -EINVAL;
>  
> -	mutex_lock(&shost->scan_mutex);
> +	ret = mutex_lock_interruptible(&shost->scan_mutex);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Are we risking silent discovery failures by using mutex_lock_interruptible()?

Since mutex_lock_interruptible() returns -EINTR on non-fatal signals,
it prematurely fails the scan lock acquisition.

Furthermore, do_scsi_scan_host() silently discards the -EINTR return value:

drivers/scsi/scsi_scan.c:do_scsi_scan_host() {
    ...
    scsi_scan_host_selected(shost, SCAN_WILD_CARD, SCAN_WILD_CARD,
                            SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
    ...
}

Will this cause module loads to incorrectly succeed without discovering
attached devices if a non-fatal signal is pending during initialization?


[Severity: High]
This is a pre-existing issue, but transport-backed sysfs scans still seem
vulnerable to the original hung task issue this patch addresses.

Concurrent sysfs scans initiated by transport layers (FC, iSCSI, SAS) bypass
scsi_scan_host_selected() and block uninterruptibly in scsi_scan_target()
and __scsi_add_device():

drivers/scsi/scsi_scan.c:scsi_scan_target() {
    ...
    mutex_lock(&shost->scan_mutex);
    ...
}

Will tasks initiating these scans still exceed the hung task timeout if
fatal signals like SIGKILL are delivered while they wait on scan_mutex?

>  	if (!shost->async_scan)
>  		scsi_complete_async_scans();
>  
> -	if (scsi_host_scan_allowed(shost) && scsi_autopm_get_host(shost) == 0) {
> +	if (!signal_pending(current) && scsi_host_scan_allowed(shost) &&

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/74786683eadc0a32ab335373ead8dd76a9d5a342.1789157339.git.bvanassche@acm.org?part=1