Re: [PATCH] PCI: Add pcie_get_link_endpoints() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A race condition between `pcie_get_link_endpoints()` and `pci_remove_bus_device()` can lead to a Use-After-Free when accessing `pdev->subordinate`.
- [High] `pcie_get_link_endpoints()` returns success (0) even when it fails to find both ends of the PCIe link, leaving output pointers as NULL.
- [Medium] `pcie_get_link_endpoints()` does not guarantee returning Function 0 for multi-function devices, and resolves Virtual Functions improperly.
--

commit f96df0f9857c01cf8b72af2e93119c328134d9e0
Author: Priyank Rathod <rathodpriyank@google.com>

PCI: Add pcie_get_link_endpoints() helper

In PCIe topologies, physical links are point-to-point connections
between an upstream component and a downstream component. This patch
introduces pcie_get_link_endpoints() in the PCI core to provide a
standardized, symmetric helper to identify both ends of a PCIe link.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..7bdfe7e5ab3a4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -4618,6 +4618,56 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
>  	return rc;
>  }
>  
> +/**
> + * pcie_get_link_endpoints - Identify Upstream and Downstream ends of a PCIe link
> + * @pdev: Any PCIe device on the link (Downstream Port or Endpoint)
> + * @downstream_port: Output pointer to Downstream Port (Upstream Component)
> + * @upstream_port: Output pointer to Upstream Port (Downstream Component)
> + *
> + * Identifies both ends of a point-to-point PCIe link. Increments reference count
> + * on @upstream_port if dynamically discovered on a downstream port. Callers must
> + * release with pcie_put_link_endpoints().
> + *
> + * Return: 0 on success, or -EINVAL if @pdev is NULL or not PCIe.
> + */
> +int pcie_get_link_endpoints(struct pci_dev *pdev,
> +			    struct pci_dev **downstream_port,
> +			    struct pci_dev **upstream_port)
> +{
> +	if (!pdev || !pci_is_pcie(pdev))
> +		return -EINVAL;
> +
> +	if (pcie_downstream_port(pdev)) {
> +		*downstream_port = pdev;
> +		down_read(&pci_bus_sem);
> +		*upstream_port = pdev->subordinate ?
> +			pci_dev_get(list_first_entry_or_null(&pdev->subordinate->devices,
> +							     struct pci_dev, bus_list)) : NULL;

[Severity: High]
Can this result in a use-after-free regression if a concurrent hot-unplug
occurs?

In pci_remove_bus_device(), the subordinate bus is removed by calling
pci_remove_bus(bus). This takes pci_bus_sem for write, removes the bus
from lists, drops the lock, and calls device_unregister(&bus->dev) which
frees the struct pci_bus memory.

Crucially, dev->subordinate is cleared only after pci_remove_bus() returns,
completely outside of pci_bus_sem:

drivers/pci/remove.c:pci_remove_bus_device() {
    ...
    pci_remove_bus(bus);
    dev->subordinate = NULL;
    ...
}

If pcie_get_link_endpoints() acquires pci_bus_sem in this narrow window,
will it observe a dangling pdev->subordinate pointer and cause a
use-after-free read when dereferencing &pdev->subordinate->devices?

[Severity: Medium]
Will this grab the wrong function if function 0 of a multi-function device
has been removed?

The commit message mentions standardizing aspm logic, which explicitly
resolves subordinate function 0. By using list_first_entry_or_null() here,
if function 0 is hot-removed, function 1 or later becomes the first entry.

Since the PCIe base specification requires link capabilities and control
registers to be implemented in function 0 (while others may hardwire them
to 0), could returning a non-zero function cause feature drivers to fail
when configuring the link?

> +		up_read(&pci_bus_sem);
> +	} else {
> +		*downstream_port = pci_upstream_bridge(pdev);
> +		*upstream_port = pdev;

[Severity: Medium]
Does this improperly set the link endpoint if pdev is a virtual function?

If this is called on a virtual function, it returns the device directly
without resolving its physical function. Like non-zero functions, virtual
functions may hardwire link capabilities to 0, which could break drivers
relying on this helper to read valid link states.

> +	}
> +
> +	return 0;

[Severity: High]
Will callers encounter a NULL pointer dereference regression when this
returns success but fails to resolve the endpoints?

If this helper is called on an empty downstream port,
list_first_entry_or_null() returns NULL, leaving *upstream_port = NULL.

Similarly, if called on a root complex integrated endpoint,
pci_upstream_bridge(pdev) returns NULL, leaving *downstream_port = NULL.

In both scenarios, the function unconditionally returns 0 (success). Since
callers expect valid pointers on success, won't they blindly dereference
these NULL pointers? Should this return an error code like -ENODEV if
either endpoint cannot be resolved?

> +}
> +EXPORT_SYMBOL_GPL(pcie_get_link_endpoints);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-pcie-link-endpoints-v1-1-32c2fd893e9e@google.com?part=1