[PATCH v5 0/3] cxl/edac: Fix event record lookups for memory repair

[PATCH v5 0/3] cxl/edac: Fix event record lookups for memory repair

[Jongwon Lee]

This series fixes how the memory repair path looks up cached event
records.

Patch 1 fixes a use-after-free between the repair path and the event
path. Sashiko's review of v2 found that the repair path reads cached
DRAM event records without any lock that stops the event path from
freeing them [1]. This race was already there in cxl_find_rec_dram()
and cxl_mem_perform_sparing(). Patch 1 does the DRAM lookup under the
xarray lock, and copies the record out for the sparing path.

Patch 2 makes both lookups skip records that the device marked
NOT_REPAIRABLE. Since the General Media lookup now reads the record, it
also takes the xarray lock.

Patch 3 is the DPA flag masking fix from the earlier versions. It has to
come after patch 2. With the masked key, the DPA from the trace event
matches a NOT_REPAIRABLE record, so masking alone would let such a
record be repaired.

I used xa_lock() for the race. kfree_rcu() would need a wrapper struct
with an rcu_head, since the cached records are copies of the event
structures from the spec. The lookups only compare a few fields, so the
lock is held for a short time. Taking device_lock() in the repair path,
like the event path does, looked like it could deadlock with unbind.
Unbind removes the EDAC sysfs files while holding device_lock().

Changes in v5:
- New patch 2: the NOT_REPAIRABLE check, split out of v4 patch 2 and
  placed before the masking patch (Li Ming)
- Patch 3: rename cxl_rec_key() to cxl_rec_dpa() (Li Ming)
- Patch 1: drop the found variable in cxl_find_rec_dram() (Li Ming).
  It now returns rec. I said rec != NULL in my reply, but checkpatch
  --strict warns about that.
- No message is printed when a NOT_REPAIRABLE record is skipped. The
  trace events already show dpa_flags='NOT_REPAIRABLE' for it. I can
  add a dev_dbg() there if that is preferred.
- Richard's Reviewed-by on v4 2/2 is not carried over to patches 2 and
  3, since that patch was split.
- Rebased on cxl/next (e44dc3a303), no conflicts.

Changes in v4:
- Patch 1: copy the whole struct cxl_event_dram out instead of only
  validity_flags (Richard Cheng)
- Patch 2: make cxl_rec_key() and cxl_rec_not_repairable() static
  inline (Richard Cheng)
- Cover letter: describe patch 1 first (Richard Cheng)

Changes in v3:
- New patch 1 for the record lifetime race (Sashiko)
- Patch 2: General Media lookup under the xarray lock, returns bool

Changes in v2:
- Moved the Claude Code note below the --- (Dave Jiang)

v4: https://lore.kernel.org/r/20261102123233.74598-1-ai@linux.com
v3: https://lore.kernel.org/r/20261101013418.87326-1-ai@linux.com
v2: https://lore.kernel.org/r/20261031161121.15948-1-ai@linux.com
v1: https://lore.kernel.org/r/20261030072721.31970-1-ai@linux.com

Testing was done in QEMU master (b574eaf) with a CXL type 3 device and
a committed region, on top of d89e1d186e. Debug options: KASAN,
PROVE_LOCKING, PROVE_RAW_LOCK_NESTING, PROVE_RCU, DEBUG_ATOMIC_SLEEP,
DEBUG_LIST, UBSAN_BOUNDS, KMEMLEAK, hung task and soft lockup detection,
booted with preempt=full. The unpatched results are from the v4 runs on
the same base. Not tested on real CXL hardware.

- Repair table: DRAM events at 0x1001, 0x2000 and 0x3002 and General
  Media events at 0x5001 and 0x6002, then a repair request on all five
  mem_repair devices for eight DPAs. With the series, 0x1000 and 0x2000
  succeed for all five repair types, and 0x5000 succeeds for PPR only.
  0x3000 and 0x6000 (NOT_REPAIRABLE set), 0x7000 (no record), 0x1001
  and 0x3002 fail with -EINVAL, same as v4. With only patches 1 and 2,
  0x3002 is rejected, 0x1001 is accepted, and 0x1000 and the PPR
  request at 0x5000 still fail. Unpatched, 0x1000 and the PPR request
  at 0x5000 fail, and 0x1001 and 0x3002 are accepted.
- Race test with a test-only mdelay(20): the host injects DRAM and
  General Media events back to back. Some DPAs repeat, so xa_store()
  replaces and frees records, and there are enough distinct DPAs to hit
  the overflow path. Four guest loops request repairs on all five
  devices at the same time. Unpatched, with the delay after xa_load()
  and after the lookup in the sparing path, KASAN reported a
  slab-use-after-free in cxl_do_mem_sparing() after 120 s. With v5 and
  the delay after both xa_load() calls inside the locked section and
  after the lookup in the sparing path: 600 s, 10650 repair requests,
  241298 events, no report.
- The same race test without the delay: v5 ran for 1800 s, 67830
  repair requests and 988817 events, no report.
- Unbind test: two repair loops and the same event injection for 600 s,
  while another loop unbinds cxl_mem, waits 2 s, binds it again and
  waits 3 s. v5: 82 unbind/bind cycles, 8838 repair requests, no
  report, and mem0 was still bound at the end.
- kmemleak scan at the end of each run: no unreferenced objects.
- Builds: the x86_64 test kernel with gcc. For drivers/cxl/core/edac.o:
  W=1 after each patch, sparse, clang W=1 for x86_64 and arm64, and
  CONFIG_CXL_EDAC_MEM_REPAIR=n, with no new warnings. The series
  applies to cxl/next (e44dc3a303) and edac.o builds there after each
  patch with W=1 and no new warnings.

This series was developed with Claude Code.

[1] https://lore.kernel.org/r/sashiko-outbox-192042@kernel.org

Jongwon Lee (3):
  cxl/edac: Read cached DRAM event records under the xarray lock
  cxl/edac: Skip event records marked NOT_REPAIRABLE
  cxl/edac: Mask DPA flags in the event record cache key

 drivers/cxl/core/edac.c | 113 ++++++++++++++++++++++++++--------------
 1 file changed, 74 insertions(+), 39 deletions(-)


base-commit: e44dc3a3037346354ac0dcd70415a7e049fab059
-- 
2.55.0

[PATCH v5 1/3] cxl/edac: Read cached DRAM event records under the xarray lock

[Jongwon Lee]

The event path caches DRAM event records in an xarray. It frees a record
when xa_store() replaces it, when the record expires, and when the cache
overflows. The event path runs with device_lock() and cxl_rwsem held for
read.

The repair path looks up records with cxl_find_rec_dram() while holding
only cxl_rwsem for read, so both paths can run at the same time.
cxl_find_rec_dram() reads the record after xa_load() returns, and
cxl_mem_perform_sparing() reads validity_flags from the returned record
again later. Both reads can hit a record that the event path has just
freed.

Move the lookup into __cxl_find_rec_dram(), which is called with the
xarray lock held. cxl_find_rec_dram() now returns whether a record
matched, and copies the matching record into a buffer passed in by the
caller. The PPR path only needs to know whether there is a match, so it
passes NULL. No caller keeps a pointer to the record after the lock is
dropped. The event path frees a record only after xa_store() or
xa_erase() has removed it from the xarray, and both of them take the
same lock.

Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
Fixes: 588ca944c277 ("cxl/edac: Add CXL memory device memory sparing control feature")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-192042@kernel.org
Reviewed-by: Richard Cheng <icheng@nvidia.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Signed-off-by: Jongwon Lee <ai@linux.com>
---
 drivers/cxl/core/edac.c | 70 +++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 34b81e8dfb..4624ab4644 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -938,18 +938,17 @@ cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
 	return NULL;
 }
 
+/* Caller holds the xarray lock. The record may be freed once it is dropped. */
 static struct cxl_event_dram *
-cxl_find_rec_dram(struct cxl_memdev *cxlmd,
-		  struct cxl_mem_repair_attrbs *attrbs)
+__cxl_find_rec_dram(struct xarray *rec_xarray,
+		    struct cxl_mem_repair_attrbs *attrbs)
 {
-	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
 	struct cxl_event_dram *rec;
 	u16 validity_flags;
 
-	if (!array_rec)
-		return NULL;
+	lockdep_assert_held(&rec_xarray->xa_lock);
 
-	rec = xa_load(&array_rec->rec_dram, attrbs->dpa);
+	rec = xa_load(rec_xarray, attrbs->dpa);
 	if (!rec)
 		return NULL;
 
@@ -1029,6 +1028,30 @@ cxl_find_rec_dram(struct cxl_memdev *cxlmd,
 	return NULL;
 }
 
+/*
+ * cxl_store_rec_dram() replaces and frees records while the repair path only
+ * holds cxl_rwsem for read, so look the record up and read it under the
+ * xarray lock, and hand back a copy of the record.
+ */
+static bool cxl_find_rec_dram(struct cxl_memdev *cxlmd,
+			      struct cxl_mem_repair_attrbs *attrbs,
+			      struct cxl_event_dram *out)
+{
+	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
+	struct cxl_event_dram *rec;
+
+	if (!array_rec)
+		return false;
+
+	xa_lock(&array_rec->rec_dram);
+	rec = __cxl_find_rec_dram(&array_rec->rec_dram, attrbs);
+	if (rec && out)
+		*out = *rec;
+	xa_unlock(&array_rec->rec_dram);
+
+	return rec;
+}
+
 #define CXL_MAX_STORAGE_DAYS 10
 #define CXL_MAX_STORAGE_TIME_SECS (CXL_MAX_STORAGE_DAYS * 24 * 60 * 60)
 
@@ -1298,9 +1321,9 @@ cxl_mem_sparing_get_attrbs(struct cxl_mem_sparing_context *cxl_sparing_ctx)
 	return 0;
 }
 
-static struct cxl_event_dram *
-cxl_mem_get_rec_dram(struct cxl_memdev *cxlmd,
-		     struct cxl_mem_sparing_context *ctx)
+static bool cxl_mem_get_rec_dram(struct cxl_memdev *cxlmd,
+				 struct cxl_mem_sparing_context *ctx,
+				 struct cxl_event_dram *rec)
 {
 	struct cxl_mem_repair_attrbs attrbs = { 0 };
 
@@ -1332,10 +1355,10 @@ cxl_mem_get_rec_dram(struct cxl_memdev *cxlmd,
 		attrbs.repair_type = CXL_RANK_SPARING;
 		break;
 	default:
-		return NULL;
+		return false;
 	}
 
-	return cxl_find_rec_dram(cxlmd, &attrbs);
+	return cxl_find_rec_dram(cxlmd, &attrbs, rec);
 }
 
 static int
@@ -1344,7 +1367,8 @@ cxl_mem_perform_sparing(struct device *dev,
 {
 	struct cxl_memdev *cxlmd = cxl_sparing_ctx->cxlmd;
 	struct cxl_memdev_sparing_in_payload sparing_pi;
-	struct cxl_event_dram *rec = NULL;
+	struct cxl_event_dram rec;
+	bool rec_found = false;
 	u16 validity_flags = 0;
 	int ret;
 
@@ -1362,11 +1386,14 @@ cxl_mem_perform_sparing(struct device *dev,
 			return -EBUSY;
 	} else {
 		if (cxl_is_memdev_memory_online(cxlmd)) {
-			rec = cxl_mem_get_rec_dram(cxlmd, cxl_sparing_ctx);
-			if (!rec)
+			rec_found = cxl_mem_get_rec_dram(cxlmd, cxl_sparing_ctx,
+							 &rec);
+			if (!rec_found)
 				return -EINVAL;
 
-			if (!get_unaligned_le16(rec->media_hdr.validity_flags))
+			validity_flags =
+				get_unaligned_le16(rec.media_hdr.validity_flags);
+			if (!validity_flags)
 				return -EINVAL;
 		}
 	}
@@ -1376,13 +1403,10 @@ cxl_mem_perform_sparing(struct device *dev,
 	if (cxl_sparing_ctx->persist_mode)
 		sparing_pi.flags |= CXL_SET_HARD_SPARING(1);
 
-	if (rec)
-		validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
-
 	switch (cxl_sparing_ctx->repair_type) {
 	case EDAC_REPAIR_CACHELINE_SPARING:
 		sparing_pi.column = cpu_to_le16(cxl_sparing_ctx->column);
-		if (!rec || (validity_flags & CXL_DER_VALID_SUB_CHANNEL)) {
+		if (!rec_found || (validity_flags & CXL_DER_VALID_SUB_CHANNEL)) {
 			sparing_pi.flags |= CXL_SET_SPARING_SUB_CHNL_VALID(1);
 			sparing_pi.sub_channel = cxl_sparing_ctx->sub_channel;
 		}
@@ -1399,9 +1423,9 @@ cxl_mem_perform_sparing(struct device *dev,
 		fallthrough;
 	default:
 		sparing_pi.channel = cxl_sparing_ctx->channel;
-		if ((rec && (validity_flags & CXL_DER_VALID_NIBBLE)) ||
-		    (!rec && (!cxl_sparing_ctx->nibble_mask ||
-			     (cxl_sparing_ctx->nibble_mask & 0xFFFFFF)))) {
+		if ((rec_found && (validity_flags & CXL_DER_VALID_NIBBLE)) ||
+		    (!rec_found && (!cxl_sparing_ctx->nibble_mask ||
+				    (cxl_sparing_ctx->nibble_mask & 0xFFFFFF)))) {
 			sparing_pi.flags |= CXL_SET_SPARING_NIB_MASK_VALID(1);
 			put_unaligned_le24(cxl_sparing_ctx->nibble_mask,
 					   sparing_pi.nibble_mask);
@@ -1814,7 +1838,7 @@ static int cxl_mem_perform_ppr(struct cxl_ppr_context *cxl_ppr_ctx)
 			attrbs.repair_type = CXL_PPR;
 			attrbs.dpa = cxl_ppr_ctx->dpa;
 			attrbs.nibble_mask = cxl_ppr_ctx->nibble_mask;
-			if (!cxl_find_rec_dram(cxlmd, &attrbs) &&
+			if (!cxl_find_rec_dram(cxlmd, &attrbs, NULL) &&
 			    !cxl_find_rec_gen_media(cxlmd, &attrbs))
 				return -EINVAL;
 		}
-- 
2.55.0

[PATCH v5 2/3] cxl/edac: Skip event records marked NOT_REPAIRABLE

[Jongwon Lee]

Bit 1 of the DPA field in General Media and DRAM event records is the
NOT_REPAIRABLE flag. The device sets it when the memory at that DPA
cannot be repaired.

cxl_find_rec_gen_media() and cxl_find_rec_dram() do not check this flag.
A repair request that matches such a record passes the lookup and is
sent to the device. Since the records are keyed by the raw DPA, this
happens today when user space writes the DPA with bit 1 set.

Skip these records in both lookups, so the repair request fails with
-EINVAL. The trace events already show dpa_flags='NOT_REPAIRABLE' for
them, so user space can tell before it asks for a repair.

cxl_find_rec_gen_media() now reads the record, so take the xarray lock
there too, as cxl_find_rec_dram() does. Also make it return bool, since
its only caller just checks the result.

Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
Signed-off-by: Jongwon Lee <ai@linux.com>
---
 drivers/cxl/core/edac.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 4624ab4644..42683b7c54 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -918,24 +918,27 @@ struct cxl_mem_repair_attrbs {
 	enum cxl_mem_repair_type repair_type;
 };
 
-static struct cxl_event_gen_media *
-cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
-		       struct cxl_mem_repair_attrbs *attrbs)
+static inline bool cxl_rec_not_repairable(const struct cxl_event_media_hdr *hdr)
+{
+	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_NOT_REPAIRABLE;
+}
+
+static bool cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
+				   struct cxl_mem_repair_attrbs *attrbs)
 {
 	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
 	struct cxl_event_gen_media *rec;
+	bool found;
 
-	if (!array_rec)
-		return NULL;
+	if (!array_rec || attrbs->repair_type != CXL_PPR)
+		return false;
 
+	xa_lock(&array_rec->rec_gen_media);
 	rec = xa_load(&array_rec->rec_gen_media, attrbs->dpa);
-	if (!rec)
-		return NULL;
-
-	if (attrbs->repair_type == CXL_PPR)
-		return rec;
+	found = rec && !cxl_rec_not_repairable(&rec->media_hdr);
+	xa_unlock(&array_rec->rec_gen_media);
 
-	return NULL;
+	return found;
 }
 
 /* Caller holds the xarray lock. The record may be freed once it is dropped. */
@@ -949,7 +952,7 @@ __cxl_find_rec_dram(struct xarray *rec_xarray,
 	lockdep_assert_held(&rec_xarray->xa_lock);
 
 	rec = xa_load(rec_xarray, attrbs->dpa);
-	if (!rec)
+	if (!rec || cxl_rec_not_repairable(&rec->media_hdr))
 		return NULL;
 
 	validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
-- 
2.55.0

[PATCH v5 3/3] cxl/edac: Mask DPA flags in the event record cache key

[Jongwon Lee]

The low bits of the DPA field in General Media and DRAM event records
are flags. Bit 0 is VOLATILE and bit 1 is NOT_REPAIRABLE. The trace
events print the DPA with CXL_DPA_MASK applied, while
cxl_store_rec_gen_media() and cxl_store_rec_dram() use the raw value as
the xarray key.

User space takes the DPA from the trace event and writes it to the
mem_repair dpa attribute. If the record has any flag set, xa_load() does
not find it and the repair fails with -EINVAL.

Use the masked DPA as the key. Records marked NOT_REPAIRABLE are still
rejected, because the lookup skips them.

Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
Signed-off-by: Jongwon Lee <ai@linux.com>
---
 drivers/cxl/core/edac.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 42683b7c54..f627c73ff6 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -918,6 +918,16 @@ struct cxl_mem_repair_attrbs {
 	enum cxl_mem_repair_type repair_type;
 };
 
+/*
+ * The low bits of the event record DPA are flags (VOLATILE, NOT_REPAIRABLE).
+ * Records are looked up by the DPA user space writes back, which comes from
+ * the trace event and has the flags masked off, so key them the same way.
+ */
+static inline unsigned long cxl_rec_dpa(const struct cxl_event_media_hdr *hdr)
+{
+	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_MASK;
+}
+
 static inline bool cxl_rec_not_repairable(const struct cxl_event_media_hdr *hdr)
 {
 	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_NOT_REPAIRABLE;
@@ -1131,8 +1141,7 @@ int cxl_store_rec_gen_media(struct cxl_memdev *cxlmd, union cxl_event *evt)
 		return -ENOMEM;
 
 	old_rec = xa_store(&array_rec->rec_gen_media,
-			   le64_to_cpu(rec->media_hdr.phys_addr), rec,
-			   GFP_KERNEL);
+			   cxl_rec_dpa(&rec->media_hdr), rec, GFP_KERNEL);
 	if (xa_is_err(old_rec)) {
 		kfree(rec);
 		return xa_err(old_rec);
@@ -1161,8 +1170,7 @@ int cxl_store_rec_dram(struct cxl_memdev *cxlmd, union cxl_event *evt)
 		return -ENOMEM;
 
 	old_rec = xa_store(&array_rec->rec_dram,
-			   le64_to_cpu(rec->media_hdr.phys_addr), rec,
-			   GFP_KERNEL);
+			   cxl_rec_dpa(&rec->media_hdr), rec, GFP_KERNEL);
 	if (xa_is_err(old_rec)) {
 		kfree(rec);
 		return xa_err(old_rec);
-- 
2.55.0