linux-kernel - Re: [PATCH v2 1/2] cxl/port: Hold port host lock while dport adding.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6987763fea2ab_125f100f5@dwillia2-mobl4.notmuch>
Date: Sat, 7 Feb 2026 09:28:31 -0800
From: <dan.j.williams@...el.com>
To: Li Ming <ming.li@...omail.com>, <dave@...olabs.net>,
	<jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
	<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
	<ira.weiny@...el.com>, <dan.j.williams@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] cxl/port: Hold port host lock while dport adding.

Li Ming wrote:
> CXL testing environment can trigger following trace
>  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000092: 0000 [#1] SMP KASAN NOPTI
>  KASAN: null-ptr-deref in range [0x0000000000000490-0x0000000000000497]
>  RIP: 0010:cxl_dpa_to_region+0x105/0x1f0 [cxl_core]
>  Call Trace:
>   <TASK>
>   cxl_event_trace_record+0xd1/0xa70 [cxl_core]
>   __cxl_event_trace_record+0x12f/0x1e0 [cxl_core]
>   cxl_mem_get_records_log+0x261/0x500 [cxl_core]
>   cxl_mem_get_event_records+0x7c/0xc0 [cxl_core]
>   cxl_mock_mem_probe+0xd38/0x1c60 [cxl_mock_mem]
>   platform_probe+0x9d/0x130
>   really_probe+0x1c8/0x960
>   __driver_probe_device+0x187/0x3e0
>   driver_probe_device+0x45/0x120
>   __device_attach_driver+0x15d/0x280
> 
> When CXL subsystem adds a cxl port to a hierarchy, there is a small
> window where the new port becomes visible before it is bound to a
> driver. This happens because device_add() adds a device to bus device
> list before bus_probe_device() binds it to a driver.
> So if two cxl memdevs are trying to add a dport to a same port via
> devm_cxl_enumerate_ports(), the second cxl memdev may observe the port
> and attempt to add a dport, but fails because the port has not yet been
> attached to cxl port driver. That causes the memdev->endpoint can not be
> updated.
> 
> The sequence is like:
> 
> CPU 0					CPU 1
> devm_cxl_enumerate_ports()
>   # port not found, add it
>   add_port_attach_ep()
>     # hold the parent port lock
>     # to add the new port
>     devm_cxl_create_port()
>       device_add()
> 	# Add dev to bus devs list
> 	bus_add_device()
> 					devm_cxl_enumerate_ports()
> 					  # found the port
> 					  find_cxl_port_by_uport()
> 					  # hold port lock to add a dport
> 					  device_lock(the port)
> 					  find_or_add_dport()
> 					    cxl_port_add_dport()
> 					      return -ENXIO because port->dev.driver is NULL
> 					  device_unlock(the port)
> 	bus_probe_device()
> 	  # hold the port lock
> 	  # for attaching
> 	  device_lock(the port)
> 	    attaching the new port
> 	  device_unlock(the port)
> 
> To fix this race, require that dport addition holds the host lock
> of the target port(the host of CXL root and all cxl host bridge ports is
> the platform firmware device, the host of all other ports is their
> parent port). The CXL subsystem already requires holding the host lock
> while attaching a new port. Therefore, successfully acquiring the host
> lock guarantees that port attaching has completed.
> 
> Fixes: 4f06d81e7c6a ("cxl: Defer dport allocation for switch ports")
> Signed-off-by: Li Ming <ming.li@...omail.com>
> ---
>  drivers/cxl/core/port.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 5c82e6f32572..6f0d9fe439db 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -612,6 +612,23 @@ struct cxl_port *parent_port_of(struct cxl_port *port)
>  	return port->parent_dport->port;
>  }
>  
> +static struct device *to_port_host(struct cxl_port *port)
> +{
> +	struct cxl_port *parent = parent_port_of(port);
> +
> +	/*
> +	 * The host of CXL root port and the first level of ports is
> +	 * the platform firmware device, the host of all other ports
> +	 * is their parent port.
> +	 */
> +	if (!parent)
> +		return port->uport_dev;

This helper looks good and this case is theoretically correct, but I
assume that find_or_add_dport() never hits this case, right?

How about move the introduction of this helper to its own lead-in patch
and use it to replace the open coded version of this pattern in
unregister_port(), __devm_cxl_add_dport(), and endpoint_host().

I do notice you caught the one in unregister_port(), but there are more.

With that change you can add:

Reviewed-by: Dan Williams <dan.j.williams@...el.com>