Re: [PATCH v3 3/8] PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving the D0 power state transition inside __pci_enable_link_state introduces a TOCTOU race leading to a Use-After-Free of the ASPM link pointer.
--

--- Patch [3]: [PATCH v3 3/8] PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states ---
commit abe3f397e26797e78eb9f5d0555a20f019b19168
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states

This commit moves the D0 power state transition into the shared 
__pci_enable_link_state() helper when enabling PCI-PM L1 PM Substates.
This centralizes the transition for all callers and removes the redundant
D0 transitions from the vmd and pcie-qcom drivers.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c04fb71de91c8..6d6862fd2ebb7 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1524,6 +1524,17 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked,
>  		return -EPERM;
>  	}
>  
> +	/*
> +	 * Ensure the device is in D0 before enabling PCI-PM L1 PM Substates, per
> +	 * PCIe r6.0, sec 5.5.4.
> +	 */
> +	if (state & PCIE_LINK_STATE_L1_SS_PCIPM) {
> +		if (locked)
> +			pci_set_power_state_locked(pdev, PCI_D0);
> +		else
> +			pci_set_power_state(pdev, PCI_D0);

[Severity: High]
Does this change introduce a use-after-free if the device is concurrently 
hot-removed?

The link pointer is fetched at the top of __pci_enable_link_state() 
locklessly:

	struct pcie_link_state *link = pcie_aspm_get_link(pdev);

If a caller uses the unlocked APIs, the new code takes the else branch and
calls pci_set_power_state(pdev, PCI_D0). This transition can sleep for tens 
of milliseconds without holding pci_bus_sem.

Could a concurrent hot-remove free the link structure while this thread is
sleeping?

CPU1 (Unlocked API):
__pci_enable_link_state()
    link = pcie_aspm_get_link(pdev);
    // ...
    pci_set_power_state(pdev, PCI_D0); // Sleeps without pci_bus_sem

CPU2 (Hot-remove):
pci_destroy_dev()
    pcie_aspm_exit_link_state()
        // Acquires ASPM locks and frees the `link` structure

CPU1 (wakes up):
    down_read(&pci_bus_sem);
    mutex_lock(&aspm_lock);
    link->aspm_default = pci_calc_aspm_enable_mask(state);

Would CPU1 end up writing to the freed pcie_link_state structure when it 
resumes and modifies link->aspm_default?

> +	}
> +
>  	if (!locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=3