* [PATCH v4 1/3] cxl/port: Clear cached dport pointers when a dport is removed
2026-10-01 15:47 [PATCH v4 0/3] cxl: dport fixes from sashiko reports Dave Jiang
@ 2026-10-01 15:47 ` Dave Jiang
2026-10-01 15:47 ` [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use Dave Jiang
2026-10-01 15:47 ` [PATCH v4 3/3] cxl/port: Attach endpoints to a dport under the lock that pins it Dave Jiang
2 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2026-10-01 15:47 UTC (permalink / raw)
To: linux-cxl
Cc: dave, jic23, alison.schofield, ming.li, icheng, stable,
Jonathan Cameron, Gregory Price (Meta)
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: stable@vger.kernel.org
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Assisted-by: LLM
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Gregory Price (Meta) <gourry@gourry.net>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
---
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(+)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 625e4aa427db..6ff3353865e3 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -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);
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use
2026-10-01 15:47 [PATCH v4 0/3] cxl: dport fixes from sashiko reports Dave Jiang
2026-10-01 15:47 ` [PATCH v4 1/3] cxl/port: Clear cached dport pointers when a dport is removed Dave Jiang
@ 2026-10-01 15:47 ` Dave Jiang
2026-10-01 16:02 ` sashiko-bot
2026-10-01 15:47 ` [PATCH v4 3/3] cxl/port: Attach endpoints to a dport under the lock that pins it Dave Jiang
2 siblings, 1 reply; 6+ messages in thread
From: Dave Jiang @ 2026-10-01 15:47 UTC (permalink / raw)
To: linux-cxl; +Cc: dave, jic23, alison.schofield, ming.li, icheng
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.
With every caller converted, the dport output parameter of
cxl_pci_find_port() and cxl_mem_find_port() is unused. Drop it.
Fixes: 733b57f262b0 ("cxl/pci: Early setup RCH dport component registers from RCRB")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Assisted-by: LLM
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
---
v4:
- Drop the now unused dport output parameter. (Sashiko, Alison)
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 | 62 ++++++++++++++++++++++++++------------
drivers/cxl/core/ras_rch.c | 12 +++++++-
drivers/cxl/cxl.h | 22 +++++++++++---
drivers/cxl/mem.c | 14 ++++++---
drivers/cxl/pci.c | 11 ++++---
7 files changed, 95 insertions(+), 40 deletions(-)
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 35eaf636adc9..0a39e500029a 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -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);
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9d807c1a002c..55a571f86d47 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));
+
+ 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/port.c b/drivers/cxl/core/port.c
index 6ff3353865e3..4cc29433216d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1387,13 +1387,11 @@ static int cxl_add_ep(struct cxl_dport *dport, struct device *ep_dev)
struct cxl_find_port_ctx {
const struct device *dport_dev;
const struct cxl_port *parent_port;
- struct cxl_dport **dport;
};
static int match_port_by_dport(struct device *dev, const void *data)
{
const struct cxl_find_port_ctx *ctx = data;
- struct cxl_dport *dport;
struct cxl_port *port;
if (!is_cxl_port(dev))
@@ -1402,10 +1400,7 @@ static int match_port_by_dport(struct device *dev, const void *data)
return 0;
port = to_cxl_port(dev);
- dport = cxl_find_dport_by_dev(port, ctx->dport_dev);
- if (ctx->dport)
- *ctx->dport = dport;
- return dport != NULL;
+ return !!cxl_find_dport_by_dev(port, ctx->dport_dev);
}
static struct cxl_port *__find_cxl_port_by_dport(struct cxl_find_port_ctx *ctx)
@@ -1424,22 +1419,21 @@ static struct cxl_port *__find_cxl_port_by_dport(struct cxl_find_port_ctx *ctx)
/**
* find_cxl_port_by_dport - find a cxl_port by one of its targets
* @dport_dev: device representing the dport target
- * @dport: optional output of the 'struct cxl_dport' companion of the @dport_dev
*
* Return a 'struct cxl_port' with an elevated reference if found. Use
* __free(put_cxl_port) to release.
+ *
+ * The port reference does not pin the dport, which is a devm allocation of
+ * cxl_port_dport_host(). Use cxl_pci_find_dport() or cxl_mem_find_dport()
+ * under that device's lock to get it.
*/
-static struct cxl_port *find_cxl_port_by_dport(struct device *dport_dev,
- struct cxl_dport **dport)
+static struct cxl_port *find_cxl_port_by_dport(struct device *dport_dev)
{
struct cxl_find_port_ctx ctx = {
.dport_dev = dport_dev,
- .dport = dport,
};
- struct cxl_port *port;
- port = __find_cxl_port_by_dport(&ctx);
- return port;
+ return __find_cxl_port_by_dport(&ctx);
}
/*
@@ -1929,20 +1923,50 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, "CXL");
-struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
- struct cxl_dport **dport)
+struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev)
{
- return find_cxl_port_by_dport(pdev->dev.parent, dport);
+ return find_cxl_port_by_dport(pdev->dev.parent);
}
EXPORT_SYMBOL_NS_GPL(cxl_pci_find_port, "CXL");
-struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
- struct cxl_dport **dport)
+struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd)
{
- return find_cxl_port_by_dport(grandparent(&cxlmd->dev), dport);
+ return find_cxl_port_by_dport(grandparent(&cxlmd->dev));
}
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)
{
diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
index e0e01aa5eba6..3c6aae26deed 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));
+
+ 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..cd636d014872 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -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,10 +759,12 @@ 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_port *cxl_pci_find_port(struct pci_dev *pdev,
- struct cxl_dport **dport);
-struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
- struct cxl_dport **dport);
+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_port *cxl_mem_find_port(struct cxl_memdev *cxlmd);
bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 798e5c369cfc..165cb048b177 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -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);
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;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index c7c91e8dc51d..913c7517420f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -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);
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",
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use
2026-10-01 15:47 ` [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use Dave Jiang
@ 2026-10-01 16:02 ` sashiko-bot
2026-10-01 17:36 ` Dave Jiang
0 siblings, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-10-01 16:02 UTC (permalink / raw)
To: Dave Jiang; +Cc: linux-cxl
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use
2026-10-01 16:02 ` sashiko-bot
@ 2026-10-01 17:36 ` Dave Jiang
0 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2026-10-01 17:36 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-cxl
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;
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 3/3] cxl/port: Attach endpoints to a dport under the lock that pins it
2026-10-01 15:47 [PATCH v4 0/3] cxl: dport fixes from sashiko reports Dave Jiang
2026-10-01 15:47 ` [PATCH v4 1/3] cxl/port: Clear cached dport pointers when a dport is removed Dave Jiang
2026-10-01 15:47 ` [PATCH v4 2/3] cxl/core: Hold the dport host lock across dport lookup and use Dave Jiang
@ 2026-10-01 15:47 ` Dave Jiang
2 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2026-10-01 15:47 UTC (permalink / raw)
To: linux-cxl; +Cc: dave, jic23, alison.schofield, ming.li, icheng
devm_cxl_enumerate_ports() and add_port_attach_ep() look up or create a
dport under its port's device lock, drop the lock, then pass the dport
to cxl_add_ep(). add_ep() reads dport->port before taking the lock, and
cxl_gpf_port_setup() reads and writes the dport with no lock held.
A concurrent cxl_detach_ep() for another memdev below the same port can
free the dport in that window: it calls del_dports() under the port lock
when the last endpoint leaves. The enumerating memdev then dereferences
freed memory.
Attach the endpoint and set up GPF while the port lock from the lookup
is still held. Make add_ep() assert that lock instead of taking it.
Under the lock, a dport found in port->dports is still live, since
removal erases it from the xarray before freeing it.
Fixes: de516b40116e ("cxl/port: Record dport in endpoint references")
Reported-by: Li Ming <ming.li@zohomail.com>
Closes: https://lore.kernel.org/linux-cxl/0c68324c-cfc7-46b2-9344-24ae40e88923@zohomail.com/
Assisted-by: LLM
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
---
Found by review, not from a failure report.
Tested in QEMU with cxl_test, PROVE_LOCKING and KASAN on: 6 module
load/unload cycles, 12 concurrent target_list readers, and each cycle 20
rounds of every memdev unbinding and rebinding in parallel. No KASAN
report, no WARNING, no lockdep splat; debug_locks stays 1. Both converted
paths run under this load, so the new device_lock_assert() in add_ep()
never fired.
Enumeration is unchanged against the same test at patch 2: 11 memdev,
8 port, 11 endpoint, 158 decoder after every load.
The races produce transient -EBUSY cxl_mem probe failures (424 with this
patch, 407 without) from the pending detach_work check in
cxl_mem_probe(). Memdevs left unbound rebind on the first retry.
---
drivers/cxl/core/port.c | 115 ++++++++++++++++++++++------------------
1 file changed, 62 insertions(+), 53 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4cc29433216d..25e582718c06 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1349,7 +1349,7 @@ static int add_ep(struct cxl_ep *new)
{
struct cxl_port *port = new->dport->port;
- guard(device)(&port->dev);
+ device_lock_assert(&port->dev);
if (port->dead)
return -ENXIO;
@@ -1365,6 +1365,9 @@ static int add_ep(struct cxl_ep *new)
* Intermediate CXL ports are scanned based on the arrival of endpoints.
* When those endpoints depart the port can be destroyed once all
* endpoints that care about that port have been removed.
+ *
+ * Context: Caller must hold the device lock of @dport->port, which pins
+ * @dport, across the lookup of @dport and this call.
*/
static int cxl_add_ep(struct cxl_dport *dport, struct device *ep_dev)
{
@@ -1695,13 +1698,15 @@ static struct cxl_dport *probe_dport(struct cxl_port *port,
return drv->add_dport(port, dport_dev);
}
-static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
- struct cxl_port *parent_port,
- struct cxl_dport *parent_dport,
- struct device *uport_dev,
- struct device *dport_dev)
+static int devm_cxl_create_port(struct device *ep_dev,
+ struct cxl_port *parent_port,
+ struct cxl_dport *parent_dport,
+ struct device *uport_dev,
+ struct device *dport_dev)
{
resource_size_t component_reg_phys;
+ struct cxl_dport *dport;
+ int rc;
device_lock_assert(&parent_port->dev);
if (!parent_port->dev.driver) {
@@ -1718,7 +1723,7 @@ static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
port = devm_cxl_add_port(&parent_port->dev, uport_dev,
component_reg_phys, parent_dport);
if (IS_ERR(port))
- return ERR_CAST(port);
+ return PTR_ERR(port);
/*
* retry to make sure a port is found. a port device
@@ -1726,7 +1731,7 @@ static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
*/
port = find_cxl_port_by_uport(uport_dev);
if (!port)
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
dev_dbg(ep_dev, "created port %s:%s\n",
dev_name(&port->dev), dev_name(port->uport_dev));
@@ -1735,11 +1740,25 @@ static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
* Port was created before right before this function is
* called. Signal the caller to deal with it.
*/
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
+ /* @dport is only valid while @port's lock is held */
guard(device)(&port->dev);
- return probe_dport(port, dport_dev);
+ dport = probe_dport(port, dport_dev);
+ if (IS_ERR(dport))
+ return PTR_ERR(dport);
+
+ rc = cxl_add_ep(dport, ep_dev);
+ if (rc == -EBUSY) {
+ /*
+ * "can't" happen, but this error code means
+ * something to the caller, so translate it.
+ */
+ rc = -ENXIO;
+ }
+
+ return rc;
}
static int add_port_attach_ep(struct cxl_memdev *cxlmd,
@@ -1747,7 +1766,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
struct device *dport_dev)
{
struct device *dparent = grandparent(dport_dev);
- struct cxl_dport *dport, *parent_dport;
+ struct cxl_dport *parent_dport;
int rc;
if (is_cxl_host_bridge(dparent)) {
@@ -1776,33 +1795,23 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
return PTR_ERR(parent_dport);
}
- dport = devm_cxl_create_port(&cxlmd->dev, parent_port,
- parent_dport, uport_dev,
- dport_dev);
- if (IS_ERR(dport)) {
- /* Port or dport already exists, restart iteration */
- if (PTR_ERR(dport) == -EAGAIN || PTR_ERR(dport) == -EBUSY)
- return 0;
- return PTR_ERR(dport);
- }
+ rc = devm_cxl_create_port(&cxlmd->dev, parent_port,
+ parent_dport, uport_dev, dport_dev);
}
- rc = cxl_add_ep(dport, &cxlmd->dev);
- if (rc == -EBUSY) {
- /*
- * "can't" happen, but this error code means
- * something to the caller, so translate it.
- */
- rc = -ENXIO;
- }
+ /* Port or dport already exists, restart iteration */
+ if (rc == -EAGAIN || rc == -EBUSY)
+ return 0;
return rc;
}
-static struct cxl_dport *find_or_add_dport(struct cxl_port *port,
- struct device *dport_dev)
+static int find_or_add_dport_attach_ep(struct cxl_memdev *cxlmd,
+ struct cxl_port *port,
+ struct device *dport_dev)
{
struct cxl_dport *dport;
+ int rc;
/*
* The port is already visible in CXL hierarchy, but it may still
@@ -1818,13 +1827,28 @@ static struct cxl_dport *find_or_add_dport(struct cxl_port *port,
if (!dport) {
dport = probe_dport(port, dport_dev);
if (IS_ERR(dport))
- return dport;
+ return PTR_ERR(dport);
/* New dport added, restart iteration */
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
- return dport;
+ /* @dport is only valid while @port's lock is held */
+ rc = cxl_add_ep(dport, &cxlmd->dev);
+
+ /*
+ * If the endpoint already exists in the port's list,
+ * that's ok, it was added on a previous pass.
+ * Otherwise, retry in add_port_attach_ep() after taking
+ * the parent_port lock as the current port may be being
+ * reaped.
+ */
+ if (rc && rc != -EBUSY)
+ return rc;
+
+ cxl_gpf_port_setup(dport);
+
+ return 0;
}
int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
@@ -1853,7 +1877,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
for (iter = dev; iter; iter = grandparent(iter)) {
struct device *dport_dev = grandparent(iter);
struct device *uport_dev;
- struct cxl_dport *dport;
if (is_cxl_host_bridge(dport_dev))
return 0;
@@ -1880,27 +1903,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
* RP port enumerated by cxl_acpi without dport will
* have the dport added here.
*/
- dport = find_or_add_dport(port, dport_dev);
- if (IS_ERR(dport)) {
- if (PTR_ERR(dport) == -EAGAIN)
- goto retry;
- return PTR_ERR(dport);
- }
-
- rc = cxl_add_ep(dport, &cxlmd->dev);
-
- /*
- * If the endpoint already exists in the port's list,
- * that's ok, it was added on a previous pass.
- * Otherwise, retry in add_port_attach_ep() after taking
- * the parent_port lock as the current port may be being
- * reaped.
- */
- if (rc && rc != -EBUSY)
+ rc = find_or_add_dport_attach_ep(cxlmd, port,
+ dport_dev);
+ if (rc == -EAGAIN)
+ goto retry;
+ if (rc)
return rc;
- cxl_gpf_port_setup(dport);
-
/* Any more ports to add between this one and the root? */
if (!dev_is_cxl_root_child(&port->dev))
continue;
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread