[PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[Li Ming]

Currently, CXL subsystem implementation has some functions that may
access CXL memdev's endpoint before the endpoint initialization
completed or without checking the CXL memdev endpoint validity. 
This patchset fixes three scenarios as above description.

1. cxl_dpa_to_region() is possible to access an invalid CXL memdev
   endpoint.
   there are two scenarios that can trigger this issue:
   a. memdev poison injection/clearing debugfs interfaces:
      devm_cxl_add_endpoint() is used to register CXL memdev endpoint
      and update cxlmd->endpoint from -ENXIO to the endpoint structure.
      memdev poison injection/clearing debugfs interfaces are registered
      before devm_cxl_add_endpoint() is invoked in cxl_mem_probe().
      There is a small window where user can use the debugfs interfaces
      to access an invalid endpoint.
   b. cxl_event_config() in the end of cxl_pci_probe():
      cxl_event_config() invokes cxl_mem_get_event_record() to get
      remain event logs from CXL device during cxl_pci_probe(). If CXL
      memdev probing failed before that, it is also possible to access
      an invalid endpoint.
   To fix these two cases, cxl_dpa_to_region() requires callers holding
   CXL memdev lock to access it and check if CXL memdev driver bingding
   status. Holding CXL memdev lock ensures that CXL memdev probing has
   completed, and if CXL memdev driver is bound, it will mean
   cxlmd->endpoint is valid. (PATCH #1-#5)

2. cxl_reset_done() callback in cxl_pci module.
   cxl_reset_done() callback also accesses cxlmd->endpoint without any
   checking. If CXL memdev probing fails, then cxl_reset_done() is
   called by PCI subsystem, it will access an invalid endpoint. The
   solution is adding a CXL memdev driver binding status inside
   cxl_reset_done(). (PATCH #6)

Besides, the patchset also includes a fix for cxlmd->endpoint reset,
cxlmd->endpoint is set to -ENXIO by default during cxlmd allocation. It
will be updated when endpoint is allocated and added to the bus.
However, the CXL driver does not reset it to -ENXIO when the endpoint is
released. (PATCH #7)

---
Li Ming (7):
      driver core: Add conditional guard support for device_lock()
      cxl/memdev: Hold memdev lock during memdev poison injection/clear
      cxl/region: Hold memdev lock during region poison injection/clear
      cxl/pci: Hold memdev lock in cxl_event_trace_record()
      cxl/region: Ensure endpoint is valid in cxl_dpa_to_region()
      cxl/pci: Check memdev driver binding status in cxl_reset_done()
      cxl/port: Reset cxlmd->endpoint to -ENXIO by default

 drivers/cxl/core/core.h   |   4 +-
 drivers/cxl/core/mbox.c   |   5 ++-
 drivers/cxl/core/memdev.c |  10 +++++
 drivers/cxl/core/port.c   |  14 ++++--
 drivers/cxl/core/region.c | 112 ++++++++++++++++++++++++++++++++++------------
 drivers/cxl/cxlmem.h      |   2 +-
 drivers/cxl/pci.c         |   3 ++
 include/linux/device.h    |   1 +
 8 files changed, 114 insertions(+), 37 deletions(-)
---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20260308-fix_access_endpoint_without_drv_check-f2e6ff4bdc48

Best regards,
-- 
Li Ming <ming.li@zohomail.com>

[PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Li Ming]

Introduce conditional guard version of device_lock() for scenarios that
require conditional device lock holding.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 include/linux/device.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 0be95294b6e6..4fafee80524b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
 }
 
 DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
+DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
 
 static inline void device_lock_assert(struct device *dev)
 {

-- 
2.43.0

[PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Li Ming]

CXL memdev poison injection/clearing debugfs interfaces are visible
before the CXL memdev endpoint initialization, If user accesses the
interfaces before cxlmd->endpoint updated, it is possible to access an
invalid endpoint in cxl_dpa_to_region().

Hold CXL memdev lock at the beginning of the interfaces, this blocks the
interfaces until CXL memdev probing completed.

The following patch will check the given endpoint validity in
cxl_dpa_to_region().

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/memdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 273c22118d3d..8ebaf9e96035 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
+	device_lock_assert(&cxlmd->dev);
 	lockdep_assert_held(&cxl_rwsem.dpa);
 	lockdep_assert_held(&cxl_rwsem.region);
 
@@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	int rc;
 
+	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
+	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+		return rc;
+
 	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
@@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
+	device_lock_assert(&cxlmd->dev);
 	lockdep_assert_held(&cxl_rwsem.dpa);
 	lockdep_assert_held(&cxl_rwsem.region);
 
@@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	int rc;
 
+	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
+	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+		return rc;
+
 	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;

-- 
2.43.0

[PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear

[Li Ming]

cxl_dpa_to_region() will require callers holding the given CXL memdev
lock for endpoint validity checking in the following patch. To prepare
it, region poison injection/clearing debugfs interfaces need to ensure
the correct CXL memdev lock is held at the beginning.

To keep lock sequence(cxlmd.dev -> cxl_rwsem.region -> cxl_rwsem.dpa)
for avoiding deadlock. the interfaces have to find out the correct CXL
memdev at first, holding lock in the sequence then checking if the DPA
data has been changed before holding locks.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/region.c | 101 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 24 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 42874948b589..806512dfab07 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -4074,12 +4074,60 @@ static int validate_region_offset(struct cxl_region *cxlr, u64 offset)
 	return 0;
 }
 
+static int cxl_region_poison_lookup_locked(struct cxl_region *cxlr, u64 offset,
+					   struct dpa_result *res)
+{
+	int rc;
+
+	*res = (struct dpa_result){ .dpa = ULLONG_MAX, .cxlmd = NULL };
+
+	if (validate_region_offset(cxlr, offset))
+		return -EINVAL;
+
+	offset -= cxlr->params.cache_size;
+	rc = region_offset_to_dpa_result(cxlr, offset, res);
+	if (rc || !res->cxlmd || res->dpa == ULLONG_MAX) {
+		dev_dbg(&cxlr->dev,
+			"Failed to resolve DPA for region offset %#llx rc %d\n",
+			offset, rc);
+
+		return rc ? rc : -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cxl_region_poison_lookup(struct cxl_region *cxlr, u64 offset,
+				    struct dpa_result *res)
+{
+	int rc;
+
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
+		return rc;
+
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
+		return rc;
+
+	return cxl_region_poison_lookup_locked(cxlr, offset, res);
+}
+
 static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
 {
-	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
 	struct cxl_region *cxlr = data;
+	struct dpa_result res1, res2;
 	int rc;
 
+	/* To retrieve the correct memdev */
+	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
+	if (rc)
+		return rc;
+
+	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
+	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+		return rc;
+
 	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
@@ -4088,20 +4136,18 @@ static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
 		return rc;
 
-	if (validate_region_offset(cxlr, offset))
-		return -EINVAL;
-
-	offset -= cxlr->params.cache_size;
-	rc = region_offset_to_dpa_result(cxlr, offset, &result);
-	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
+	/*
+	 * Retrieve memdev and DPA data again in case that the data
+	 * has been changed before holding locks.
+	 */
+	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
+	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
 		dev_dbg(&cxlr->dev,
-			"Failed to resolve DPA for region offset %#llx rc %d\n",
-			offset, rc);
-
-		return rc ? rc : -EINVAL;
+			"Error injection raced region reconfiguration: %d", rc);
+		return -ENXIO;
 	}
 
-	return cxl_inject_poison_locked(result.cxlmd, result.dpa);
+	return cxl_inject_poison_locked(res2.cxlmd, res2.dpa);
 }
 
 DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
@@ -4109,10 +4155,19 @@ DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
 
 static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
 {
-	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
 	struct cxl_region *cxlr = data;
+	struct dpa_result res1, res2;
 	int rc;
 
+	/* To retrieve the correct memdev */
+	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
+	if (rc)
+		return rc;
+
+	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
+	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+		return rc;
+
 	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
@@ -4121,20 +4176,18 @@ static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
 	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
 		return rc;
 
-	if (validate_region_offset(cxlr, offset))
-		return -EINVAL;
-
-	offset -= cxlr->params.cache_size;
-	rc = region_offset_to_dpa_result(cxlr, offset, &result);
-	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
+	/*
+	 * Retrieve memdev and DPA data again in case that the data
+	 * has been changed before holding locks.
+	 */
+	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
+	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
 		dev_dbg(&cxlr->dev,
-			"Failed to resolve DPA for region offset %#llx rc %d\n",
-			offset, rc);
-
-		return rc ? rc : -EINVAL;
+			"Error clearing raced region reconfiguration: %d", rc);
+		return -ENXIO;
 	}
 
-	return cxl_clear_poison_locked(result.cxlmd, result.dpa);
+	return cxl_clear_poison_locked(res2.cxlmd, res2.dpa);
 }
 
 DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,

-- 
2.43.0

[PATCH 4/7] cxl/pci: Hold memdev lock in cxl_event_trace_record()

[Li Ming]

This is a preparatory patch for the following changes.

To enable endpoint validity checks in cxl_dpa_to_region().
cxl_dpa_to_region() has to require caller to hold CXL memdev lock to
ensure the CXL memdev probing is completed.

So holding the given CXL memdev lock before invoking cxl_dpa_to_region()
in cxl_event_trace_record().

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/mbox.c | 5 +++--
 drivers/cxl/cxlmem.h    | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index e7a6452bf544..3f34bbabf4d3 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -893,7 +893,7 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, "CXL");
 
-void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
+void cxl_event_trace_record(struct cxl_memdev *cxlmd,
 			    enum cxl_event_log_type type,
 			    enum cxl_event_type event_type,
 			    const uuid_t *uuid, union cxl_event *evt)
@@ -920,6 +920,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 		 * translations. Take topology mutation locks and lookup
 		 * { HPA, REGION } from { DPA, MEMDEV } in the event record.
 		 */
+		guard(device)(&cxlmd->dev);
 		guard(rwsem_read)(&cxl_rwsem.region);
 		guard(rwsem_read)(&cxl_rwsem.dpa);
 
@@ -968,7 +969,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, "CXL");
 
-static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
+static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
 				     enum cxl_event_log_type type,
 				     struct cxl_event_record_raw *record)
 {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index e21d744d639b..7a34a19c02c8 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -864,7 +864,7 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				  unsigned long *cmds);
 void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
-void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
+void cxl_event_trace_record(struct cxl_memdev *cxlmd,
 			    enum cxl_event_log_type type,
 			    enum cxl_event_type event_type,
 			    const uuid_t *uuid, union cxl_event *evt);

-- 
2.43.0

[PATCH 5/7] cxl/region: Ensure endpoint is valid in cxl_dpa_to_region()

[Li Ming]

cxl_dpa_to_region() needs to access the endpoint of the given CXL memdev
to confirm whether the CXL memdev has memory range attached to a CXL
region. But it is possible to be called before endpoint allocation or
after the CXL memdev probing failure.  In these two cases,
cxlmd->endpoint is invalid.

To solve the problem, cxl_dpa_to_region() requires that callers have to
hold the given CXL memdev lock, it guarantees the CXL memdev probing has
completed. And cxl_dpa_to_region() will check the given CXL memdev
driver binding status, if the memdev has bound to the driver, the
endpoint is guaranteed to be valid. This checking also requires the CXL
memdev lock held.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/core.h   |  4 ++--
 drivers/cxl/core/region.c | 11 +++++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 5b0570df0fd9..fa8402be860f 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -47,7 +47,7 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
 int cxl_region_init(void);
 void cxl_region_exit(void);
 int cxl_get_poison_by_endpoint(struct cxl_port *port);
-struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
+struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa);
 u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 		   u64 dpa);
 
@@ -58,7 +58,7 @@ static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
 	return ULLONG_MAX;
 }
 static inline
-struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
+struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	return NULL;
 }
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 806512dfab07..c555e3ae8b62 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2947,16 +2947,19 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
 	return 1;
 }
 
-struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
+struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	struct cxl_dpa_to_region_context ctx;
-	struct cxl_port *port;
+	struct cxl_port *port = cxlmd->endpoint;
+
+	device_lock_assert(&cxlmd->dev);
+	if (!cxlmd->dev.driver)
+		return NULL;
 
 	ctx = (struct cxl_dpa_to_region_context) {
 		.dpa = dpa,
 	};
-	port = cxlmd->endpoint;
-	if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
+	if (cxl_num_decoders_committed(port))
 		device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
 
 	return ctx.cxlr;

-- 
2.43.0

[PATCH 6/7] cxl/pci: Check memdev driver binding status in cxl_reset_done()

[Li Ming]

cxl_reset_done() accesses the endpoint of the corresponding CXL memdev
without endpoint validity checking. By default, cxlmd->endpoint is
initialized to -ENXIO, if cxl_reset_done() is triggered after the
corresponding CXL memdev probing failed, this results in access to an
invalid endpoint.

CXL subsystem can always check CXL memdev driver binding status to
confirm its endpoint validity. So adding the CXL memdev driver checking
inside cxl_reset_done() to avoid accessing an invalid endpoint.

Fixes: 934edcd436dc ("cxl: Add post-reset warning if reset results in loss of previously committed HDM decoders")
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index fbb300a01830..a5922116db2a 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1043,6 +1043,9 @@ static void cxl_reset_done(struct pci_dev *pdev)
 	 * that no longer exists.
 	 */
 	guard(device)(&cxlmd->dev);
+	if (!cxlmd->dev.driver)
+		return;
+
 	if (cxlmd->endpoint &&
 	    cxl_endpoint_decoder_reset_detected(cxlmd->endpoint)) {
 		dev_crit(dev, "SBR happened without memory regions removal.\n");

-- 
2.43.0

[PATCH 7/7] cxl/port: Reset cxlmd->endpoint to -ENXIO by default

[Li Ming]

cxlmd->endpoint is set to -ENXIO by default, This intentional invalid
value ensures that any unintended access to the endpoint will be
detectable. But CXL driver didn't reset it to the default value when the
endpoint is released in delete_endpoint().

Besides, cxlmd->endpoint is updated to point to an valid endpoint in
cxl_port_add(), but if the device_add() in cxl_port_add() fails, the
endpoint will be released, but cxlmd->endpoint remains pointing to the
released endpoint, it may introduce a potential use-after-free issue.

Fixes: 3d8be8b398e3 ("cxl: Set cxlmd->endpoint before adding port device")
Fixes: 29317f8dc6ed ("cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation")
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/port.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0c5957d1d329..ec3cb62b44b7 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -834,11 +834,14 @@ static int cxl_port_add(struct cxl_port *port,
 			struct cxl_dport *parent_dport)
 {
 	struct device *dev __free(put_device) = &port->dev;
+	struct cxl_memdev *cxlmd = NULL;
 	int rc;
 
 	if (is_cxl_memdev(port->uport_dev)) {
-		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
-		struct cxl_dev_state *cxlds = cxlmd->cxlds;
+		struct cxl_dev_state *cxlds;
+
+		cxlmd = to_cxl_memdev(port->uport_dev);
+		cxlds = cxlmd->cxlds;
 
 		rc = dev_set_name(dev, "endpoint%d", port->id);
 		if (rc)
@@ -865,8 +868,11 @@ static int cxl_port_add(struct cxl_port *port,
 	}
 
 	rc = device_add(dev);
-	if (rc)
+	if (rc) {
+		if (cxlmd)
+			cxlmd->endpoint = ERR_PTR(-ENXIO);
 		return rc;
+	}
 
 	/* Inhibit the cleanup function invoked */
 	dev = NULL;
@@ -1425,7 +1431,7 @@ static void delete_endpoint(void *data)
 			devm_release_action(host, cxl_unlink_uport, endpoint);
 			devm_release_action(host, unregister_port, endpoint);
 		}
-		cxlmd->endpoint = NULL;
+		cxlmd->endpoint = ERR_PTR(-ENXIO);
 	}
 	put_device(&endpoint->dev);
 	put_device(host);

-- 
2.43.0

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Dave Jiang]

On 3/10/26 8:57 AM, Li Ming wrote:
> Introduce conditional guard version of device_lock() for scenarios that
> require conditional device lock holding.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  include/linux/device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0be95294b6e6..4fafee80524b 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
>  }
>  
>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)

Can you please just squash this small change to the same patch that is using it? Thanks!

>  
>  static inline void device_lock_assert(struct device *dev)
>  {
> 

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Dave Jiang]

On 3/10/26 8:57 AM, Li Ming wrote:
> CXL memdev poison injection/clearing debugfs interfaces are visible
> before the CXL memdev endpoint initialization, If user accesses the
> interfaces before cxlmd->endpoint updated, it is possible to access an
> invalid endpoint in cxl_dpa_to_region().
> 
> Hold CXL memdev lock at the beginning of the interfaces, this blocks the
> interfaces until CXL memdev probing completed.
> 
> The following patch will check the given endpoint validity in
> cxl_dpa_to_region().
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/memdev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 273c22118d3d..8ebaf9e96035 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);
>  
> @@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> @@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);
>  
> @@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> 

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Danilo Krummrich]

On Tue Mar 10, 2026 at 6:45 PM CET, Dave Jiang wrote:
> On 3/10/26 8:57 AM, Li Ming wrote:
>> Introduce conditional guard version of device_lock() for scenarios that
>> require conditional device lock holding.
>> 
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>  include/linux/device.h | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 0be95294b6e6..4fafee80524b 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
>>  }
>>  
>>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
>> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
>
> Can you please just squash this small change to the same patch that is using it? Thanks!

Why? It is a single logical change and hence should be a separate patch, no?

We even tell contributors in the documentation [1] that adding new APIs and
using them should be separate patches.

Additionally, in this case it affects another subsystem, so it also makes sense
in terms of making the change obvious to the maintainers of the other subsystem.

[1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Dave Jiang]

On 3/10/26 11:06 AM, Danilo Krummrich wrote:
> On Tue Mar 10, 2026 at 6:45 PM CET, Dave Jiang wrote:
>> On 3/10/26 8:57 AM, Li Ming wrote:
>>> Introduce conditional guard version of device_lock() for scenarios that
>>> require conditional device lock holding.
>>>
>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>>> ---
>>>  include/linux/device.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 0be95294b6e6..4fafee80524b 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
>>>  }
>>>  
>>>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
>>> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
>>
>> Can you please just squash this small change to the same patch that is using it? Thanks!
> 
> Why? It is a single logical change and hence should be a separate patch, no?

For some reason I missed it's in linux/device.h. So sure ok. But typically I would like to see the usage if it's in the same sub-system.

DJ

> 
> We even tell contributors in the documentation [1] that adding new APIs and
> using them should be separate patches.
> 
> Additionally, in this case it affects another subsystem, so it also makes sense
> in terms of making the change obvious to the maintainers of the other subsystem.
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Dan Williams]

Dave Jiang wrote:
[..]
> >>> diff --git a/include/linux/device.h b/include/linux/device.h
> >>> index 0be95294b6e6..4fafee80524b 100644
> >>> --- a/include/linux/device.h
> >>> +++ b/include/linux/device.h
> >>> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
> >>>  }
> >>>  
> >>>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
> >>> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
> >>
> >> Can you please just squash this small change to the same patch that is using it? Thanks!
> > 
> > Why? It is a single logical change and hence should be a separate patch, no?
> 
> For some reason I missed it's in linux/device.h. So sure ok. But
> typically I would like to see the usage if it's in the same
> sub-system.

I generally expect the same as well.

...however, when we get into multiple in flight patch sets wanting the
same API [1] it would be nice to have a stable commit id to share, Greg?

[1]: TEE I/O enabling also has a use case, and introduced the helper
with the "first" user.
http://lore.kernel.org/20260303000207.1836586-7-dan.j.williams@intel.com

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Danilo Krummrich]

On Tue Mar 10, 2026 at 7:39 PM CET, Dan Williams wrote:
> Dave Jiang wrote:
> [..]
>> >>> diff --git a/include/linux/device.h b/include/linux/device.h
>> >>> index 0be95294b6e6..4fafee80524b 100644
>> >>> --- a/include/linux/device.h
>> >>> +++ b/include/linux/device.h
>> >>> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
>> >>>  }
>> >>>  
>> >>>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
>> >>> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
>> >>
>> >> Can you please just squash this small change to the same patch that is using it? Thanks!
>> > 
>> > Why? It is a single logical change and hence should be a separate patch, no?
>> 
>> For some reason I missed it's in linux/device.h. So sure ok. But
>> typically I would like to see the usage if it's in the same
>> sub-system.
>
> I generally expect the same as well.
>
> ...however, when we get into multiple in flight patch sets wanting the
> same API [1] it would be nice to have a stable commit id to share, Greg?
>
> [1]: TEE I/O enabling also has a use case, and introduced the helper
> with the "first" user.
> http://lore.kernel.org/20260303000207.1836586-7-dan.j.williams@intel.com

This patch has neither Rafael, me nor the driver-core mailing list Cc'd and
buries this change in a patch named "PCI/TSM: Add Device Security (TVM Guest)
LOCK operation support", which makes pretty it hard to catch.

Please submit such changes as a separate patch and send it to all maintainers
and the corresponding mailing list such that people have a chance to take note
of it.

As for sharing the commit throughout multiple trees, I can provide a signed tag
similar to [1] once Greg and Rafael had a chance to take a look at this patch as
well.

You may also want to sort out authorship / tags with Li.

Thanks,
Danilo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tag/?h=platform_device_info_swnode-7.1-rc1

Re: [PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[Alison Schofield]

On Tue, Mar 10, 2026 at 11:57:52PM +0800, Li Ming wrote:
> Currently, CXL subsystem implementation has some functions that may
> access CXL memdev's endpoint before the endpoint initialization
> completed or without checking the CXL memdev endpoint validity. 
> This patchset fixes three scenarios as above description.

Hi Ming, 

I notice you didn't give each patch a fixes tag, I'll comment
in that per patch.

Below - a couple of questions about reproducing these failures:

> 
> 1. cxl_dpa_to_region() is possible to access an invalid CXL memdev
>    endpoint.
>    there are two scenarios that can trigger this issue:
>    a. memdev poison injection/clearing debugfs interfaces:
>       devm_cxl_add_endpoint() is used to register CXL memdev endpoint
>       and update cxlmd->endpoint from -ENXIO to the endpoint structure.
>       memdev poison injection/clearing debugfs interfaces are registered
>       before devm_cxl_add_endpoint() is invoked in cxl_mem_probe().
>       There is a small window where user can use the debugfs interfaces
>       to access an invalid endpoint.

Do you have a reproducer for this 1.a above?
Meson unit test does support parallel execution, so we could create
a race btw poison inject and region reconfig. It's an interesting task,
yet low priority considering the debugfs feature is for our expert users.
If you do have something, please share.

>    b. cxl_event_config() in the end of cxl_pci_probe():
>       cxl_event_config() invokes cxl_mem_get_event_record() to get
>       remain event logs from CXL device during cxl_pci_probe(). If CXL
>       memdev probing failed before that, it is also possible to access
>       an invalid endpoint.

b. is reproducible.
Since your original find of this, I've been thinking about adding a
new group of tests, like the 'stressors', so that we can do things 
like this on demand, but not as default in the existing cxl suite.


>    To fix these two cases, cxl_dpa_to_region() requires callers holding
>    CXL memdev lock to access it and check if CXL memdev driver bingding
>    status. Holding CXL memdev lock ensures that CXL memdev probing has
>    completed, and if CXL memdev driver is bound, it will mean
>    cxlmd->endpoint is valid. (PATCH #1-#5)
> 
> 2. cxl_reset_done() callback in cxl_pci module.
>    cxl_reset_done() callback also accesses cxlmd->endpoint without any
>    checking. If CXL memdev probing fails, then cxl_reset_done() is
>    called by PCI subsystem, it will access an invalid endpoint. The
>    solution is adding a CXL memdev driver binding status inside
>    cxl_reset_done(). (PATCH #6)

Is 2. above reproducible, by inspection, or something else?

> 
> Besides, the patchset also includes a fix for cxlmd->endpoint reset,
> cxlmd->endpoint is set to -ENXIO by default during cxlmd allocation. It
> will be updated when endpoint is allocated and added to the bus.
> However, the CXL driver does not reset it to -ENXIO when the endpoint is
> released. (PATCH #7)
> 
> ---
> Li Ming (7):
>       driver core: Add conditional guard support for device_lock()
>       cxl/memdev: Hold memdev lock during memdev poison injection/clear
>       cxl/region: Hold memdev lock during region poison injection/clear
>       cxl/pci: Hold memdev lock in cxl_event_trace_record()
>       cxl/region: Ensure endpoint is valid in cxl_dpa_to_region()
>       cxl/pci: Check memdev driver binding status in cxl_reset_done()
>       cxl/port: Reset cxlmd->endpoint to -ENXIO by default
> 
>  drivers/cxl/core/core.h   |   4 +-
>  drivers/cxl/core/mbox.c   |   5 ++-
>  drivers/cxl/core/memdev.c |  10 +++++
>  drivers/cxl/core/port.c   |  14 ++++--
>  drivers/cxl/core/region.c | 112 ++++++++++++++++++++++++++++++++++------------
>  drivers/cxl/cxlmem.h      |   2 +-
>  drivers/cxl/pci.c         |   3 ++
>  include/linux/device.h    |   1 +
>  8 files changed, 114 insertions(+), 37 deletions(-)
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260308-fix_access_endpoint_without_drv_check-f2e6ff4bdc48
> 
> Best regards,
> -- 
> Li Ming <ming.li@zohomail.com>
> 

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Alison Schofield]

On Tue, Mar 10, 2026 at 11:57:54PM +0800, Li Ming wrote:
> CXL memdev poison injection/clearing debugfs interfaces are visible
> before the CXL memdev endpoint initialization, If user accesses the
> interfaces before cxlmd->endpoint updated, it is possible to access an
> invalid endpoint in cxl_dpa_to_region().
> 
> Hold CXL memdev lock at the beginning of the interfaces, this blocks the
> interfaces until CXL memdev probing completed.
> 
> The following patch will check the given endpoint validity in
> cxl_dpa_to_region().

'The following patch' makes it sound (to me) like the patch that follows
right here in this email. Probably better to say 'A follow-on patch...'
meaning it follows after this.

Maybe DaveJ can fix that upon applying if this does not rev for
anything else.

I don't see a fixes tag and like I commented in the cover letter, this
is low exposure as the debugfs interfaces for poison are aimed at expert
users, who I'd hope are not reconfiguring their regions while injecting
poison. So I'm good w no fixes tag, if that was your same reasoning.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/core/memdev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 273c22118d3d..8ebaf9e96035 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);
>  
> @@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> @@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);
>  
> @@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> 
> -- 
> 2.43.0
> 

Re: [PATCH 7/7] cxl/port: Reset cxlmd->endpoint to -ENXIO by default

[Dan Williams]

Li Ming wrote:
> cxlmd->endpoint is set to -ENXIO by default, This intentional invalid
> value ensures that any unintended access to the endpoint will be
> detectable. But CXL driver didn't reset it to the default value when the
> endpoint is released in delete_endpoint().
> 
> Besides, cxlmd->endpoint is updated to point to an valid endpoint in
> cxl_port_add(), but if the device_add() in cxl_port_add() fails, the
> endpoint will be released, but cxlmd->endpoint remains pointing to the
> released endpoint, it may introduce a potential use-after-free issue.
> 
> Fixes: 3d8be8b398e3 ("cxl: Set cxlmd->endpoint before adding port device")
> Fixes: 29317f8dc6ed ("cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation")

So I disagree that this is a fix. It may be a consistency change to make
sure that all invalid ->endpoint accesses have the same result, but it
is not a fix for any discernable end user problem.

> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/core/port.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0c5957d1d329..ec3cb62b44b7 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -834,11 +834,14 @@ static int cxl_port_add(struct cxl_port *port,
>  			struct cxl_dport *parent_dport)
>  {
>  	struct device *dev __free(put_device) = &port->dev;
> +	struct cxl_memdev *cxlmd = NULL;
>  	int rc;
>  
>  	if (is_cxl_memdev(port->uport_dev)) {
> -		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> -		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +		struct cxl_dev_state *cxlds;
> +
> +		cxlmd = to_cxl_memdev(port->uport_dev);
> +		cxlds = cxlmd->cxlds;
>  
>  		rc = dev_set_name(dev, "endpoint%d", port->id);
>  		if (rc)
> @@ -865,8 +868,11 @@ static int cxl_port_add(struct cxl_port *port,
>  	}
>  
>  	rc = device_add(dev);
> -	if (rc)
> +	if (rc) {
> +		if (cxlmd)
> +			cxlmd->endpoint = ERR_PTR(-ENXIO);

I see no need for this. All of the upper levels paths ensure that all
the results of setup are torn down on failure. Consider that if
device_add() succeeds and any of the follow on functions in
__devm_cxl_add_port() then this cleanup will also not happen.

So you must rely on upper level cleanup. This is similar to the
semantics of dev_set_drvdata(). That is only reset up after _probe()
finishes failing.



Now, if you want to do some separate cleanups in this space I would redo
the anti-patterns of scope-based cleanup in cxl_port_alloc() and
cxl_port_add(). cxl_port_alloc() does the "__free = NULL" anti-pattern.
It appears to be doing that to avoid adding a DEFINE_FREE() for ida
cleanup if the port allocation fails.

Also, if the cxl_port_alloc() call became something like:

struct cxl_port *port __free(put_cxl_port) = cxl_port_alloc(...)

...then I do not think cxl_port_add() would need to do the odd
__free(put_device()) initialization without a corresponding
get_device(). Nor the odd "dev = NULL" at the end.

>  		return rc;
> +	}
>  
>  	/* Inhibit the cleanup function invoked */
>  	dev = NULL;
> @@ -1425,7 +1431,7 @@ static void delete_endpoint(void *data)
>  			devm_release_action(host, cxl_unlink_uport, endpoint);
>  			devm_release_action(host, unregister_port, endpoint);
>  		}
> -		cxlmd->endpoint = NULL;
> +		cxlmd->endpoint = ERR_PTR(-ENXIO);

I also see no need for this. Either dereferencing NULL or ERR_PTR() will
result in what is needed, a kernel crash. How the use after-free crash
is signalled does not really matter. The only expectation that other
code has is that "->endpoint != NULL" does not mean ->endpoint is valid.
Nothing should care about that though because cxlmd->dev.driver is the
authoritative gate for ->endpoint validity.

Re: [PATCH 6/7] cxl/pci: Check memdev driver binding status in cxl_reset_done()

[Dan Williams]

Li Ming wrote:
> cxl_reset_done() accesses the endpoint of the corresponding CXL memdev
> without endpoint validity checking. By default, cxlmd->endpoint is
> initialized to -ENXIO, if cxl_reset_done() is triggered after the
> corresponding CXL memdev probing failed, this results in access to an
> invalid endpoint.
> 
> CXL subsystem can always check CXL memdev driver binding status to
> confirm its endpoint validity. So adding the CXL memdev driver checking
> inside cxl_reset_done() to avoid accessing an invalid endpoint.
> 
> Fixes: 934edcd436dc ("cxl: Add post-reset warning if reset results in loss of previously committed HDM decoders")
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index fbb300a01830..a5922116db2a 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1043,6 +1043,9 @@ static void cxl_reset_done(struct pci_dev *pdev)
>  	 * that no longer exists.
>  	 */
>  	guard(device)(&cxlmd->dev);
> +	if (!cxlmd->dev.driver)
> +		return;
> +

Looks good,

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

Re: [PATCH 4/7] cxl/pci: Hold memdev lock in cxl_event_trace_record()

[Dan Williams]

Li Ming wrote:
> This is a preparatory patch for the following changes.

It is a preparatory patch that does not stand alone though.

I would squash with the next patch, but you can add to that combo:

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

Re: [PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear

[Dan Williams]

Li Ming wrote:
> cxl_dpa_to_region() will require callers holding the given CXL memdev
> lock for endpoint validity checking in the following patch.

So, the justification for this is not that cxl_dpa_to_region() is going
to start enforcing lock holding, it is that cxl_dpa_to_region() has
expectations that ->endpoint remains valid for the duration of the call.

> To prepare
> it, region poison injection/clearing debugfs interfaces need to ensure
> the correct CXL memdev lock is held at the beginning.

My RFC [1] suggestion was "if there is a problem, here is locking
changes to fix it". That "if there is a problem" is what needed more thought
and time. Before we act on that RFC I want to see some plausible
analysis of how not holding this lock can cause a failure in theory
(better in practice), or how it makes the code hard to reason about.

In other words the justification is not "future patch adds a lockdep
assert so all code paths need to get more complicated". The
justification is identifying a theoretical race window and how the
locking solves it, and how the previous fix [2] did not handle all the
potential races.

[1]: http://lore.kernel.org/69813ac070f79_55fa1005c@dwillia2-mobl4.notmuch
[2]: 0066688dbcdc cxl/port: Hold port host lock during dport adding.

Re: [PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[Dan Williams]

Li Ming wrote:
> Currently, CXL subsystem implementation has some functions that may
> access CXL memdev's endpoint before the endpoint initialization
> completed or without checking the CXL memdev endpoint validity. 
> This patchset fixes three scenarios as above description.
> 
> 1. cxl_dpa_to_region() is possible to access an invalid CXL memdev
>    endpoint.
>    there are two scenarios that can trigger this issue:
>    a. memdev poison injection/clearing debugfs interfaces:
>       devm_cxl_add_endpoint() is used to register CXL memdev endpoint
>       and update cxlmd->endpoint from -ENXIO to the endpoint structure.
>       memdev poison injection/clearing debugfs interfaces are registered
>       before devm_cxl_add_endpoint() is invoked in cxl_mem_probe().
>       There is a small window where user can use the debugfs interfaces
>       to access an invalid endpoint.

This is the justification I wanted to see in the changelog of the
patches themselves. That is a reasonable theoretical window.

>    b. cxl_event_config() in the end of cxl_pci_probe():
>       cxl_event_config() invokes cxl_mem_get_event_record() to get
>       remain event logs from CXL device during cxl_pci_probe(). If CXL
>       memdev probing failed before that, it is also possible to access
>       an invalid endpoint.

Makes sense, please put this in the changelog.

>    To fix these two cases, cxl_dpa_to_region() requires callers holding
>    CXL memdev lock to access it and check if CXL memdev driver bingding
>    status. Holding CXL memdev lock ensures that CXL memdev probing has
>    completed, and if CXL memdev driver is bound, it will mean
>    cxlmd->endpoint is valid. (PATCH #1-#5)
> 
> 2. cxl_reset_done() callback in cxl_pci module.
>    cxl_reset_done() callback also accesses cxlmd->endpoint without any
>    checking. If CXL memdev probing fails, then cxl_reset_done() is
>    called by PCI subsystem, it will access an invalid endpoint. The
>    solution is adding a CXL memdev driver binding status inside
>    cxl_reset_done(). (PATCH #6)

Makes sense. I jumped into the patches first since I was familiar with
the problem space, but happy to see you did this analysis. Just
cover-letter analysis can typically get lost in teh shuffle.

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Dan Williams]

Danilo Krummrich wrote:
[..] 
> As for sharing the commit throughout multiple trees, I can provide a signed tag
> similar to [1] once Greg and Rafael had a chance to take a look at this patch as
> well.

That would be lovely, thank you. Please take this patch.

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

> You may also want to sort out authorship / tags with Li.

No worries about authorship for something so tiny.

Also, I misremembered that I sent the base device_lock() guard as a
standalone patch rather than with the first user.

[1]: 134c6eaa6087 driver core: Add a guard() definition for the device_lock()

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Danilo Krummrich]

On Tue Mar 10, 2026 at 9:37 PM CET, Dan Williams wrote:
> Danilo Krummrich wrote:
> [..] 
>> As for sharing the commit throughout multiple trees, I can provide a signed tag
>> similar to [1] once Greg and Rafael had a chance to take a look at this patch as
>> well.
>
> That would be lovely, thank you. Please take this patch.
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks!

>> You may also want to sort out authorship / tags with Li.
>
> No worries about authorship for something so tiny.

Neither would I, but I'd rather make sure. :)

Re: [PATCH 6/7] cxl/pci: Check memdev driver binding status in cxl_reset_done()

[Dave Jiang]

On 3/10/26 8:57 AM, Li Ming wrote:
> cxl_reset_done() accesses the endpoint of the corresponding CXL memdev
> without endpoint validity checking. By default, cxlmd->endpoint is
> initialized to -ENXIO, if cxl_reset_done() is triggered after the
> corresponding CXL memdev probing failed, this results in access to an
> invalid endpoint.
> 
> CXL subsystem can always check CXL memdev driver binding status to
> confirm its endpoint validity. So adding the CXL memdev driver checking
> inside cxl_reset_done() to avoid accessing an invalid endpoint.
> 
> Fixes: 934edcd436dc ("cxl: Add post-reset warning if reset results in loss of previously committed HDM decoders")
> Signed-off-by: Li Ming <ming.li@zohomail.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index fbb300a01830..a5922116db2a 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1043,6 +1043,9 @@ static void cxl_reset_done(struct pci_dev *pdev)
>  	 * that no longer exists.
>  	 */
>  	guard(device)(&cxlmd->dev);
> +	if (!cxlmd->dev.driver)
> +		return;
> +
>  	if (cxlmd->endpoint &&
>  	    cxl_endpoint_decoder_reset_detected(cxlmd->endpoint)) {
>  		dev_crit(dev, "SBR happened without memory regions removal.\n");
> 

Re: [PATCH 4/7] cxl/pci: Hold memdev lock in cxl_event_trace_record()

[Dave Jiang]

On 3/10/26 8:57 AM, Li Ming wrote:
> This is a preparatory patch for the following changes.
> 
> To enable endpoint validity checks in cxl_dpa_to_region().
> cxl_dpa_to_region() has to require caller to hold CXL memdev lock to
> ensure the CXL memdev probing is completed.
> 
> So holding the given CXL memdev lock before invoking cxl_dpa_to_region()
> in cxl_event_trace_record().
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>

With what Dan said,
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/mbox.c | 5 +++--
>  drivers/cxl/cxlmem.h    | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index e7a6452bf544..3f34bbabf4d3 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -893,7 +893,7 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, "CXL");
>  
> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  			    enum cxl_event_log_type type,
>  			    enum cxl_event_type event_type,
>  			    const uuid_t *uuid, union cxl_event *evt)
> @@ -920,6 +920,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  		 * translations. Take topology mutation locks and lookup
>  		 * { HPA, REGION } from { DPA, MEMDEV } in the event record.
>  		 */
> +		guard(device)(&cxlmd->dev);
>  		guard(rwsem_read)(&cxl_rwsem.region);
>  		guard(rwsem_read)(&cxl_rwsem.dpa);
>  
> @@ -968,7 +969,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, "CXL");
>  
> -static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  				     enum cxl_event_log_type type,
>  				     struct cxl_event_record_raw *record)
>  {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index e21d744d639b..7a34a19c02c8 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -864,7 +864,7 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
>  void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  			    enum cxl_event_log_type type,
>  			    enum cxl_event_type event_type,
>  			    const uuid_t *uuid, union cxl_event *evt);
> 

Re: [PATCH 5/7] cxl/region: Ensure endpoint is valid in cxl_dpa_to_region()

[Dave Jiang]

On 3/10/26 8:57 AM, Li Ming wrote:
> cxl_dpa_to_region() needs to access the endpoint of the given CXL memdev
> to confirm whether the CXL memdev has memory range attached to a CXL
> region. But it is possible to be called before endpoint allocation or
> after the CXL memdev probing failure.  In these two cases,
> cxlmd->endpoint is invalid.
> 
> To solve the problem, cxl_dpa_to_region() requires that callers have to
> hold the given CXL memdev lock, it guarantees the CXL memdev probing has
> completed. And cxl_dpa_to_region() will check the given CXL memdev
> driver binding status, if the memdev has bound to the driver, the
> endpoint is guaranteed to be valid. This checking also requires the CXL
> memdev lock held.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/cxl/core/core.h   |  4 ++--
>  drivers/cxl/core/region.c | 11 +++++++----
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 5b0570df0fd9..fa8402be860f 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -47,7 +47,7 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
>  int cxl_region_init(void);
>  void cxl_region_exit(void);
>  int cxl_get_poison_by_endpoint(struct cxl_port *port);
> -struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> +struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa);
>  u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		   u64 dpa);
>  
> @@ -58,7 +58,7 @@ static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
>  	return ULLONG_MAX;
>  }
>  static inline
> -struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
> +struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	return NULL;
>  }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 806512dfab07..c555e3ae8b62 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2947,16 +2947,19 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>  	return 1;
>  }
>  
> -struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
> +struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	struct cxl_dpa_to_region_context ctx;
> -	struct cxl_port *port;
> +	struct cxl_port *port = cxlmd->endpoint;
> +
> +	device_lock_assert(&cxlmd->dev);
> +	if (!cxlmd->dev.driver)
> +		return NULL;
>  
>  	ctx = (struct cxl_dpa_to_region_context) {
>  		.dpa = dpa,
>  	};
> -	port = cxlmd->endpoint;
> -	if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
> +	if (cxl_num_decoders_committed(port))
>  		device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
>  
>  	return ctx.cxlr;
> 

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Alison Schofield]

On Tue, Mar 10, 2026 at 11:57:54PM +0800, Li Ming wrote:
> CXL memdev poison injection/clearing debugfs interfaces are visible
> before the CXL memdev endpoint initialization, If user accesses the
> interfaces before cxlmd->endpoint updated, it is possible to access an
> invalid endpoint in cxl_dpa_to_region().
> 
> Hold CXL memdev lock at the beginning of the interfaces, this blocks the
> interfaces until CXL memdev probing completed.
> 
> The following patch will check the given endpoint validity in
> cxl_dpa_to_region().
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/core/memdev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 273c22118d3d..8ebaf9e96035 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);

I'm having second thoughts about this since this call site is not
the 'beginning of the interfaces' as the commit msg suggests.

What about taking the device lock in the debugfs func, ie -
mem.c : cxl_inject_poison. If the goal is to avoid using the debugfs
interface before probe completes, that does it.

At this callsite, we make sure nothing changes out from under us,
no endpoints attach or detach during the work.

>  
> @@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> @@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> +	device_lock_assert(&cxlmd->dev);
>  	lockdep_assert_held(&cxl_rwsem.dpa);
>  	lockdep_assert_held(&cxl_rwsem.region);
>  
> @@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	int rc;
>  
> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> 
> -- 
> 2.43.0
> 

Re: [PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear

[Alison Schofield]

On Tue, Mar 10, 2026 at 11:57:55PM +0800, Li Ming wrote:
> cxl_dpa_to_region() will require callers holding the given CXL memdev
> lock for endpoint validity checking in the following patch. To prepare
> it, region poison injection/clearing debugfs interfaces need to ensure
> the correct CXL memdev lock is held at the beginning.
> 
> To keep lock sequence(cxlmd.dev -> cxl_rwsem.region -> cxl_rwsem.dpa)
> for avoiding deadlock. the interfaces have to find out the correct CXL
> memdev at first, holding lock in the sequence then checking if the DPA
> data has been changed before holding locks.

This seems complicated, that this path needs to do needless work to
comply with a new locking scheme that this path doesn't really need.

One thought, if in Patch 2 we lock at the debugfs callsite for
by memdev poison, then here, we don't need to think about following
unnecessary locking protocol. I say that because I think locking
down w dpa and region rwsem is enough here. Then there is nothing
new to do in this path. Might that work?

Along w that can cxl_dpa_to_region() go back to safety checking as
opposed to the new device_lock_assert?

I haven't gone thru every case in this set, so might be way off ;)


> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/core/region.c | 101 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 77 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 42874948b589..806512dfab07 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4074,12 +4074,60 @@ static int validate_region_offset(struct cxl_region *cxlr, u64 offset)
>  	return 0;
>  }
>  
> +static int cxl_region_poison_lookup_locked(struct cxl_region *cxlr, u64 offset,
> +					   struct dpa_result *res)
> +{
> +	int rc;
> +
> +	*res = (struct dpa_result){ .dpa = ULLONG_MAX, .cxlmd = NULL };
> +
> +	if (validate_region_offset(cxlr, offset))
> +		return -EINVAL;
> +
> +	offset -= cxlr->params.cache_size;
> +	rc = region_offset_to_dpa_result(cxlr, offset, res);
> +	if (rc || !res->cxlmd || res->dpa == ULLONG_MAX) {
> +		dev_dbg(&cxlr->dev,
> +			"Failed to resolve DPA for region offset %#llx rc %d\n",
> +			offset, rc);
> +
> +		return rc ? rc : -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_region_poison_lookup(struct cxl_region *cxlr, u64 offset,
> +				    struct dpa_result *res)
> +{
> +	int rc;
> +
> +	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
> +		return rc;
> +
> +	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
> +		return rc;
> +
> +	return cxl_region_poison_lookup_locked(cxlr, offset, res);
> +}
> +
>  static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
>  {
> -	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
>  	struct cxl_region *cxlr = data;
> +	struct dpa_result res1, res2;
>  	int rc;
>  
> +	/* To retrieve the correct memdev */
> +	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
> +	if (rc)
> +		return rc;
> +
> +	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> @@ -4088,20 +4136,18 @@ static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
>  		return rc;
>  
> -	if (validate_region_offset(cxlr, offset))
> -		return -EINVAL;
> -
> -	offset -= cxlr->params.cache_size;
> -	rc = region_offset_to_dpa_result(cxlr, offset, &result);
> -	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
> +	/*
> +	 * Retrieve memdev and DPA data again in case that the data
> +	 * has been changed before holding locks.
> +	 */
> +	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
> +	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
>  		dev_dbg(&cxlr->dev,
> -			"Failed to resolve DPA for region offset %#llx rc %d\n",
> -			offset, rc);
> -
> -		return rc ? rc : -EINVAL;
> +			"Error injection raced region reconfiguration: %d", rc);
> +		return -ENXIO;
>  	}
>  
> -	return cxl_inject_poison_locked(result.cxlmd, result.dpa);
> +	return cxl_inject_poison_locked(res2.cxlmd, res2.dpa);
>  }
>  
>  DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
> @@ -4109,10 +4155,19 @@ DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
>  
>  static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
>  {
> -	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
>  	struct cxl_region *cxlr = data;
> +	struct dpa_result res1, res2;
>  	int rc;
>  
> +	/* To retrieve the correct memdev */
> +	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
> +	if (rc)
> +		return rc;
> +
> +	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> +		return rc;
> +
>  	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>  		return rc;
> @@ -4121,20 +4176,18 @@ static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
>  	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
>  		return rc;
>  
> -	if (validate_region_offset(cxlr, offset))
> -		return -EINVAL;
> -
> -	offset -= cxlr->params.cache_size;
> -	rc = region_offset_to_dpa_result(cxlr, offset, &result);
> -	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
> +	/*
> +	 * Retrieve memdev and DPA data again in case that the data
> +	 * has been changed before holding locks.
> +	 */
> +	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
> +	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
>  		dev_dbg(&cxlr->dev,
> -			"Failed to resolve DPA for region offset %#llx rc %d\n",
> -			offset, rc);
> -
> -		return rc ? rc : -EINVAL;
> +			"Error clearing raced region reconfiguration: %d", rc);
> +		return -ENXIO;
>  	}
>  
> -	return cxl_clear_poison_locked(result.cxlmd, result.dpa);
> +	return cxl_clear_poison_locked(res2.cxlmd, res2.dpa);
>  }
>  
>  DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
> 
> -- 
> 2.43.0
> 

Re: [PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[Li Ming]

在 2026/3/11 03:20, Alison Schofield 写道:
> On Tue, Mar 10, 2026 at 11:57:52PM +0800, Li Ming wrote:
>> Currently, CXL subsystem implementation has some functions that may
>> access CXL memdev's endpoint before the endpoint initialization
>> completed or without checking the CXL memdev endpoint validity.
>> This patchset fixes three scenarios as above description.
> Hi Ming,
>
> I notice you didn't give each patch a fixes tag, I'll comment
> in that per patch.
>
> Below - a couple of questions about reproducing these failures:
Thank you Alison.
>
>> 1. cxl_dpa_to_region() is possible to access an invalid CXL memdev
>>     endpoint.
>>     there are two scenarios that can trigger this issue:
>>     a. memdev poison injection/clearing debugfs interfaces:
>>        devm_cxl_add_endpoint() is used to register CXL memdev endpoint
>>        and update cxlmd->endpoint from -ENXIO to the endpoint structure.
>>        memdev poison injection/clearing debugfs interfaces are registered
>>        before devm_cxl_add_endpoint() is invoked in cxl_mem_probe().
>>        There is a small window where user can use the debugfs interfaces
>>        to access an invalid endpoint.
> Do you have a reproducer for this 1.a above?
> Meson unit test does support parallel execution, so we could create
> a race btw poison inject and region reconfig. It's an interesting task,
> yet low priority considering the debugfs feature is for our expert users.
> If you do have something, please share.

1.a is a theoretical issue, I added a delay between memdev debugfs 
interfaces creating and devm_cxl_add_endpoint() so that I have a chance 
to access the debugfs interfaces before endpoint initialization manually.

Then I can trigger below trace. Maybe use script to write the intetface 
when the interfaces showing up is a choice?


  Oops: general protection fault, probably for non-canonical address 
0xdffffc000000008b: 0000 [#1] SMP KASAN PTI
  KASAN: null-ptr-deref in range [0x0000000000000458-0x000000000000045f]
  CPU: 1 UID: 0 PID: 811 Comm: bash Tainted: G           O 
7.0.0-rc2-debug-dirty #19 PREEMPT(full)
  Tainted: [O]=OOT_MODULE
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
2024.02-2ubuntu0.7 11/27/2025
  RIP: 0010:cxl_dpa_to_region+0xa2/0x130 [cxl_core]
  Call Trace:
   <TASK>
   cxl_inject_poison_locked+0x1d7/0x620 [cxl_core]
   cxl_inject_poison+0x8a/0xe0 [cxl_core]

   ....
   debugfs_attr_write+0x62/0x90

>
>>     b. cxl_event_config() in the end of cxl_pci_probe():
>>        cxl_event_config() invokes cxl_mem_get_event_record() to get
>>        remain event logs from CXL device during cxl_pci_probe(). If CXL
>>        memdev probing failed before that, it is also possible to access
>>        an invalid endpoint.
> b. is reproducible.
> Since your original find of this, I've been thinking about adding a
> new group of tests, like the 'stressors', so that we can do things
> like this on demand, but not as default in the existing cxl suite.
It sounds good!
>
>>     To fix these two cases, cxl_dpa_to_region() requires callers holding
>>     CXL memdev lock to access it and check if CXL memdev driver bingding
>>     status. Holding CXL memdev lock ensures that CXL memdev probing has
>>     completed, and if CXL memdev driver is bound, it will mean
>>     cxlmd->endpoint is valid. (PATCH #1-#5)
>>
>> 2. cxl_reset_done() callback in cxl_pci module.
>>     cxl_reset_done() callback also accesses cxlmd->endpoint without any
>>     checking. If CXL memdev probing fails, then cxl_reset_done() is
>>     called by PCI subsystem, it will access an invalid endpoint. The
>>     solution is adding a CXL memdev driver binding status inside
>>     cxl_reset_done(). (PATCH #6)
> Is 2. above reproducible, by inspection, or something else?

I reproduced it by forcedly letting cxl_mem_probe() fail before 
devm_cxl_add_endpoint(), and manully write the sysfs reset interface of 
the device to trigger it.


Ming

[snip]

Re: [PATCH 0/7] cxl: Consolidate cxlmd->endpoint accessing

[Li Ming]

在 2026/3/11 04:33, Dan Williams 写道:
> Li Ming wrote:
>> Currently, CXL subsystem implementation has some functions that may
>> access CXL memdev's endpoint before the endpoint initialization
>> completed or without checking the CXL memdev endpoint validity.
>> This patchset fixes three scenarios as above description.
>>
>> 1. cxl_dpa_to_region() is possible to access an invalid CXL memdev
>>     endpoint.
>>     there are two scenarios that can trigger this issue:
>>     a. memdev poison injection/clearing debugfs interfaces:
>>        devm_cxl_add_endpoint() is used to register CXL memdev endpoint
>>        and update cxlmd->endpoint from -ENXIO to the endpoint structure.
>>        memdev poison injection/clearing debugfs interfaces are registered
>>        before devm_cxl_add_endpoint() is invoked in cxl_mem_probe().
>>        There is a small window where user can use the debugfs interfaces
>>        to access an invalid endpoint.
> This is the justification I wanted to see in the changelog of the
> patches themselves. That is a reasonable theoretical window.
>
>>     b. cxl_event_config() in the end of cxl_pci_probe():
>>        cxl_event_config() invokes cxl_mem_get_event_record() to get
>>        remain event logs from CXL device during cxl_pci_probe(). If CXL
>>        memdev probing failed before that, it is also possible to access
>>        an invalid endpoint.
> Makes sense, please put this in the changelog.
>
>>     To fix these two cases, cxl_dpa_to_region() requires callers holding
>>     CXL memdev lock to access it and check if CXL memdev driver bingding
>>     status. Holding CXL memdev lock ensures that CXL memdev probing has
>>     completed, and if CXL memdev driver is bound, it will mean
>>     cxlmd->endpoint is valid. (PATCH #1-#5)
>>
>> 2. cxl_reset_done() callback in cxl_pci module.
>>     cxl_reset_done() callback also accesses cxlmd->endpoint without any
>>     checking. If CXL memdev probing fails, then cxl_reset_done() is
>>     called by PCI subsystem, it will access an invalid endpoint. The
>>     solution is adding a CXL memdev driver binding status inside
>>     cxl_reset_done(). (PATCH #6)
> Makes sense. I jumped into the patches first since I was familiar with
> the problem space, but happy to see you did this analysis. Just
> cover-letter analysis can typically get lost in teh shuffle.
>
Thanks for review, Will do you mentioned above.


Ming

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Li Ming]

在 2026/3/11 05:34, Alison Schofield 写道:
> On Tue, Mar 10, 2026 at 11:57:54PM +0800, Li Ming wrote:
>> CXL memdev poison injection/clearing debugfs interfaces are visible
>> before the CXL memdev endpoint initialization, If user accesses the
>> interfaces before cxlmd->endpoint updated, it is possible to access an
>> invalid endpoint in cxl_dpa_to_region().
>>
>> Hold CXL memdev lock at the beginning of the interfaces, this blocks the
>> interfaces until CXL memdev probing completed.
>>
>> The following patch will check the given endpoint validity in
>> cxl_dpa_to_region().
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>   drivers/cxl/core/memdev.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 273c22118d3d..8ebaf9e96035 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>>   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>   		return 0;
>>   
>> +	device_lock_assert(&cxlmd->dev);
>>   	lockdep_assert_held(&cxl_rwsem.dpa);
>>   	lockdep_assert_held(&cxl_rwsem.region);
> I'm having second thoughts about this since this call site is not
> the 'beginning of the interfaces' as the commit msg suggests.
>
> What about taking the device lock in the debugfs func, ie -
> mem.c : cxl_inject_poison. If the goal is to avoid using the debugfs
> interface before probe completes, that does it.
>
> At this callsite, we make sure nothing changes out from under us,
> no endpoints attach or detach during the work.
>
Thanks for taking time to dive into this issue.

But I don't quite understand your comment, do you mean that we don't 
need above device_lock_assert() in cxl_inject/clear_poison_locked()?

You mentioned that taking the device lock in cxl_inject_poison() to 
ensure endpoint won't be changed during the debugfs interfaces calling,

That is right and that is what this patch does. So I am a little bit 
confused.


Ming

>>   
>> @@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>   {
>>   	int rc;
>>   
>> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +		return rc;
>> +
>>   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>>   		return rc;
>> @@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>>   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>   		return 0;
>>   
>> +	device_lock_assert(&cxlmd->dev);
>>   	lockdep_assert_held(&cxl_rwsem.dpa);
>>   	lockdep_assert_held(&cxl_rwsem.region);
>>   
>> @@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>   {
>>   	int rc;
>>   
>> +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +		return rc;
>> +
>>   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>>   		return rc;
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear

[Li Ming]

在 2026/3/11 05:57, Alison Schofield 写道:
> On Tue, Mar 10, 2026 at 11:57:55PM +0800, Li Ming wrote:
>> cxl_dpa_to_region() will require callers holding the given CXL memdev
>> lock for endpoint validity checking in the following patch. To prepare
>> it, region poison injection/clearing debugfs interfaces need to ensure
>> the correct CXL memdev lock is held at the beginning.
>>
>> To keep lock sequence(cxlmd.dev -> cxl_rwsem.region -> cxl_rwsem.dpa)
>> for avoiding deadlock. the interfaces have to find out the correct CXL
>> memdev at first, holding lock in the sequence then checking if the DPA
>> data has been changed before holding locks.
> This seems complicated, that this path needs to do needless work to
> comply with a new locking scheme that this path doesn't really need.
>
> One thought, if in Patch 2 we lock at the debugfs callsite for
> by memdev poison, then here, we don't need to think about following
> unnecessary locking protocol. I say that because I think locking
> down w dpa and region rwsem is enough here. Then there is nothing
> new to do in this path. Might that work?
>
> Along w that can cxl_dpa_to_region() go back to safety checking as
> opposed to the new device_lock_assert?
>
> I haven't gone thru every case in this set, so might be way off ;)

Actually, my understanding is same as yours.

I think we don't need this patch either if we remove the 
device_lock_assert() in cxl_dpa_to_region().(Just keep the 
cxlmd->dev.driver checking)

Because region is created only when endpoint is ready. Detaching an 
endpoint from a region always holds cxl_rwsem.region.

But we always hold device lock for the device driver bingding checking 
in CXL subsystem.

I'm OK to remove this patch, Let see any inputs from other reviewers.


Ming

>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>   drivers/cxl/core/region.c | 101 +++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 77 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 42874948b589..806512dfab07 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -4074,12 +4074,60 @@ static int validate_region_offset(struct cxl_region *cxlr, u64 offset)
>>   	return 0;
>>   }
>>   
>> +static int cxl_region_poison_lookup_locked(struct cxl_region *cxlr, u64 offset,
>> +					   struct dpa_result *res)
>> +{
>> +	int rc;
>> +
>> +	*res = (struct dpa_result){ .dpa = ULLONG_MAX, .cxlmd = NULL };
>> +
>> +	if (validate_region_offset(cxlr, offset))
>> +		return -EINVAL;
>> +
>> +	offset -= cxlr->params.cache_size;
>> +	rc = region_offset_to_dpa_result(cxlr, offset, res);
>> +	if (rc || !res->cxlmd || res->dpa == ULLONG_MAX) {
>> +		dev_dbg(&cxlr->dev,
>> +			"Failed to resolve DPA for region offset %#llx rc %d\n",
>> +			offset, rc);
>> +
>> +		return rc ? rc : -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_region_poison_lookup(struct cxl_region *cxlr, u64 offset,
>> +				    struct dpa_result *res)
>> +{
>> +	int rc;
>> +
>> +	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>> +		return rc;
>> +
>> +	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
>> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
>> +		return rc;
>> +
>> +	return cxl_region_poison_lookup_locked(cxlr, offset, res);
>> +}
>> +
>>   static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
>>   {
>> -	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
>>   	struct cxl_region *cxlr = data;
>> +	struct dpa_result res1, res2;
>>   	int rc;
>>   
>> +	/* To retrieve the correct memdev */
>> +	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
>> +	if (rc)
>> +		return rc;
>> +
>> +	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +		return rc;
>> +
>>   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>>   		return rc;
>> @@ -4088,20 +4136,18 @@ static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
>>   		return rc;
>>   
>> -	if (validate_region_offset(cxlr, offset))
>> -		return -EINVAL;
>> -
>> -	offset -= cxlr->params.cache_size;
>> -	rc = region_offset_to_dpa_result(cxlr, offset, &result);
>> -	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
>> +	/*
>> +	 * Retrieve memdev and DPA data again in case that the data
>> +	 * has been changed before holding locks.
>> +	 */
>> +	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
>> +	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
>>   		dev_dbg(&cxlr->dev,
>> -			"Failed to resolve DPA for region offset %#llx rc %d\n",
>> -			offset, rc);
>> -
>> -		return rc ? rc : -EINVAL;
>> +			"Error injection raced region reconfiguration: %d", rc);
>> +		return -ENXIO;
>>   	}
>>   
>> -	return cxl_inject_poison_locked(result.cxlmd, result.dpa);
>> +	return cxl_inject_poison_locked(res2.cxlmd, res2.dpa);
>>   }
>>   
>>   DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
>> @@ -4109,10 +4155,19 @@ DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
>>   
>>   static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
>>   {
>> -	struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
>>   	struct cxl_region *cxlr = data;
>> +	struct dpa_result res1, res2;
>>   	int rc;
>>   
>> +	/* To retrieve the correct memdev */
>> +	rc = cxl_region_poison_lookup(cxlr, offset, &res1);
>> +	if (rc)
>> +		return rc;
>> +
>> +	ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +		return rc;
>> +
>>   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
>>   		return rc;
>> @@ -4121,20 +4176,18 @@ static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
>>   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
>>   		return rc;
>>   
>> -	if (validate_region_offset(cxlr, offset))
>> -		return -EINVAL;
>> -
>> -	offset -= cxlr->params.cache_size;
>> -	rc = region_offset_to_dpa_result(cxlr, offset, &result);
>> -	if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
>> +	/*
>> +	 * Retrieve memdev and DPA data again in case that the data
>> +	 * has been changed before holding locks.
>> +	 */
>> +	rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
>> +	if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
>>   		dev_dbg(&cxlr->dev,
>> -			"Failed to resolve DPA for region offset %#llx rc %d\n",
>> -			offset, rc);
>> -
>> -		return rc ? rc : -EINVAL;
>> +			"Error clearing raced region reconfiguration: %d", rc);
>> +		return -ENXIO;
>>   	}
>>   
>> -	return cxl_clear_poison_locked(result.cxlmd, result.dpa);
>> +	return cxl_clear_poison_locked(res2.cxlmd, res2.dpa);
>>   }
>>   
>>   DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH 4/7] cxl/pci: Hold memdev lock in cxl_event_trace_record()

[Li Ming]

在 2026/3/11 03:33, Dan Williams 写道:
> Li Ming wrote:
>> This is a preparatory patch for the following changes.
> It is a preparatory patch that does not stand alone though.
>
> I would squash with the next patch, but you can add to that combo:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>
Sure, Will do that in v2. Thanks for review.


Ming

Re: [PATCH 4/7] cxl/pci: Hold memdev lock in cxl_event_trace_record()

[Li Ming]

在 2026/3/11 04:52, Dave Jiang 写道:
>
> On 3/10/26 8:57 AM, Li Ming wrote:
>> This is a preparatory patch for the following changes.
>>
>> To enable endpoint validity checks in cxl_dpa_to_region().
>> cxl_dpa_to_region() has to require caller to hold CXL memdev lock to
>> ensure the CXL memdev probing is completed.
>>
>> So holding the given CXL memdev lock before invoking cxl_dpa_to_region()
>> in cxl_event_trace_record().
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
> With what Dan said,
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Thanks for review.


Ming

[snip]

Re: [PATCH 7/7] cxl/port: Reset cxlmd->endpoint to -ENXIO by default

[Li Ming]

在 2026/3/11 03:29, Dan Williams 写道:
> Li Ming wrote:
>> cxlmd->endpoint is set to -ENXIO by default, This intentional invalid
>> value ensures that any unintended access to the endpoint will be
>> detectable. But CXL driver didn't reset it to the default value when the
>> endpoint is released in delete_endpoint().
>>
>> Besides, cxlmd->endpoint is updated to point to an valid endpoint in
>> cxl_port_add(), but if the device_add() in cxl_port_add() fails, the
>> endpoint will be released, but cxlmd->endpoint remains pointing to the
>> released endpoint, it may introduce a potential use-after-free issue.
>>
>> Fixes: 3d8be8b398e3 ("cxl: Set cxlmd->endpoint before adding port device")
>> Fixes: 29317f8dc6ed ("cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation")
> So I disagree that this is a fix. It may be a consistency change to make
> sure that all invalid ->endpoint accesses have the same result, but it
> is not a fix for any discernable end user problem.
Will change it in V2.
>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>   drivers/cxl/core/port.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 0c5957d1d329..ec3cb62b44b7 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -834,11 +834,14 @@ static int cxl_port_add(struct cxl_port *port,
>>   			struct cxl_dport *parent_dport)
>>   {
>>   	struct device *dev __free(put_device) = &port->dev;
>> +	struct cxl_memdev *cxlmd = NULL;
>>   	int rc;
>>   
>>   	if (is_cxl_memdev(port->uport_dev)) {
>> -		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>> -		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +		struct cxl_dev_state *cxlds;
>> +
>> +		cxlmd = to_cxl_memdev(port->uport_dev);
>> +		cxlds = cxlmd->cxlds;
>>   
>>   		rc = dev_set_name(dev, "endpoint%d", port->id);
>>   		if (rc)
>> @@ -865,8 +868,11 @@ static int cxl_port_add(struct cxl_port *port,
>>   	}
>>   
>>   	rc = device_add(dev);
>> -	if (rc)
>> +	if (rc) {
>> +		if (cxlmd)
>> +			cxlmd->endpoint = ERR_PTR(-ENXIO);
> I see no need for this. All of the upper levels paths ensure that all
> the results of setup are torn down on failure. Consider that if
> device_add() succeeds and any of the follow on functions in
> __devm_cxl_add_port() then this cleanup will also not happen.
>
> So you must rely on upper level cleanup. This is similar to the
> semantics of dev_set_drvdata(). That is only reset up after _probe()
> finishes failing.
>
>
>
> Now, if you want to do some separate cleanups in this space I would redo
> the anti-patterns of scope-based cleanup in cxl_port_alloc() and
> cxl_port_add(). cxl_port_alloc() does the "__free = NULL" anti-pattern.
> It appears to be doing that to avoid adding a DEFINE_FREE() for ida
> cleanup if the port allocation fails.
>
> Also, if the cxl_port_alloc() call became something like:
>
> struct cxl_port *port __free(put_cxl_port) = cxl_port_alloc(...)
>
> ...then I do not think cxl_port_add() would need to do the odd
> __free(put_device()) initialization without a corresponding
> get_device(). Nor the odd "dev = NULL" at the end.

To make sure that I didn't misunderstand your comment, I draft the 
following changes.

The changes also fix an issue that the unregister_port() was not invoked 
to release port if devm_cxl_link_uport() or devm_cxl_link_parent_dport() 
failed in __devm_cxl_add_port() .

If the changes are OK, I think they should be in a separate 
patchset(including reset cxlmd->endpoint to -ENXIO if adding endpoint 
failed)


diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0c5957d1d329..c1975b28b570 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -552,10 +552,13 @@ static void cxl_port_release(struct device *dev)
      xa_destroy(&port->dports);
      xa_destroy(&port->regions);
      ida_free(&cxl_port_ida, port->id);
-    if (is_cxl_root(port))
+    if (is_cxl_root(port)) {
          kfree(to_cxl_root(port));
-    else
+    } else {
+        if (is_cxl_endpoint(port))
+            to_cxl_memdev(&port->dev)->endpoint = ERR_PTR(-ENXIO);
          kfree(port);
+    }
  }

  static ssize_t decoders_committed_show(struct device *dev,
@@ -833,7 +836,7 @@ static int cxl_port_add(struct cxl_port *port,
              resource_size_t component_reg_phys,
              struct cxl_dport *parent_dport)
  {
-    struct device *dev __free(put_device) = &port->dev;
+    struct device *dev = &port->dev;
      int rc;

      if (is_cxl_memdev(port->uport_dev)) {
@@ -864,47 +867,53 @@ static int cxl_port_add(struct cxl_port *port,
              return rc;
      }

-    rc = device_add(dev);
-    if (rc)
-        return rc;
-
-    /* Inhibit the cleanup function invoked */
-    dev = NULL;
-    return 0;
+    return device_add(dev);
  }

+DEFINE_FREE(cxl_port_host_release_dr_group, struct cxl_port *,
+        if (_T) devres_release_group(port_to_host(_T), _T))
+
  static struct cxl_port *__devm_cxl_add_port(struct device *host,
                          struct device *uport_dev,
                          resource_size_t component_reg_phys,
                          struct cxl_dport *parent_dport)
  {
-    struct cxl_port *port;
+    struct cxl_port *_port;
      int rc;

-    port = cxl_port_alloc(uport_dev, parent_dport);
+    struct cxl_port *port __free(put_cxl_port) =
+        cxl_port_alloc(uport_dev, parent_dport);
      if (IS_ERR(port))
          return port;

+    void *port_dr_group __free(cxl_port_host_release_dr_group) =
+        devres_open_group(host, port, GFP_KERNEL);
+    if (!port_dr_group)
+        return ERR_PTR(-ENOMEM);
+
      rc = cxl_port_add(port, component_reg_phys, parent_dport);
      if (rc)
          return ERR_PTR(rc);

-    rc = devm_add_action_or_reset(host, unregister_port, port);
+    _port = no_free_ptr(port);
+    rc = devm_add_action_or_reset(host, unregister_port, _port);
      if (rc)
          return ERR_PTR(rc);

-    rc = devm_cxl_link_uport(host, port);
+    rc = devm_cxl_link_uport(host, _port);
      if (rc)
          return ERR_PTR(rc);

-    rc = devm_cxl_link_parent_dport(host, port, parent_dport);
+    rc = devm_cxl_link_parent_dport(host, _port, parent_dport);
      if (rc)
          return ERR_PTR(rc);

+    devres_remove_group(host, no_free_ptr(port_dr_group));
+
      if (parent_dport && dev_is_pci(uport_dev))
-        port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));
+        _port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));

-    return port;
+    return _port;
  }

>>   		return rc;
>> +	}
>>   
>>   	/* Inhibit the cleanup function invoked */
>>   	dev = NULL;
>> @@ -1425,7 +1431,7 @@ static void delete_endpoint(void *data)
>>   			devm_release_action(host, cxl_unlink_uport, endpoint);
>>   			devm_release_action(host, unregister_port, endpoint);
>>   		}
>> -		cxlmd->endpoint = NULL;
>> +		cxlmd->endpoint = ERR_PTR(-ENXIO);
> I also see no need for this. Either dereferencing NULL or ERR_PTR() will
> result in what is needed, a kernel crash. How the use after-free crash
> is signalled does not really matter. The only expectation that other
> code has is that "->endpoint != NULL" does not mean ->endpoint is valid.
> Nothing should care about that though because cxlmd->dev.driver is the
> authoritative gate for ->endpoint validity.
>
OK, will remove it in v2.

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Alison Schofield]

On Wed, Mar 11, 2026 at 06:53:26PM +0800, Li Ming wrote:
> 
> 在 2026/3/11 05:34, Alison Schofield 写道:
> > On Tue, Mar 10, 2026 at 11:57:54PM +0800, Li Ming wrote:
> > > CXL memdev poison injection/clearing debugfs interfaces are visible
> > > before the CXL memdev endpoint initialization, If user accesses the
> > > interfaces before cxlmd->endpoint updated, it is possible to access an
> > > invalid endpoint in cxl_dpa_to_region().
> > > 
> > > Hold CXL memdev lock at the beginning of the interfaces, this blocks the
> > > interfaces until CXL memdev probing completed.
> > > 
> > > The following patch will check the given endpoint validity in
> > > cxl_dpa_to_region().
> > > 
> > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Li Ming <ming.li@zohomail.com>
> > > ---
> > >   drivers/cxl/core/memdev.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > > index 273c22118d3d..8ebaf9e96035 100644
> > > --- a/drivers/cxl/core/memdev.c
> > > +++ b/drivers/cxl/core/memdev.c
> > > @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
> > >   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
> > >   		return 0;
> > > +	device_lock_assert(&cxlmd->dev);
> > >   	lockdep_assert_held(&cxl_rwsem.dpa);
> > >   	lockdep_assert_held(&cxl_rwsem.region);
> > I'm having second thoughts about this since this call site is not
> > the 'beginning of the interfaces' as the commit msg suggests.
> > 
> > What about taking the device lock in the debugfs func, ie -
> > mem.c : cxl_inject_poison. If the goal is to avoid using the debugfs
> > interface before probe completes, that does it.
> > 
> > At this callsite, we make sure nothing changes out from under us,
> > no endpoints attach or detach during the work.
> > 
> Thanks for taking time to dive into this issue.
> 
> But I don't quite understand your comment, do you mean that we don't need
> above device_lock_assert() in cxl_inject/clear_poison_locked()?
> 
> You mentioned that taking the device lock in cxl_inject_poison() to ensure
> endpoint won't be changed during the debugfs interfaces calling,
> 
> That is right and that is what this patch does. So I am a little bit
> confused.

I was only thinking of moving the ACQUIRE one level up, to here:
drivers/cxl/mem.c: cxl_debugfs_poison_inject|clear ()

That would mean dropping the assert in clear_poison_locked().

> 
> 
> Ming
> 
> > > @@ -331,6 +332,10 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> > >   {
> > >   	int rc;
> > > +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> > > +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> > > +		return rc;
> > > +
> > >   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
> > >   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
> > >   		return rc;
> > > @@ -355,6 +360,7 @@ int cxl_clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
> > >   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
> > >   		return 0;
> > > +	device_lock_assert(&cxlmd->dev);
> > >   	lockdep_assert_held(&cxl_rwsem.dpa);
> > >   	lockdep_assert_held(&cxl_rwsem.region);
> > > @@ -400,6 +406,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
> > >   {
> > >   	int rc;
> > > +	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
> > > +	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
> > > +		return rc;
> > > +
> > >   	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
> > >   	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
> > >   		return rc;
> > > 
> > > -- 
> > > 2.43.0
> > > 

Re: [PATCH 2/7] cxl/memdev: Hold memdev lock during memdev poison injection/clear

[Li Ming]

在 2026/3/12 12:05, Alison Schofield 写道:
> On Wed, Mar 11, 2026 at 06:53:26PM +0800, Li Ming wrote:
>> 在 2026/3/11 05:34, Alison Schofield 写道:
>>> On Tue, Mar 10, 2026 at 11:57:54PM +0800, Li Ming wrote:
>>>> CXL memdev poison injection/clearing debugfs interfaces are visible
>>>> before the CXL memdev endpoint initialization, If user accesses the
>>>> interfaces before cxlmd->endpoint updated, it is possible to access an
>>>> invalid endpoint in cxl_dpa_to_region().
>>>>
>>>> Hold CXL memdev lock at the beginning of the interfaces, this blocks the
>>>> interfaces until CXL memdev probing completed.
>>>>
>>>> The following patch will check the given endpoint validity in
>>>> cxl_dpa_to_region().
>>>>
>>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>>>> ---
>>>>    drivers/cxl/core/memdev.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>>> index 273c22118d3d..8ebaf9e96035 100644
>>>> --- a/drivers/cxl/core/memdev.c
>>>> +++ b/drivers/cxl/core/memdev.c
>>>> @@ -295,6 +295,7 @@ int cxl_inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>>>>    	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>>>    		return 0;
>>>> +	device_lock_assert(&cxlmd->dev);
>>>>    	lockdep_assert_held(&cxl_rwsem.dpa);
>>>>    	lockdep_assert_held(&cxl_rwsem.region);
>>> I'm having second thoughts about this since this call site is not
>>> the 'beginning of the interfaces' as the commit msg suggests.
>>>
>>> What about taking the device lock in the debugfs func, ie -
>>> mem.c : cxl_inject_poison. If the goal is to avoid using the debugfs
>>> interface before probe completes, that does it.
>>>
>>> At this callsite, we make sure nothing changes out from under us,
>>> no endpoints attach or detach during the work.
>>>
>> Thanks for taking time to dive into this issue.
>>
>> But I don't quite understand your comment, do you mean that we don't need
>> above device_lock_assert() in cxl_inject/clear_poison_locked()?
>>
>> You mentioned that taking the device lock in cxl_inject_poison() to ensure
>> endpoint won't be changed during the debugfs interfaces calling,
>>
>> That is right and that is what this patch does. So I am a little bit
>> confused.
> I was only thinking of moving the ACQUIRE one level up, to here:
> drivers/cxl/mem.c: cxl_debugfs_poison_inject|clear ()
>
> That would mean dropping the assert in clear_poison_locked().

Oh, got it, will do that in v2, thanks for your explaination.


Ming

[snip]

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Greg Kroah-Hartman]

On Tue, Mar 10, 2026 at 11:57:53PM +0800, Li Ming wrote:
> Introduce conditional guard version of device_lock() for scenarios that
> require conditional device lock holding.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  include/linux/device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0be95294b6e6..4fafee80524b 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -911,6 +911,7 @@ static inline void device_unlock(struct device *dev)
>  }
>  
>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
> +DEFINE_GUARD_COND(device, _intr, device_lock_interruptible(_T), _RET == 0)
>  
>  static inline void device_lock_assert(struct device *dev)
>  {
> 
> -- 
> 2.43.0
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/7] driver core: Add conditional guard support for device_lock()

[Danilo Krummrich]

On Tue Mar 10, 2026 at 4:57 PM CET, Li Ming wrote:
> Introduce conditional guard version of device_lock() for scenarios that
> require conditional device lock holding.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>

Applied to driver-core-next, thanks!

As dicussed, here's the signed tag:

https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tag/?h=device_lock_cond_guard-7.1-rc1

Thanks,
Danilo

Re: [PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear

[Dan Williams]

Li Ming wrote:
> 
> 在 2026/3/11 05:57, Alison Schofield 写道:
> > On Tue, Mar 10, 2026 at 11:57:55PM +0800, Li Ming wrote:
> >> cxl_dpa_to_region() will require callers holding the given CXL memdev
> >> lock for endpoint validity checking in the following patch. To prepare
> >> it, region poison injection/clearing debugfs interfaces need to ensure
> >> the correct CXL memdev lock is held at the beginning.
> >>
> >> To keep lock sequence(cxlmd.dev -> cxl_rwsem.region -> cxl_rwsem.dpa)
> >> for avoiding deadlock. the interfaces have to find out the correct CXL
> >> memdev at first, holding lock in the sequence then checking if the DPA
> >> data has been changed before holding locks.
> > This seems complicated, that this path needs to do needless work to
> > comply with a new locking scheme that this path doesn't really need.
> >
> > One thought, if in Patch 2 we lock at the debugfs callsite for
> > by memdev poison, then here, we don't need to think about following
> > unnecessary locking protocol. I say that because I think locking
> > down w dpa and region rwsem is enough here. Then there is nothing
> > new to do in this path. Might that work?
> >
> > Along w that can cxl_dpa_to_region() go back to safety checking as
> > opposed to the new device_lock_assert?
> >
> > I haven't gone thru every case in this set, so might be way off ;)
> 
> Actually, my understanding is same as yours.
> 
> I think we don't need this patch either if we remove the 
> device_lock_assert() in cxl_dpa_to_region().(Just keep the 
> cxlmd->dev.driver checking)
> 
> Because region is created only when endpoint is ready. Detaching an 
> endpoint from a region always holds cxl_rwsem.region.
> 
> But we always hold device lock for the device driver bingding checking 
> in CXL subsystem.
> 
> I'm OK to remove this patch, Let see any inputs from other reviewers.

The question is can cxlmd->dev.driver be invalidated while userspace is
pending inside of a debugfs callback?

Yes, it can. Debugfs, unlike sysfs which uses kernfs_drain(), does not
flush active threads inside debugfs handlers. At least not as far as I
can see.

The next question is, are we protected by the fact that a region needs
to disconnect a decoder and it only does that under the rwsem held for
write?

I think the answer is "no".

If the protocol was to have a region in hand and then validate its
locked association to an endpoint decoder then, yes. In this case though
we have no idea if the cxlmd is associated with any region at all.  So I
think you can theoretically race injecting an error to an idle cxlmd
that is in the process of destroying its ->endpoint.

Now, maybe some other detail saves us, but I do not think it is worth
the mental gymnastics of just requiring that no driver detach
shenanigans are running concurrent with poison injection shenanigans.