Re: [PATCH v19 07/14] PCI: Establish common CXL Port protocol error flow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent devm cleanup and port driver unbinds cause a Use-After-Free of `cxl_dport` and its MMIO registers in the AER handling workqueue.
--

--- Patch [7]: [PATCH v19 07/14] PCI: Establish common CXL Port protocol error flow ---
commit cee1514715e6511f03b640de984db92a0e13bd58
Author: Terry Bowman <terry.bowman@amd.com>

PCI: Establish common CXL Port protocol error flow
    
Establish a single CXL protocol error path shared by both CXL Virtual
Hierarchy (VH) and Restricted CXL Host (RCH) topologies. AER dispatch in
handle_error_source() uses is_cxl_error() and cxl_forward_error() to route
CXL protocol errors through the AER-CXL kfifo to a cxl_core consumer for
logging and recovery.

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index e307361bb39e4..83df544e5a656 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c

[ ... ]

> @@ -335,3 +371,79 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  	return PCI_ERS_RESULT_NEED_RESET;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_error_detected, "CXL");
> +
> +static void cxl_handle_proto_error(struct pci_dev *pdev, struct cxl_port *port,
> +				   struct cxl_dport *dport, int severity)
> +{

[ ... ]

> +}
> +
> +static void __cxl_proto_err_work_fn(struct cxl_proto_err_work_data *wd)
> +{
> +	struct cxl_dport *dport;
> +
> +	/*
> +	 * For RCD devices, handle RCH Downstream Port errors first.
> +	 * cxl_handle_rdport_errors() does its own port lookup and locking,
> +	 * keeping the Downstream Port lock separate from the Endpoint Port
> +	 * lock taken below.
> +	 */
> +	if (is_cxl_restricted(wd->pdev))
> +		cxl_handle_rdport_errors(wd->pdev);
> +
> +	struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_dev(&wd->pdev->dev, NULL);
> +	if (!port) {
> +		dev_err_ratelimited(&wd->pdev->dev,
> +				    "Failed to find parent port device in CXL topology\n");
> +		return;
> +	}
> +
> +	guard(device)(&port->dev);
> +	if (!port->dev.driver) {
> +		dev_err_ratelimited(&port->dev,
> +				    "Port device is unbound, abort error handling\n");
> +		return;
> +	}
> +
> +	dport = cxl_find_dport_by_dev(port, &wd->pdev->dev);

[Severity: Critical]
This is a pre-existing issue, but does checking !port->dev.driver safely guard
against an ACPI unbind racing with the AER handling workqueue?

The commit message states the kfifo consumer "holds the Port device lock and
skips handling when the Port driver is unbound, so RAS register iomaps are
not torn down while the handler reads them."

However, for Root Ports, the cxl_dport and its devres are attached to the
ACPI device (port->uport_dev), not &port->dev.

If devres_release_all(acpi_dev) runs concurrently with the AER workqueue,
it frees dport and unmaps the registers independently of device_lock(&port->dev).

Because the cxl_port driver remains bound until devres_release_all(acpi_dev)
finishes and unregisters the port, checking !port->dev.driver wouldn't protect
against this. Could this lead to a use-after-free of the cxl_dport structure
and reading from unmapped MMIO (RAS/AER registers) in __cxl_proto_err_work_fn()?

> +	if (!dport && (pci_pcie_type(wd->pdev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		       pci_pcie_type(wd->pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> +		dev_err_ratelimited(&wd->pdev->dev,
> +				    "Failed to find dport device in CXL topology\n");
> +		return;
> +	}
> +
> +	cxl_handle_proto_error(wd->pdev, port, dport, wd->severity);
> +}

[ ... ]

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index e0e01aa5eba6c..41959c0333711 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c

[ ... ]

> @@ -110,18 +109,21 @@ static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
>  	return false;
>  }
>  
> -void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
> +void cxl_handle_rdport_errors(struct pci_dev *pdev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct aer_capability_regs aer_regs;
>  	struct cxl_dport *dport;
>  	int severity;
>  
> -	struct cxl_port *port __free(put_cxl_port) =
> -		cxl_pci_find_port(pdev, &dport);
> +	struct cxl_port *port __free(put_cxl_port) = cxl_pci_find_port(pdev, NULL);
>  	if (!port)
>  		return;
>  
> +	guard(device)(&port->dev);
> +	dport = cxl_find_dport_by_dev(port, pdev->dev.parent);
> +	if (!dport)
> +		return;
> +
> +	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))

[Severity: Critical]
This is a pre-existing issue, but does this still suffer from the same race?

The dport is looked up under the &port->dev lock, but for Root Ports its
devm lifecycle is tied to the parent ACPI device. If a concurrent ACPI unbind
occurs, couldn't dport be freed and unmapped before being dereferenced here?

>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803221810.3685703-1-terry.bowman@amd.com?part=7