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

["Bowman, Terry" <terry.bowman@amd.com>]

On 7/17/2026 5:53 PM, sashiko-bot@kernel.org wrote:
> 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.
This is a false positive.

The Downstream port lifecycle is tied to the Port's xarray and synchronized here using the 
Port's device lock.

> - [High] Live PCI configuration space reads on a potentially frozen or dead link during AER recovery.
This is fixed in patch 11 with introduction of cached PCI DSN.

-Terry

> --
> 
> --- Patch [10]: [PATCH v18 10/13] cxl: Add port and dport identifiers to CXL AER trace events ---
> commit 069c60a58d08507582ec548c08666c5374faa22d
> Author: Dan Williams <djbw@kernel.org>
> 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.
>