Re: [PATCH v18 07/13] PCI/CXL: Add RCH support to CXL handlers

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

On 7/23/2026 12:35 AM, Richard Cheng wrote:
> On Mon, Jul 20, 2026 at 10:06:34AM +0800, Bowman, Terry wrote:
>> On 7/17/2026 5:43 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>>>
>>> New issues:
>>> - [High] Removal of explicit PCI error handler forwarding for RCH devices leaves RCDs bound after a fatal PCI error.
>> This is a false positive. device_release_driver() is called in cxl_pci_error_detected() 
>>
>>>
>>> Pre-existing issues:
>>> - [High] Preexisting lockless dereference of `dport` returned by `cxl_pci_find_port()` and `cxl_mem_find_port()` leading to Use-After-Free.
>> This is existing issue and is not planned to be fixed in this series.
>>
>>> --
>>>
>>> --- Patch [7]: [PATCH v18 07/13] PCI/CXL: Add RCH support to CXL handlers ---
>>> commit 6a99c70b18865d01d350aa367342e701ebe6dde2
>>> Author: Terry Bowman <terry.bowman@amd.com>
>>>
>>> PCI/CXL: Add RCH support to CXL handlers
>>>
>>> Restricted CXL Host (RCH) error handling is folded into the new CXL Port
>>> error handling flow. RCH protocol errors are forwarded through the AER-CXL
>>> kfifo, and the manual CXL.mem error handler invocation is removed.
>>>
>>>> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
>>>> --- a/drivers/cxl/core/ras_rch.c
>>>> +++ b/drivers/cxl/core/ras_rch.c
>>>> @@ -96,18 +95,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);
>>>
>>> [Severity: High]
>>> This is a pre-existing issue, but while this change correctly closes a
>>> lockless Use-After-Free window for dport here, does the same vulnerability
>>> still exist in cxl_pci_setup_regs() and rcd_pcie_cap_emit()?
>>>
>>> In cxl_pci_setup_regs(), cxl_rcrb_get_comp_regs() dereferences the dport
>>> returned by cxl_pci_find_port() before any lock is taken.
>>>
>>> Similarly, in rcd_pcie_cap_emit(), dport->regs.rcd_pcie_cap is dereferenced
>>> entirely locklessly, and then the function takes guard(device)(root_dev)
>>> instead of the required &root->dev lock.
>>>
>>> Since rcd_pcie_cap_emit() is reachable via sysfs attributes, could an
>>> unprivileged local user trigger a Use-After-Free by reading sysfs during a
>>> concurrent device hot-unplug?
>>>
>>> [ ... ]
>>>
>>
>> This is existing issue and is not planned to be fixed in this series.
>>
>>>> diff --git a/drivers/pci/pcie/aer_cxl_rch.c b/drivers/pci/pcie/aer_cxl_rch.c
>>>> --- a/drivers/pci/pcie/aer_cxl_rch.c
>>>> +++ b/drivers/pci/pcie/aer_cxl_rch.c
>>>> @@ -34,42 +34,37 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>>>  	return (pcie_ports_native || host->native_aer);
>>>>  }
>>>>  
>>>> +struct cxl_rch_error_ctx {
>>>> +	struct aer_err_info *info;
>>>> +	bool enqueued;
>>>> +};
>>>> +
>>>>  static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>>>>  {
>>>> -	struct aer_err_info *info = (struct aer_err_info *)data;
>>>> -	const struct pci_error_handlers *err_handler;
>>>> +	struct cxl_rch_error_ctx *ctx = data;
>>>>  
>>>>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>>>>  		return 0;
>>>>  
>>>> -	guard(device)(&dev->dev);
>>>> -
>>>> -	err_handler = dev->driver ? dev->driver->err_handler : NULL;
>>>> -	if (!err_handler)
>>>> -		return 0;
>>>> -
>>>> -	if (info->severity == AER_CORRECTABLE) {
>>>> -		if (err_handler->cor_error_detected)
>>>> -			err_handler->cor_error_detected(dev);
>>>> -	} else if (err_handler->error_detected) {
>>>> -		if (info->severity == AER_NONFATAL)
>>>> -			err_handler->error_detected(dev, pci_channel_io_normal);
>>>> -		else if (info->severity == AER_FATAL)
>>>> -			err_handler->error_detected(dev, pci_channel_io_frozen);
>>>> -	}
>>>> +	if (cxl_forward_error(dev, ctx->info))
>>>> +		ctx->enqueued = true;
>>>
>>> [Severity: High]
>>> Does removing the explicit invocation of the err_handler leave RCDs bound
>>> after a fatal PCI error?
>>>
>>> The standard pcie_do_recovery() on an RCEC does not iterate over its
>>> associated RC_END (RCD) devices, so this manual invocation used to handle the
>>> CXL.mem driver fallback. Now that errors are routed solely through the CXL
>>> protocol error work queue (__cxl_proto_err_work_fn()), the work queue only
>>> panics on uncorrectable cachemem errors.
>>>
>>> Does the work queue replicate the PCI recovery state machine's fallback
>>> behavior, such as calling device_release_driver() to unbind the CXL.mem
>>> driver on frozen channel states?
>>>
>>>>  	return 0;
>>>>  }
>>>
>>
>> The change to forward CXL errors through Kfifo using schedulable work is 
>> intentional. This series does not exactly *replicate* the PCI handling.
>>
>> The device_release_driver() is called in cxl_pci_error_detected() for 
>> pci_channel_io_frozen. cxl_pci_error_detected() serves as the PCIe error 
>> handler for Endpoints, including RCDs.
>>
>> Terry
>>
> 
> Hi Terry,
> 
> I agree with you on this part, but what I can't find after this patch is the
> path that invokees it for an associated RCD.
> 
> you replace all error_detected() for associated RCD with cxl_forward_error()
> and queue CXL RAS work, what if a fatal case where it finds no RAS UCE?
> 
> It'll return normal and RCD driver will remain bound won't it ?
> 
> --Richard
>  

Hi Richard,

Rereading your question, let me give a fuller explanation of the Endpoint UCE
paths.

CXL Endpoint UCEs (both VH and RCD) are handled by the AER handlers, including
pcie_do_recovery() calling cxl_pci_error_detected(), for both severities - so
the Endpoint is not left bound. The two severities take different
call paths:

UCE non-fatal
=============
The AER status is readable, so the event is classified as CXL and forwarded to
the kfifo. Below is the UCE fatal AER path:

handle_error_source()
   cxl_forward_error()            [enqueue kfifo]
   cxl_proto_err_wait_for_empty() [drain]
      cxl_do_recovery() -> cxl_handle_ras()
   pci_aer_handle_error()
      pcie_do_recovery(io_normal)
         cxl_pci_error_detected() -> cxl_handle_ras()
            return CAN_RECOVER        [normal -> recoverable, driver stays bound]

UCE fatal
=========
The uplink is unstable, so the AER core cannot read the source's AER status.
is_cxl_error() cannot classify the event as CXL, so it is handled as a PCIe
error (not forwarded to the kfifo). It still reaches cxl_pci_error_detected()
via pcie_do_recovery(), which releases the CXL.mem driver on the frozen
channel. Below is the UCE non-fatal path:

handle_error_source()
   pci_aer_handle_error()
      pcie_do_recovery(io_frozen)
         cxl_pci_error_detected() -> cxl_handle_ras()
            device_release_driver()   [frozen -> unbind CXL.mem]

So the Endpoint is not left bound after a fatal (frozen) error.

The non-fatal case does call the RAS handler twice (kfifo + AER path), but this
is benign since the CXL RAS status is W1C.

Thanks for the careful review.

Terry