Re: [PATCH v3 1/8] PCI/ASPM: Always disable ASPM when driver requests it

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition leading to Use-After-Free in ASPM link state management.
--

--- Patch [1]: [PATCH v3 1/8] PCI/ASPM: Always disable ASPM when driver requests it ---
commit 3f3308cae9c38dfb61e4a9d6f6d9f1413c0e0c87
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI/ASPM: Always disable ASPM when driver requests it

This commit makes pci_disable_link_state() unconditionally disable ASPM so
the motivation for drivers to use custom ASPM handling code is eliminated.
This ensures ASPM state changes are properly tracked by the service driver.

Link: https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
Link: https://lore.kernel.org/all/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com/

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1449,16 +1449,23 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
>  {
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  
>  	if (!link)
>  		return -EINVAL;

[ ... ]

>  	if (!locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	link->aspm_disable |= pci_calc_aspm_disable_mask(state);

[Severity: High]
This is a pre-existing issue, but could this lockless fetch of the link
pointer lead to a use-after-free when we write to aspm_disable here?

If one CPU calls this asynchronously (for example, from a network driver TX
timeout handler) and another CPU concurrently removes a sibling PCI function
on the same bridge, could we hit this sequence:

CPU1
__pci_disable_link_state()
    link = pcie_aspm_get_link(pdev);

CPU2
pcie_aspm_exit_link_state()
    // acquires locks, frees parent bridge's link pointer
    bridge->link_state = NULL;
    kfree(link);

CPU1
    mutex_lock(&aspm_lock);
    link->aspm_disable |= pci_calc_aspm_disable_mask(state);

Could this concurrent removal cause the link pointer to be freed before
CPU1 acquires the locks, leading to memory corruption when modified?

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