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

[sashiko-bot@kernel.org]

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?

> +
> +		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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001154744.1095902-1-dave.jiang@intel.com?part=2