Re: [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use

[Dave Jiang <dave.jiang@intel.com>]

On 10/1/26 9:02 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] cxl/core: AB-BA deadlock in RCH topologies due to child-to-parent lock inversion
> --
> 
> commit 689adbac9a3f5c00d58408df886ec90fd00ca46d
> Author: Dave Jiang <dave.jiang@intel.com>
> 
> cxl/core: Hold the dport host lock across dport lookup and use
> 
> Callers of cxl_pci_find_port() and cxl_mem_find_port() can be handed a
> struct cxl_dport that is already freed, and all four dereference it
> without the lock that would prevent that.
>     
> A dport is kzalloc()'d into a devres group on its host device, so it dies
> when that host unbinds. The host is &port->dev for a switch port and
> port->uport_dev for the root port.
> 
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 9d807c1a002ca..55a571f86d477 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -734,10 +734,16 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>  	if (rc && type == CXL_REGLOC_RBI_COMPONENT && is_cxl_restricted(pdev)) {
>>  		struct cxl_dport *dport;
>>  		struct cxl_port *port __free(put_cxl_port) =
>> -			cxl_pci_find_port(pdev, &dport);
>> +			cxl_pci_find_port(pdev);
>>  		if (!port)
>>  			return -EPROBE_DEFER;
>>  
>> +		guard(device)(cxl_port_dport_host(port));
> 
> [Severity: High]
> Does acquiring the ancestor host bridge lock here while the driver core holds
> the descendant (pdev) device lock cause an AB-BA deadlock?
> 
> If concurrent device removal locks the parent device and then iterates to lock
> children, could it deadlock against this path in cxl_pci_setup_regs() during
> device probe?

No. The lock taken is not the host bridge and not an ancestor of pdev.

These paths only run for an RCH, where cxl_pci_find_port() returns the CXL
root port. cxl_port_dport_host() of a root port is root->uport_dev, the
ACPI0017 platform device. The host bridge is only the dport device.

On x86 the PCI root bus is created with a NULL parent, so the RCiEP and its
memdev live under /sys/devices/pci0000:xx, and ACPI0017 lives under the
platform bus. A parent-to-child walk from either tree never reaches the
other.

The order is also not new. cxl_mem_probe() already locks root->uport_dev
for an RCH endpoint, and cxl_mem probes synchronously from cxl_pci_probe().
That establishes:

	pdev -> cxlmd -> ACPI0017

cxl_pci_setup_regs() adds pdev -> ACPI0017, which that chain already
implies.

Nothing takes these locks in the reverse order:

  - delete_endpoint() runs from cxlmd devm teardown: cxlmd -> ACPI0017.
  - unregister_port() of the endpoint takes only the endpoint's own lock.
  - detach_memdev() locks cxlmd from cxl_bus_wq, and nothing waits on that
    work under the ACPI0017 lock.
  - PCI hot-remove goes pdev -> cxlmd -> ACPI0017.

> 
>> +
>> +		dport = cxl_pci_find_dport(pdev, port);
>> +		if (!dport)
>> +			return -EPROBE_DEFER;
>> +
>>  		rc = cxl_rcrb_get_comp_regs(pdev, map, dport);
>>  		if (rc)
>>  			return rc;
> 
> [ ... ]
> 
>> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
>> index e0e01aa5eba6c..3c6aae26deedd 100644
>> --- a/drivers/cxl/core/ras_rch.c
>> +++ b/drivers/cxl/core/ras_rch.c
>> @@ -118,10 +118,20 @@ void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>>  	int severity;
>>  
>>  	struct cxl_port *port __free(put_cxl_port) =
>> -		cxl_pci_find_port(pdev, &dport);
>> +		cxl_pci_find_port(pdev);
>>  	if (!port)
>>  		return;
>>  
>> +	/*
>> +	 * The dport may have been freed since the lookup above, so take the
>> +	 * lock that pins it and find it again.
>> +	 */
>> +	guard(device)(cxl_port_dport_host(port));
> 
> [Severity: High]
> Could this also lead to an AB-BA deadlock on Restricted CXL Host (RCH)
> topologies?
> 
> This acquires the ancestor host bridge lock while the caller holds the
> descendant (&cxlmd->dev) device lock. Concurrent device hot-remove paths
> typically lock from parent to child, which seems like it would deadlock against
> this AER handling path.

Same answer. This is cxlmd -> ACPI0017, the pair cxl_mem_probe() takes in
the same order. Hot-remove takes them in that order too.

> 
>> +
>> +	dport = cxl_pci_find_dport(pdev, port);
>> +	if (!dport)
>> +		return;
>> +
>>  	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>>  		return;
>>
>