Re: [PATCH v6 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardcoding LUN 0 in `atapi_qc_complete()` after a failed `ALLOW_MEDIUM_REMOVAL` command re-introduces an infinite SCSI EH loop for higher LUNs.
- [High] The `ATA_DFLAG_RESUMING` flag is permanently leaked if `scsi_rescan_device()` fails, blocking future power management operations.
- [High] `ata_scsi_sdev_destroy()` triggers a full ATA device detach simply when LUN 0 is destroyed, abruptly severing all other active LUNs.

Pre-existing issues:
- [High] In `ata_port_detach()`, `dev->sdev[lun]` is not cleared before calling `scsi_remove_device()`, leaving a window for Use-After-Free by lockless readers.
--

commit 21e5b1c2633a42382bfa70b2a8ecf4892badd416
Author: Phil Pemberton <philpem@philpem.me.uk>
Subject: ata: libata-scsi: convert dev->sdev to per-LUN array

This commit replaces the single scsi_device pointer in ata_device with an
array to support multi-LUN ATAPI devices. It also adds a dev->nr_luns field
and an ata_dev_scsi_device() helper to manage the per-LUN access.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[ ... ]
> @@ -6275,11 +6276,15 @@ static void ata_port_detach(struct ata_port *ap)
>  	/* Remove scsi devices */
>  	ata_for_each_link(link, ap, HOST_FIRST) {
>  		ata_for_each_dev(dev, link, ALL) {
> -			if (dev->sdev) {
> +			int lun;
> +
> +			for (lun = dev->nr_luns - 1; lun >= 0; lun--) {
> +				if (!dev->sdev[lun])
> +					continue;
>  				spin_unlock_irqrestore(ap->lock, flags);
> -				scsi_remove_device(dev->sdev);
> +				scsi_remove_device(dev->sdev[lun]);
>  				spin_lock_irqsave(ap->lock, flags);
> -				dev->sdev = NULL;
> +				dev->sdev[lun] = NULL;
>  			}
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does this leave a window for a
use-after-free? By dropping the lock and calling scsi_remove_device() while
the pointer is still accessible in the array slot, concurrent lockless
readers might access the scsi_device during or after its destruction.
Would it be safer to snapshot the pointer and clear the slot under the lock
before calling scsi_remove_device(), similar to how it is handled in
ata_scsi_remove_dev()?

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -1220,11 +1220,12 @@ void ata_scsi_sdev_destroy(struct scsi_device *sdev)
>  
>  	spin_lock_irqsave(ap->lock, flags);
>  	dev = __ata_scsi_find_dev(ap, sdev);
> -	if (dev && dev->sdev) {
> -		/* SCSI device already in CANCEL state, no need to offline it */
> -		dev->sdev = NULL;
> -		dev->flags |= ATA_DFLAG_DETACH;
> -		ata_port_schedule_eh(ap);
> +	if (dev && dev->sdev[sdev->lun] == sdev) {
> +		dev->sdev[sdev->lun] = NULL;
> +		if (sdev->lun == 0) {
> +			dev->flags |= ATA_DFLAG_DETACH;
> +			ata_port_schedule_eh(ap);
> +		}
>  	}
>  	spin_unlock_irqrestore(ap->lock, flags);

[Severity: High]
Could this abruptly detach the physical device while other LUNs are still
active? If a user manually removes LUN 0 via sysfs while LUN 1 is still
running, the sdev->lun == 0 condition evaluates to true, triggering a
full ATA device teardown. Should the detach sequence instead be deferred
until all active LUN slots in the device array have been cleared?

[ ... ]
> @@ -2911,10 +2912,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  		 * avoid this infinite loop.
>  		 *
>  		 * This may happen before SCSI scan is complete.  Make
> -		 * sure qc->dev->sdev isn't NULL before dereferencing.
> +		 * sure the LUN-0 sdev isn't NULL before dereferencing.
>  		 */
> -		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
> -			qc->dev->sdev->locked = 0;
> +		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL) {
> +			struct scsi_device *sdev =
> +				ata_dev_scsi_device(qc->dev, 0);
> +
> +			if (sdev)
> +				sdev->locked = 0;
> +		}

[Severity: High]
Does hardcoding LUN 0 here re-introduce an infinite SCSI EH loop for higher
LUNs? If an ALLOW_MEDIUM_REMOVAL command fails on a LUN > 0, this code
clears the locked flag on LUN 0 instead of the target device. This leaves
the higher LUN's locked flag set, which causes SCSI EH to retry the lock
indefinitely. Should this use the actual device that the command targeted?

[ ... ]
> @@ -5007,37 +5010,39 @@ void ata_scsi_dev_rescan(struct work_struct *work)
[ ... ]
> -			spin_unlock_irqrestore(ap->lock, flags);
> -			if (do_resume) {
> -				ret = scsi_resume_device(sdev);
> -				if (ret == -EWOULDBLOCK) {
> -					scsi_device_put(sdev);
> -					goto unlock_scan;
> +			for (lun = 0; lun < dev->nr_luns; lun++) {
> +				struct scsi_device *sdev = dev->sdev[lun];
> +
> +				if (!sdev)
> +					continue;
> +				if (scsi_device_get(sdev))
> +					continue;
> +
> +				spin_unlock_irqrestore(ap->lock, flags);
> +				if (do_resume) {
> +					ret = scsi_resume_device(sdev);
> +					if (ret == -EWOULDBLOCK) {
> +						scsi_device_put(sdev);
> +						goto unlock_scan;
> +					}
>  				}
> -				dev->flags &= ~ATA_DFLAG_RESUMING;
> +				ret = scsi_rescan_device(sdev);
> +				scsi_device_put(sdev);
> +				spin_lock_irqsave(ap->lock, flags);
> +
> +				if (ret)
> +					goto unlock_ap;
>  			}
> -			ret = scsi_rescan_device(sdev);
> -			scsi_device_put(sdev);
> -			spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +			if (do_resume)
> +				dev->flags &= ~ATA_DFLAG_RESUMING;
>  		}
>  	}

[Severity: High]
Can the ATA_DFLAG_RESUMING flag be permanently leaked here?
If scsi_rescan_device() returns an error for any LUN, the code breaks out
early using goto unlock_ap. This skips the block at the bottom that clears
the ATA_DFLAG_RESUMING flag, leaving the device stuck in a resuming state
and potentially blocking future power management operations.

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