Re: [PATCH v18 10/13] cxl: Add port and dport identifiers to CXL AER trace events

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asynchronous CPER error handling races with ACPI driver unbind, causing Use-After-Free of cxl_dport.
- [High] Live PCI configuration space reads on a potentially frozen or dead link during AER recovery.
--

--- Patch [10]: [PATCH v18 10/13] cxl: Add port and dport identifiers to CXL AER trace events ---
commit 069c60a58d08507582ec548c08666c5374faa22d
Author: Dan Williams <[email protected]>
Subject: cxl: Add port and dport identifiers to CXL AER trace events

This commit updates the CXL AER trace events to include port and dport
identifiers instead of deriving a plain device pointer at the caller. It
replaces the separate uncorrectable and correctable trace events with
unified cxl_aer_* events. CPER handling is also reworked to use
find_cxl_port_by_dev() and the unified trace helpers.

> The serial number is retrieved via pci_get_dsn() which performs live
> PCI configuration space reads. A following patch ("PCI: Cache PCI
> DSN into pci_dev->dsn during probe") replaces these with a cached
> serial number to avoid config space access in error handlers and panic
> paths.

[Severity: High]
Does calling pci_get_dsn() before the follow-up patch is applied risk
causing a lockup?

cxl_pci_error_detected() is called during AER recovery, where the link may
be in the pci_channel_io_frozen state. pci_get_dsn() executes live
configuration reads (pci_read_config_dword()) to traverse PCIe extended
capabilities.

Accessing a frozen link can cause the host bridge to lock up or trigger a
secondary master abort. Will this cause completion timeouts and prevent
recovery during the window between this patch and the subsequent caching
patch?

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index d5dc2c22565da..acf40b2396c3b 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
[ ... ]
> @@ -109,47 +77,34 @@ static struct cxl_port *find_cxl_port_by_dev(struct device *dev, struct cxl_dpor
>  
>  void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
>  {
[ ... ]
> +	guard(device)(&port->dev);
> +
> +	/* dport is NULL for Endpoint and Upstream Port devices */
> +	dport = cxl_find_dport_by_dev(port, &pdev->dev);

[Severity: High]
Is it possible for the dport to be freed concurrently here, leading to a
use-after-free regression?

While cxl_cper_handle_prot_err() holds the port's device lock, cxl_dport
structures for root ports are tied to the parent ACPI device's devres
lifecycle.

If the parent driver unbinds (e.g., during ACPI hot-remove), it executes
devres_release_all() under the parent's lock, which frees the dport
directly via cxl_dport_remove() and free_dport().

Could cxl_find_dport_by_dev() load this freed pointer, which is then
dereferenced in the trace event?

drivers/cxl/core/trace.c:cxl_trace_dport_name() {
    if (dport)
        return dev_name(dport->dport_dev);
    ...
}

> -	cxlmd = to_cxl_memdev(mem_dev);
>  	if (data->severity == AER_CORRECTABLE)
> -		cxl_cper_trace_corr_prot_err(cxlmd, data->ras_cap);
> +		cxl_cper_trace_corr_prot_err(port, dport, pci_get_dsn(pdev),
> +					     &data->ras_cap);
>  	else
> -		cxl_cper_trace_uncorr_prot_err(cxlmd, data->ras_cap);
> +		cxl_cper_trace_uncorr_prot_err(port, dport, pci_get_dsn(pdev),
> +					       &data->ras_cap);
>  }
[ ... ]
> @@ -360,7 +306,8 @@ pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
>  		 * cases below handle AER recovery for devices without active
>  		 * CXL.mem traffic.
>  		 */
> -		ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
> +		ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL),
> +				    pci_get_dsn(pdev));
>  	}

[Severity: High]
Does this live config read trigger the lockups mentioned in the commit
message? Since cxl_pci_error_detected() executes when the link state is
potentially pci_channel_io_frozen, the pci_get_dsn() call will traverse
PCIe extended capabilities on a dead link.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10