Re: [PATCH 1/1] PCI/ASPM: fix link state exit during switch upstream function removal.

[Bjorn Helgaas <helgaas@kernel.org>]

On Sun, Dec 22, 2024 at 07:39:08PM -0800, Daniel Stodden wrote:
> From: Daniel Stodden <daniel.stodden@gmail.com>
> 
> Before change 456d8aa37d0f "Disable ASPM on MFD function removal to
> avoid use-after-free", we would free the ASPM link only after the last
> function on the bus pertaining to the given link was removed.
> 
> That was too late. If function 0 is removed before sibling function,
> link->downstream would point to free'd memory after.
> 
> After above change, we freed the ASPM parent link state upon any
> function removal on the bus pertaining to a given link.
> 
> That is too early. If the link is to a PCIe switch with MFD on the
> upstream port, then removing functions other than 0 first would free a
> link which still remains parent_link to the remaining downstream
> ports.

Is this specific to a Switch?  It seems like removal of any
multi-function device might trip over this.

> The resulting GPFs are especially frequent during hot-unplug, because
> pciehp removes devices on the link bus in reverse order.

Do you have a sample GPF?  If we can include a few pertinent lines
here it may help people connect a problem with this fix.

> On that switch, function 0 is the virtual P2P bridge to the internal
> bus. Free exactly when function 0 is removed -- before the parent link
> is obsolete, but after all subordinate links are gone.

I agree this is a problem.

IIUC we allocate pcie_link_state when enumerating a device on the
upstream end of a link, i.e., a Root Port or Switch Downstream Port,
but we deallocate it when removing a device on the downstream end of
the link, i.e., an Endpoint or Switch Upstream Port.  This asymmetry
seems kind of prone to error.

Also, struct pcie_link_state contains redundant information, e.g., the
pdev, downstream, parent, and sibling members basically duplicate the
hierarchy already described by the struct pci_bus parent, self, and
devices members.  Redundancy like this is also error prone.

This patch is attractive because it's a very small fix, and maybe we
should use it for that reason.  But I do think we're basically
papering over a pretty serious design defect in ASPM.

I think we'd ultimately be better off if we allocated pcie_link_state
either as a member of struct pci_dev (instead of using a pointer), or
perhaps in pci_setup_device() when we set up the rest of the
bridge-related things and made it live as long as the bridge device.

Actually, if we removed all the redundant pointers in struct
pcie_link_state, it would be smaller than a single pointer, so there'd
be no reason to allocate it dynamically.

Of course this would be a much bigger change to aspm.c.

> Fixes: 456d8aa37d0f ("PCI/ASPM: Disable ASPM on MFD function removal to avoid use-after-free")

Do we have any public problem reports we could mention here?  I'm
actually a little surprised that this hasn't been a bigger problem,
given that 456d8aa37d0f appeared in v6.5 in Aug 2023.  But maybe there
is some unusual topology or hot-unplug involved?

> Signed-off-by: Daniel Stodden <dns@arista.com>
> ---
>  drivers/pci/pcie/aspm.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index e0bc90597dca..8ae7c75b408c 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1273,16 +1273,16 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  	parent_link = link->parent;
>  
>  	/*
> -	 * link->downstream is a pointer to the pci_dev of function 0.  If
> -	 * we remove that function, the pci_dev is about to be deallocated,
> -	 * so we can't use link->downstream again.  Free the link state to
> -	 * avoid this.
> +	 * Free the parent link state, no later than function 0 (i.e.
> +	 * link->downstream) being removed.
>  	 *
> -	 * If we're removing a non-0 function, it's possible we could
> -	 * retain the link state, but PCIe r6.0, sec 7.5.3.7, recommends
> -	 * programming the same ASPM Control value for all functions of
> -	 * multi-function devices, so disable ASPM for all of them.
> +	 * Do not free free the link state any earlier. If function 0
> +	 * is a switch upstream port, this link state is parent_link
> +	 * to all subordinate ones.
>  	 */
> +	if (pdev != link->downstream)
> +		goto out;
> +
>  	pcie_config_aspm_link(link, 0);
>  	list_del(&link->sibling);
>  	free_link_state(link);
> @@ -1293,6 +1293,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  		pcie_config_aspm_path(parent_link);
>  	}
>  
> + out:
>  	mutex_unlock(&aspm_lock);
>  	up_read(&pci_bus_sem);
>  }
> -- 
> 2.47.0
>