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

[Richard Cheng <icheng@nvidia.com>]

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