cxl: dport fixes from sashiko reports

Switch decoders cache dport pointers in cxlsd->target[], and nothing clears them when a dport is freed. Readers then dereference freed memory.

KASAN caught it under a cxl_test load/unload loop with concurrent sysfs reads (abbreviated).

Clear the matching slots from cxl_dport_remove(), walking the port's decoders the way cxl_port_update_decoder_targets() does on the add side. Scan all nr_targets slots, the target[] allocation size, and clear every hit.

Fixes 8330671c57c7 ("cxl: Add helper to delete dport")

Cc [email protected]

Signed-off-by Dave Jiang <[email protected]>

Assisted-by LLM

Reviewed-by Jonathan Cameron <[email protected]>

---
Found by KASAN while stress-testing a separate dport locking fix, not from a failure report.

Reproducer: cxl_test, CONFIG_KASAN=y, 12 concurrent readers cat'ing /sys/bus/cxl/devices/*/target_list while cxl_test is unloaded and reloaded.

With the fix reverted the UAF lands ~1s into the first unload, on the trace above. With it applied, 36 cycles across two runs are clean: no KASAN report, no WARNING, lockdep never self-disables. clear_decoder_target() fired 2916 times during those runs, so the new path ran under the load.

KASAN only reports once per boot unless kasan_multi_shot is set (report_enabled(), mm/kasan/report.c), so the reverted-fix run cannot show a per-cycle count - one report is the ceiling, not the frequency.

target_list output is unaffected. Clearing a slot truncates the list at the first NULL, so that was the regression to watch for: 88 target_list files, no empty values before or after, and the multiset of values is unchanged once the ida-assigned decoder names are stripped.

Enumeration is unchanged too: 15 memdev, 12 port, 15 endpoint, 192 decoder.

---
  drivers/cxl/core/port.c | 29 +++++++++++++++++++++++++++++
  1 file changed, 29 insertions(+)

drivers/cxl/core/port.c +29 -0

index 625e4aa427db..6ff3353865e3 100644
@@ -1089,11 +1089,40 @@ static void cond_cxl_root_unlock(struct cxl_port *port)
  		device_unlock(&port->dev);
  }
 
+static int clear_decoder_target(struct device *dev, void *data)
+{
+	struct cxl_dport *dport = data;
+	struct cxl_switch_decoder *cxlsd;
+
+	if (!is_switch_decoder(dev))
+		return 0;
+
+	cxlsd = to_cxl_switch_decoder(dev);
+	guard(rwsem_write)(&cxl_rwsem.region);
+
+	/*
+	 * A dport can occupy more than one position of an interleave, so
+	 * scan the whole target list rather than stopping at the first hit.
+	 */
+	for (int i = 0; i < cxlsd->nr_targets; i++) {
+		if (cxlsd->target[i] != dport)
+			continue;
+		cxlsd->target[i] = NULL;
+		dev_dbg(dev, "dport%d removed from target list, index %d\n",
+			dport->port_id, i);
+	}
+
+	return 0;
+}
+
  static void cxl_dport_remove(void *data)
  {
  	struct cxl_dport *dport = data;
  	struct cxl_port *port = dport->port;
 
+	/* counterpart of cxl_port_update_decoder_targets() */
+	device_for_each_child(&port->dev, dport, clear_decoder_target);
+
  	port->nr_dports--;
  	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
  	put_device(dport->dport_dev);

Switch decoders cache dport pointers in cxlsd->target[], and nothing clears them when a dport is freed. Readers then dereference freed memory.

KASAN caught it under a cxl_test load/unload loop with concurrent sysfs reads (abbreviated).

Clear the matching slots from cxl_dport_remove(), walking the port's decoders the way cxl_port_update_decoder_targets() does on the add side. Scan all nr_targets slots, the target[] allocation size, and clear every hit.

Fixes 8330671c57c7 ("cxl: Add helper to delete dport")

Cc [email protected]

Signed-off-by Dave Jiang <[email protected]>

Assisted-by LLM

Reviewed-by Jonathan Cameron <[email protected]>

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.

Both helpers return the dport through an output parameter. The caller's port reference pins struct cxl_port, not the dport allocation, and the lookup itself is a bare xa_load() under no lock.

Add cxl_port_dport_host() to name the device that pins a dport, and cxl_pci_find_dport() / cxl_mem_find_dport() to look one up with that device's lock held and asserted. Convert the four callers to use them and bail if the dport is gone. cxl_port_add_dport() already follows this rule.

cxl_mem_probe() chose its endpoint devm host from dport->rch, which meant dereferencing the dport before taking any lock. Use cxl_port_dport_host():
rch dports only ever land on the root port, so it selects the same device without needing the dport.

Fixes 733b57f262b0 ("cxl/pci: Early setup RCH dport component registers from RCRB")

Signed-off-by Dave Jiang <[email protected]>

Assisted-by LLM

---
v2:
- Reword the cxl_port_dport_host() comment. (Jonathan)
- Drop the note on stale cxlsd->target[] pointers, fixed by patch 1. (Jonathan)

Found by review, not from a failure report.

Untested: two of the four converted sites add a new lock acquisition, and neither executed under test.

cxl_pci_setup_regs() new guard() sits behind is_cxl_restricted(),
which needs a PCI_EXP_TYPE_RC_END device
cxl_handle_rdport_errors() needs an RCH dport AER error; cxl_test has no
injection path

Only cxl_pci_setup_regs() introduces a lock order lockdep has not already
seen: device_lock(&pdev->dev) from probe, then the dport host lock.

cxl_handle_rdport_errors() nests its new host lock inside the device_lock(&cxlmd->dev) that core/ras.c:274 and :298 hold, but cxl_mem_probe() takes that same pair in that same order and ran clean under PROVE_LOCKING, so the edge itself is covered. What is untested there is the path, not the ordering.

Reaching either needs a cxl-type3 attached directly to a host bridge so pci_pcie_type() is PCI_EXP_TYPE_RC_END, which run_qemu.sh does not generate.

Tested in QEMU with cxl_test, PROVE_LOCKING and KASAN on, 8 module load/unload cycles with concurrent sysfs readers. No lockdep splat, no device_lock_assert() firing, enumeration unchanged (15 memdev, 12 port, 192 decoder before and after).

cxl_mem_find_dport() ran for 15 memdevs on each of 9 loads, covering both cxl_port_dport_host() branches. The rch branch is covered too: "cxl_mem
mem2: endpoint9 added to root3" shows the new selection picking the same parent dport->rch did.

---
  drivers/cxl/core/core.h    |  6 +-----
  drivers/cxl/core/pci.c     |  8 +++++++-
  drivers/cxl/core/port.c    | 36 ++++++++++++++++++++++++++++++++++++
  drivers/cxl/core/ras_rch.c | 12 +++++++++++-
  drivers/cxl/cxl.h          | 16 ++++++++++++++++
  drivers/cxl/mem.c          | 14 +++++++++-----
  drivers/cxl/pci.c          | 11 ++++++-----
  7 files changed, 86 insertions(+), 17 deletions(-)

drivers/cxl/core/core.h +1 -5

index 35eaf636adc9..0a39e500029a 100644
@@ -178,11 +178,7 @@ static inline struct device *port_to_host(struct cxl_port *port)
 
  static inline struct device *dport_to_host(struct cxl_dport *dport)
  {
-	struct cxl_port *port = dport->port;
-
-	if (is_cxl_root(port))
-		return port->uport_dev;
-	return &port->dev;
+	return cxl_port_dport_host(dport->port);
  }
  #ifdef CONFIG_CXL_RAS
  void cxl_ras_init(void);

drivers/cxl/core/pci.c +7 -1

index 9d807c1a002c..8a236e22bb3f 100644
@@ -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, NULL);
  		if (!port)
  			return -EPROBE_DEFER;
 
+		guard(device)(cxl_port_dport_host(port));
+
+		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;

drivers/cxl/core/port.c +36 -0

index 6ff3353865e3..336d7c0f5d98 100644
@@ -1428,6 +1428,10 @@ static struct cxl_port *__find_cxl_port_by_dport(struct cxl_find_port_ctx *ctx)
   *
   * Return a 'struct cxl_port' with an elevated reference if found. Use
   * __free(put_cxl_port) to release.
+ *
+ * The port reference does not pin @dport, which is a devm allocation of
+ * cxl_port_dport_host(). Pass NULL and use cxl_pci_find_dport() or
+ * cxl_mem_find_dport() under that device's lock instead.
   */
  static struct cxl_port *find_cxl_port_by_dport(struct device *dport_dev,
  					       struct cxl_dport **dport)

@@ -1943,6 +1947,38 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
  }
  EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, "CXL");
 
+/**
+ * cxl_pci_find_dport - find the dport of @port that @pdev is below
+ * @pdev: PCI device below the dport
+ * @port: port to search, typically from cxl_pci_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *	    must not use the result after dropping it.
+ */
+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+				     struct cxl_port *port)
+{
+	device_lock_assert(cxl_port_dport_host(port));
+	return cxl_find_dport_by_dev(port, pdev->dev.parent);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_pci_find_dport, "CXL");
+
+/**
+ * cxl_mem_find_dport - find the dport of @port that @cxlmd is below
+ * @cxlmd: memdev below the dport
+ * @port: port to search, typically from cxl_mem_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *	    must not use the result after dropping it.
+ */
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+				     struct cxl_port *port)
+{
+	device_lock_assert(cxl_port_dport_host(port));
+	return cxl_find_dport_by_dev(port, grandparent(&cxlmd->dev));
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_find_dport, "CXL");
+
  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
  				    struct cxl_port *port)
  {

drivers/cxl/core/ras_rch.c +11 -1

index e0e01aa5eba6..827a2f64d892 100644
@@ -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, NULL);
  	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));
+
+	dport = cxl_pci_find_dport(pdev, port);
+	if (!dport)
+		return;
+
  	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
  		return;
 

drivers/cxl/cxl.h +16 -0

index cab8ce39f465..f08de094f412 100644
@@ -701,6 +701,18 @@ static inline bool is_cxl_root(struct cxl_port *port)
  	return port->uport_dev == port->dev.parent;
  }
 
+/*
+ * Return the owner device of the devm allocations of @port's dports. They
+ * are freed when that device unbinds, so hold device_lock() on the returned
+ * device across any lookup and use of a 'struct cxl_dport'.
+ */
+static inline struct device *cxl_port_dport_host(struct cxl_port *port)
+{
+	if (is_cxl_root(port))
+		return port->uport_dev;
+	return &port->dev;
+}
+
  /* Address translation functions exported to cxl_translate test module only */
  int cxl_validate_translation_params(u8 eiw, u16 eig, int pos);
  u64 cxl_calculate_hpa_offset(u64 dpa_offset, int pos, u8 eiw, u16 eig);

@@ -747,6 +759,10 @@ DEFINE_FREE(put_cxl_dax_region, struct cxl_dax_region *, if (!IS_ERR_OR_NULL(_T)
  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
  void cxl_bus_rescan(void);
  void cxl_bus_drain(void);
+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+				     struct cxl_port *port);
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+				     struct cxl_port *port);
  struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
  				   struct cxl_dport **dport);
  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,

drivers/cxl/mem.c +9 -5

index 798e5c369cfc..91137487ac1f 100644
@@ -133,7 +133,7 @@ static int cxl_mem_probe(struct device *dev)
  		return rc;
 
  	struct cxl_port *parent_port __free(put_cxl_port) =
-		cxl_mem_find_port(cxlmd, &dport);
+		cxl_mem_find_port(cxlmd, NULL);
  	if (!parent_port) {
  		dev_err(dev, "CXL port topology not found\n");
  		return -ENXIO;

@@ -148,10 +148,7 @@ static int cxl_mem_probe(struct device *dev)
  		}
  	}
 
-	if (dport->rch)
-		endpoint_parent = parent_port->uport_dev;
-	else
-		endpoint_parent = &parent_port->dev;
+	endpoint_parent = cxl_port_dport_host(parent_port);
 
  	scoped_guard(device, endpoint_parent) {
  		if (!endpoint_parent->driver) {

@@ -160,6 +157,13 @@ static int cxl_mem_probe(struct device *dev)
  			return -ENXIO;
  		}
 
+		/* @dport is only valid while @endpoint_parent stays bound */
+		dport = cxl_mem_find_dport(cxlmd, parent_port);
+		if (!dport) {
+			dev_err(dev, "CXL topology dport not found\n");
+			return -ENXIO;
+		}
+
  		rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
  		if (rc)
  			return rc;

drivers/cxl/pci.c +6 -5

index c7c91e8dc51d..6e1d7ee94c19 100644
@@ -706,22 +706,23 @@ static ssize_t rcd_pcie_cap_emit(struct device *dev, u16 offset, char *buf, size
  	struct device *root_dev;
  	struct cxl_dport *dport;
  	struct cxl_port *root __free(put_cxl_port) =
-		cxl_mem_find_port(cxlmd, &dport);
+		cxl_mem_find_port(cxlmd, NULL);
 
  	if (!root)
  		return -ENXIO;
 
-	root_dev = root->uport_dev;
+	root_dev = cxl_port_dport_host(root);
  	if (!root_dev)
  		return -ENXIO;
 
-	if (!dport->regs.rcd_pcie_cap)
-		return -ENXIO;
-
  	guard(device)(root_dev);
  	if (!root_dev->driver)
  		return -ENXIO;
 
+	dport = cxl_mem_find_dport(cxlmd, root);
+	if (!dport || !dport->regs.rcd_pcie_cap)
+		return -ENXIO;
+
  	switch (width) {
  	case 2:
  		return sysfs_emit(buf, "%#x\n",

Quoted 7 lines · Dave Jiang

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.

In unbinding case, seems like CXL driver needs to check port's driver before accessing a dport of the port. I guess the dport in devm_cxl_enumerate_ports() and add_port_attach_ep() also needs to be protected by port's lock? Like cxl_add_ep() in devm_cxl_enumerate_ports(), it is invoked without port's lock, the dport could be freed during the function calling.

BTW, just wondering whether adding a dedicated "struct device" to dport makes more sense? use get_device()/put_device() to prevent UAF of dports.

Quoted 275 lines

Both helpers return the dport through an output parameter. The caller's port reference pins struct cxl_port, not the dport allocation, and the lookup itself is a bare xa_load() under no lock.

Add cxl_port_dport_host() to name the device that pins a dport, and cxl_pci_find_dport() / cxl_mem_find_dport() to look one up with that device's lock held and asserted. Convert the four callers to use them and bail if the dport is gone. cxl_port_add_dport() already follows this rule.

cxl_mem_probe() chose its endpoint devm host from dport->rch, which meant dereferencing the dport before taking any lock. Use cxl_port_dport_host():
rch dports only ever land on the root port, so it selects the same device without needing the dport.

Fixes 733b57f262b0 ("cxl/pci: Early setup RCH dport component registers from RCRB")

Signed-off-by Dave Jiang <[email protected]>

Assisted-by LLM

---
v2:
- Reword the cxl_port_dport_host() comment. (Jonathan)
- Drop the note on stale cxlsd->target[] pointers, fixed by patch 1. (Jonathan)

Found by review, not from a failure report.

Untested: two of the four converted sites add a new lock acquisition, and neither executed under test.

   cxl_pci_setup_regs()        new guard() sits behind is_cxl_restricted(), which needs a PCI_EXP_TYPE_RC_END device cxl_handle_rdport_errors()  needs an RCH dport AER error; cxl_test has no injection path

Only cxl_pci_setup_regs() introduces a lock order lockdep has not already
seen: device_lock(&pdev->dev) from probe, then the dport host lock.

cxl_handle_rdport_errors() nests its new host lock inside the device_lock(&cxlmd->dev) that core/ras.c:274 and :298 hold, but cxl_mem_probe() takes that same pair in that same order and ran clean under PROVE_LOCKING, so the edge itself is covered. What is untested there is the path, not the ordering.

Reaching either needs a cxl-type3 attached directly to a host bridge so pci_pcie_type() is PCI_EXP_TYPE_RC_END, which run_qemu.sh does not generate.

Tested in QEMU with cxl_test, PROVE_LOCKING and KASAN on, 8 module load/unload cycles with concurrent sysfs readers. No lockdep splat, no device_lock_assert() firing, enumeration unchanged (15 memdev, 12 port, 192 decoder before and after).

cxl_mem_find_dport() ran for 15 memdevs on each of 9 loads, covering both cxl_port_dport_host() branches. The rch branch is covered too: "cxl_mem
mem2: endpoint9 added to root3" shows the new selection picking the same parent dport->rch did.
---
  drivers/cxl/core/core.h    |  6 +----- drivers/cxl/core/pci.c     |  8 +++++++- drivers/cxl/core/port.c    | 36 ++++++++++++++++++++++++++++++++++++ drivers/cxl/core/ras_rch.c | 12 +++++++++++-

  drivers/cxl/cxl.h          | 16 ++++++++++++++++
  drivers/cxl/mem.c          | 14 +++++++++-----
  drivers/cxl/pci.c          | 11 ++++++-----
  7 files changed, 86 insertions(+), 17 deletions(-)

drivers/cxl/core/core.h +1 -5

index 35eaf636adc9..0a39e500029a 100644
@@ -178,11 +178,7 @@ static inline struct device *port_to_host(struct cxl_port *port)
     static inline struct device *dport_to_host(struct cxl_dport *dport)
   {
-    struct cxl_port *port = dport->port;
-
-    if (is_cxl_root(port))
-        return port->uport_dev;
-    return &port->dev;
+    return cxl_port_dport_host(dport->port);
   }
   #ifdef CONFIG_CXL_RAS
   void cxl_ras_init(void);

drivers/cxl/core/pci.c +0 -0

index 9d807c1a002c..8a236e22bb3f 100644
@@ -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, NULL); if (!port)
              return -EPROBE_DEFER;
  +        guard(device)(cxl_port_dport_host(port));

drivers/cxl/core/pci.c +5 -0

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

drivers/cxl/core/port.c +4 -0

index 6ff3353865e3..336d7c0f5d98 100644
@@ -1428,6 +1428,10 @@ static struct cxl_port *__find_cxl_port_by_dport(struct cxl_find_port_ctx *ctx)
    *
    * Return a 'struct cxl_port' with an elevated reference if found. Use
    * __free(put_cxl_port) to release.
+ *
+ * The port reference does not pin @dport, which is a devm allocation of
+ * cxl_port_dport_host(). Pass NULL and use cxl_pci_find_dport() or
+ * cxl_mem_find_dport() under that device's lock instead.
    */
   static struct cxl_port *find_cxl_port_by_dport(struct device *dport_dev,
                              struct cxl_dport **dport)

@@ -1943,6 +1947,38 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
   }
   EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, "CXL");

  +/**
+ * cxl_pci_find_dport - find the dport of @port that @pdev is below
+ * @pdev: PCI device below the dport
+ * @port: port to search, typically from cxl_pci_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *        must not use the result after dropping it.
+ */

drivers/cxl/core/port.c +24 -0

+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+                     struct cxl_port *port)
+{
+    device_lock_assert(cxl_port_dport_host(port));
+    return cxl_find_dport_by_dev(port, pdev->dev.parent);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_pci_find_dport, "CXL");
+
+/**
+ * cxl_mem_find_dport - find the dport of @port that @cxlmd is below
+ * @cxlmd: memdev below the dport
+ * @port: port to search, typically from cxl_mem_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *        must not use the result after dropping it.
+ */
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+                     struct cxl_port *port)
+{
+    device_lock_assert(cxl_port_dport_host(port));
+    return cxl_find_dport_by_dev(port, grandparent(&cxlmd->dev));
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_find_dport, "CXL");
+

  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, struct cxl_port *port)
  {

drivers/cxl/core/ras_rch.c +0 -0

index e0e01aa5eba6..827a2f64d892 100644
@@ -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, NULL);
      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));

drivers/cxl/core/ras_rch.c +5 -0

+
+    dport = cxl_pci_find_dport(pdev, port);
+    if (!dport)
+        return;
+

      if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) return;
  diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index cab8ce39f465..f08de094f412 100644

drivers/cxl/cxl.h +0 -0

@@ -701,6 +701,18 @@ static inline bool is_cxl_root(struct cxl_port *port)
       return port->uport_dev == port->dev.parent;
   }

  +/*
+ * Return the owner device of the devm allocations of @port's dports. They
+ * are freed when that device unbinds, so hold device_lock() on the returned
+ * device across any lookup and use of a 'struct cxl_dport'.
+ */

drivers/cxl/cxl.h +7 -0

+static inline struct device *cxl_port_dport_host(struct cxl_port *port)
+{
+    if (is_cxl_root(port))
+        return port->uport_dev;
+    return &port->dev;
+}
+

  /* Address translation functions exported to cxl_translate test module only */ int cxl_validate_translation_params(u8 eiw, u16 eig, int pos); u64 cxl_calculate_hpa_offset(u64 dpa_offset, int pos, u8 eiw, u16 eig);

drivers/cxl/cxl.h +4 -0

@@ -747,6 +759,10 @@ DEFINE_FREE(put_cxl_dax_region, struct cxl_dax_region *, if (!IS_ERR_OR_NULL(_T)
   int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
   void cxl_bus_rescan(void);
   void cxl_bus_drain(void);
+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+                     struct cxl_port *port);
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+                     struct cxl_port *port);
   struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
                      struct cxl_dport **dport);
   struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,

drivers/cxl/mem.c +0 -0

index 798e5c369cfc..91137487ac1f 100644
@@ -133,7 +133,7 @@ static int cxl_mem_probe(struct device *dev)
           return rc;

        struct cxl_port *parent_port __free(put_cxl_port) = -        cxl_mem_find_port(cxlmd, &dport);
+        cxl_mem_find_port(cxlmd, NULL);
      if (!parent_port) {
          dev_err(dev, "CXL port topology not found\n"); return -ENXIO;

drivers/cxl/mem.c +1 -3

@@ -148,10 +148,7 @@ static int cxl_mem_probe(struct device *dev)
           }
       }
   -    if (dport->rch)
-        endpoint_parent = parent_port->uport_dev;
-    else
-        endpoint_parent = &parent_port->dev;
+    endpoint_parent = cxl_port_dport_host(parent_port);
         scoped_guard(device, endpoint_parent) {
           if (!endpoint_parent->driver) {

@@ -160,6 +157,13 @@ static int cxl_mem_probe(struct device *dev)
               return -ENXIO;
           }

  +        /* @dport is only valid while @endpoint_parent stays bound */ +        dport = cxl_mem_find_dport(cxlmd, parent_port);
+        if (!dport) {
+            dev_err(dev, "CXL topology dport not found\n"); +            return -ENXIO;
+        }
+
          rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); if (rc)
              return rc;

drivers/cxl/pci.c +0 -0

index c7c91e8dc51d..6e1d7ee94c19 100644
@@ -706,22 +706,23 @@ static ssize_t rcd_pcie_cap_emit(struct device *dev, u16 offset, char *buf, size
       struct device *root_dev;
       struct cxl_dport *dport;

      struct cxl_port *root __free(put_cxl_port) = -        cxl_mem_find_port(cxlmd, &dport); +        cxl_mem_find_port(cxlmd, NULL); if (!root)
          return -ENXIO;
  -    root_dev = root->uport_dev;
+    root_dev = cxl_port_dport_host(root); if (!root_dev)
          return -ENXIO;
  -    if (!dport->regs.rcd_pcie_cap)
-        return -ENXIO;
-
      guard(device)(root_dev);
      if (!root_dev->driver)
          return -ENXIO;
  +    dport = cxl_mem_find_dport(cxlmd, root); +    if (!dport || !dport->regs.rcd_pcie_cap) +        return -ENXIO;
+
      switch (width) {
      case 2:
          return sysfs_emit(buf, "%#x\n",

Quoted 8 lines · Li Ming

Quoted 7 lines · Dave Jiang

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.

Quoted 1 line

In unbinding case, seems like CXL driver needs to check port's driver before accessing a dport of the port. I guess the dport in devm_cxl_enumerate_ports() and add_port_attach_ep() also needs to be protected by port's lock? Like cxl_add_ep() in devm_cxl_enumerate_ports(), it is invoked without port's lock, the dport could be freed during the function calling.

I'll take a look.

Quoted 1 line

BTW, just wondering whether adding a dedicated "struct device" to dport makes more sense? use get_device()/put_device() to prevent UAF of dports.

Do you think using the dport->dport_dev is not sufficient? I fear how much messier the whole thing would be if we add a 'struct device' to dport now.

DJ

Quoted 273 lines

Quoted 273 lines

Both helpers return the dport through an output parameter. The caller's port reference pins struct cxl_port, not the dport allocation, and the lookup itself is a bare xa_load() under no lock.

Add cxl_port_dport_host() to name the device that pins a dport, and cxl_pci_find_dport() / cxl_mem_find_dport() to look one up with that device's lock held and asserted. Convert the four callers to use them and bail if the dport is gone. cxl_port_add_dport() already follows this rule.

cxl_mem_probe() chose its endpoint devm host from dport->rch, which meant dereferencing the dport before taking any lock. Use cxl_port_dport_host():
rch dports only ever land on the root port, so it selects the same device without needing the dport.

Fixes 733b57f262b0 ("cxl/pci: Early setup RCH dport component registers from RCRB")

Signed-off-by Dave Jiang <[email protected]>

Assisted-by LLM

---
v2:
- Reword the cxl_port_dport_host() comment. (Jonathan)
- Drop the note on stale cxlsd->target[] pointers, fixed by patch 1. (Jonathan)

Found by review, not from a failure report.

Untested: two of the four converted sites add a new lock acquisition, and neither executed under test.

   cxl_pci_setup_regs()        new guard() sits behind is_cxl_restricted(), which needs a PCI_EXP_TYPE_RC_END device cxl_handle_rdport_errors()  needs an RCH dport AER error; cxl_test has no injection path

Only cxl_pci_setup_regs() introduces a lock order lockdep has not already
seen: device_lock(&pdev->dev) from probe, then the dport host lock.

cxl_handle_rdport_errors() nests its new host lock inside the device_lock(&cxlmd->dev) that core/ras.c:274 and :298 hold, but cxl_mem_probe() takes that same pair in that same order and ran clean under PROVE_LOCKING, so the edge itself is covered. What is untested there is the path, not the ordering.

Reaching either needs a cxl-type3 attached directly to a host bridge so pci_pcie_type() is PCI_EXP_TYPE_RC_END, which run_qemu.sh does not generate.

Tested in QEMU with cxl_test, PROVE_LOCKING and KASAN on, 8 module load/unload cycles with concurrent sysfs readers. No lockdep splat, no device_lock_assert() firing, enumeration unchanged (15 memdev, 12 port, 192 decoder before and after).

cxl_mem_find_dport() ran for 15 memdevs on each of 9 loads, covering both cxl_port_dport_host() branches. The rch branch is covered too: "cxl_mem
mem2: endpoint9 added to root3" shows the new selection picking the same parent dport->rch did.
---
  drivers/cxl/core/core.h    |  6 +----- drivers/cxl/core/pci.c     |  8 +++++++- drivers/cxl/core/port.c    | 36 ++++++++++++++++++++++++++++++++++++ drivers/cxl/core/ras_rch.c | 12 +++++++++++-

  drivers/cxl/cxl.h          | 16 ++++++++++++++++
  drivers/cxl/mem.c          | 14 +++++++++-----
  drivers/cxl/pci.c          | 11 ++++++-----
  7 files changed, 86 insertions(+), 17 deletions(-)

drivers/cxl/core/core.h +1 -5

index 35eaf636adc9..0a39e500029a 100644
@@ -178,11 +178,7 @@ static inline struct device *port_to_host(struct cxl_port *port)
     static inline struct device *dport_to_host(struct cxl_dport *dport)
   {
-    struct cxl_port *port = dport->port;
-
-    if (is_cxl_root(port))
-        return port->uport_dev;
-    return &port->dev;
+    return cxl_port_dport_host(dport->port);
   }
   #ifdef CONFIG_CXL_RAS
   void cxl_ras_init(void);

drivers/cxl/core/pci.c +0 -0

index 9d807c1a002c..8a236e22bb3f 100644
@@ -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, NULL); if (!port)
              return -EPROBE_DEFER;
  +        guard(device)(cxl_port_dport_host(port));

drivers/cxl/core/pci.c +5 -0

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

drivers/cxl/core/port.c +4 -0

index 6ff3353865e3..336d7c0f5d98 100644
@@ -1428,6 +1428,10 @@ static struct cxl_port *__find_cxl_port_by_dport(struct cxl_find_port_ctx *ctx)
    *
    * Return a 'struct cxl_port' with an elevated reference if found. Use
    * __free(put_cxl_port) to release.
+ *
+ * The port reference does not pin @dport, which is a devm allocation of
+ * cxl_port_dport_host(). Pass NULL and use cxl_pci_find_dport() or
+ * cxl_mem_find_dport() under that device's lock instead.
    */
   static struct cxl_port *find_cxl_port_by_dport(struct device *dport_dev,
                              struct cxl_dport **dport)

@@ -1943,6 +1947,38 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
   }
   EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, "CXL");

  +/**
+ * cxl_pci_find_dport - find the dport of @port that @pdev is below
+ * @pdev: PCI device below the dport
+ * @port: port to search, typically from cxl_pci_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *        must not use the result after dropping it.
+ */

drivers/cxl/core/port.c +24 -0

+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+                     struct cxl_port *port)
+{
+    device_lock_assert(cxl_port_dport_host(port));
+    return cxl_find_dport_by_dev(port, pdev->dev.parent);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_pci_find_dport, "CXL");
+
+/**
+ * cxl_mem_find_dport - find the dport of @port that @cxlmd is below
+ * @cxlmd: memdev below the dport
+ * @port: port to search, typically from cxl_mem_find_port()
+ *
+ * Context: Caller must hold the cxl_port_dport_host() lock of @port, and
+ *        must not use the result after dropping it.
+ */
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+                     struct cxl_port *port)
+{
+    device_lock_assert(cxl_port_dport_host(port));
+    return cxl_find_dport_by_dev(port, grandparent(&cxlmd->dev));
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_find_dport, "CXL");
+

  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, struct cxl_port *port)
  {

drivers/cxl/core/ras_rch.c +0 -0

index e0e01aa5eba6..827a2f64d892 100644
@@ -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, NULL);
      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));

drivers/cxl/core/ras_rch.c +5 -0

+
+    dport = cxl_pci_find_dport(pdev, port);
+    if (!dport)
+        return;
+

      if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) return;
  diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index cab8ce39f465..f08de094f412 100644

drivers/cxl/cxl.h +0 -0

@@ -701,6 +701,18 @@ static inline bool is_cxl_root(struct cxl_port *port)
       return port->uport_dev == port->dev.parent;
   }

  +/*
+ * Return the owner device of the devm allocations of @port's dports. They
+ * are freed when that device unbinds, so hold device_lock() on the returned
+ * device across any lookup and use of a 'struct cxl_dport'.
+ */

drivers/cxl/cxl.h +7 -0

+static inline struct device *cxl_port_dport_host(struct cxl_port *port)
+{
+    if (is_cxl_root(port))
+        return port->uport_dev;
+    return &port->dev;
+}
+

  /* Address translation functions exported to cxl_translate test module only */ int cxl_validate_translation_params(u8 eiw, u16 eig, int pos); u64 cxl_calculate_hpa_offset(u64 dpa_offset, int pos, u8 eiw, u16 eig);

drivers/cxl/cxl.h +4 -0

@@ -747,6 +759,10 @@ DEFINE_FREE(put_cxl_dax_region, struct cxl_dax_region *, if (!IS_ERR_OR_NULL(_T)
   int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
   void cxl_bus_rescan(void);
   void cxl_bus_drain(void);
+struct cxl_dport *cxl_pci_find_dport(struct pci_dev *pdev,
+                     struct cxl_port *port);
+struct cxl_dport *cxl_mem_find_dport(struct cxl_memdev *cxlmd,
+                     struct cxl_port *port);
   struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
                      struct cxl_dport **dport);
   struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,

drivers/cxl/mem.c +0 -0

index 798e5c369cfc..91137487ac1f 100644
@@ -133,7 +133,7 @@ static int cxl_mem_probe(struct device *dev)
           return rc;

        struct cxl_port *parent_port __free(put_cxl_port) = -        cxl_mem_find_port(cxlmd, &dport);
+        cxl_mem_find_port(cxlmd, NULL);
      if (!parent_port) {
          dev_err(dev, "CXL port topology not found\n"); return -ENXIO;

drivers/cxl/mem.c +1 -3

@@ -148,10 +148,7 @@ static int cxl_mem_probe(struct device *dev)
           }
       }
   -    if (dport->rch)
-        endpoint_parent = parent_port->uport_dev;
-    else
-        endpoint_parent = &parent_port->dev;
+    endpoint_parent = cxl_port_dport_host(parent_port);
         scoped_guard(device, endpoint_parent) {
           if (!endpoint_parent->driver) {

@@ -160,6 +157,13 @@ static int cxl_mem_probe(struct device *dev)
               return -ENXIO;
           }

  +        /* @dport is only valid while @endpoint_parent stays bound */ +        dport = cxl_mem_find_dport(cxlmd, parent_port);
+        if (!dport) {
+            dev_err(dev, "CXL topology dport not found\n"); +            return -ENXIO;
+        }
+
          rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); if (rc)
              return rc;

drivers/cxl/pci.c +0 -0

index c7c91e8dc51d..6e1d7ee94c19 100644
@@ -706,22 +706,23 @@ static ssize_t rcd_pcie_cap_emit(struct device *dev, u16 offset, char *buf, size
       struct device *root_dev;
       struct cxl_dport *dport;

      struct cxl_port *root __free(put_cxl_port) = -        cxl_mem_find_port(cxlmd, &dport); +        cxl_mem_find_port(cxlmd, NULL); if (!root)
          return -ENXIO;
  -    root_dev = root->uport_dev;
+    root_dev = cxl_port_dport_host(root); if (!root_dev)
          return -ENXIO;
  -    if (!dport->regs.rcd_pcie_cap)
-        return -ENXIO;
-
      guard(device)(root_dev);
      if (!root_dev->driver)
          return -ENXIO;
  +    dport = cxl_mem_find_dport(cxlmd, root); +    if (!dport || !dport->regs.rcd_pcie_cap) +        return -ENXIO;
+
      switch (width) {
      case 2:
          return sysfs_emit(buf, "%#x\n",