Re: [PATCH v9 02/11] ata: libata-zpodd: reference SCSI devices in PM callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ata: TOCTOU race in zpodd wake can cause phantom tray ejections
- [Medium] ata: libata-zpodd: NULL pointer dereference in zpodd_wake_dev()
--

commit f9410c07ec8a633f31a2392169e048611d2e22b5
Author: Phil Pemberton <philpem@philpem.me.uk>
Subject: ata: libata-zpodd: reference SCSI devices in PM callbacks

This commit takes a SCSI device reference under ap->lock to prevent concurrent
sysfs deletion from releasing the device before PM callbacks access it.
Missing or deleting devices are skipped, and locks are dropped before
accessing PM state or releasing the reference.

Link: https://lore.kernel.org/linux-ide/20260611030131.5285D1F00893@smtp.kernel.org/

> diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> index 414e7c63bd850..581eb41ea0f3c 100644
> --- a/drivers/ata/libata-zpodd.c
> +++ b/drivers/ata/libata-zpodd.c

[ ... ]

>  static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
>  {
>  	struct ata_device *ata_dev = context;
>  	struct zpodd *zpodd = ata_dev->zpodd;
> -	struct device *dev = &ata_dev->sdev->sdev_gendev;
> +	struct scsi_device *sdev;
>  
> -	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
> +	if (event != ACPI_NOTIFY_DEVICE_WAKE)
> +		return;
> +	sdev = zpodd_get_sdev(ata_dev);
> +	if (!sdev)
> +		return;
> +	if (pm_runtime_suspended(&sdev->sdev_gendev)) {

[Severity: Medium]
Could this lockless check of pm_runtime_suspended() race with a concurrent
runtime PM resume?

A race between an ACPI wake event (e.g. user pressing the eject button) and a
concurrent runtime PM resume can cause the from_notify flag to be set right
after the resume path reads it as false.

If a concurrent resume is already progressing, it can execute
zpodd_post_poweron() and read from_notify as false:

drivers/ata/libata-zpodd.c:zpodd_post_poweron() {
    ...
    if (zpodd->from_notify) {
        zpodd->from_notify = false;
        if (zpodd->mech_type == ODD_MECH_TYPE_DRAWER)
            eject_tray(dev);
    }
    ...
}

Then, zpodd_wake_dev() might set from_notify = true right after, leaving it
persistently true until the next power-on cycle.

This defers the tray ejection to the next resume cycle, potentially causing a
phantom tray ejection. Alternatively, if the PM state just changed from
suspended, the ACPI handler might skip setting the flag entirely, losing the
eject event.

>  		zpodd->from_notify = true;

[Severity: Medium]
Can this dereference a NULL zpodd pointer if an ACPI event fires concurrently?

If a memory allocation failure occurs during the initial zpodd_init() call,
dev->zpodd remains NULL.

If the device later undergoes EH revalidation and kzalloc succeeds,
ata_acpi_add_pm_notifier() is called before dev->zpodd is assigned:

drivers/ata/libata-zpodd.c:zpodd_init() {
    ...
    ata_acpi_add_pm_notifier(dev);
    zpodd->dev = dev;
    dev->zpodd = zpodd;
    ...
}

If an ACPI wake event fires concurrently at this exact moment, zpodd_wake_dev()
will execute asynchronously.

Because the device was previously successfully probed, dev->sdev is valid and
zpodd_get_sdev() returns a valid SCSI device.

If the SCSI device is runtime suspended (which can happen during a hotplug or
PHY event before the SCSI layer fully resumes it), pm_runtime_suspended()
evaluates to true.

This leads to zpodd->from_notify = true executing while zpodd is still NULL,
causing a kernel oops.

> -		pm_runtime_resume(dev);
> +		pm_runtime_resume(&sdev->sdev_gendev);
>  	}
> +	scsi_device_put(sdev);
>  }

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