[PATCH v3 0/2] cxl/edac: Fix event record lookups for memory repair

[PATCH v3 0/2] cxl/edac: Fix event record lookups for memory repair

[Jongwon Lee]

Patch 2 is the DPA flag masking fix from v2. Sashiko's review of v2
pointed out that the repair path reads cached DRAM event records with
no lock that keeps the event path from freeing them [1]. That race was
already there in cxl_find_rec_dram() and cxl_mem_perform_sparing(); v2
added one more read of a record, in the General Media lookup.

Patch 1 fixes the race for the DRAM lookup and the sparing path. Patch 2
does its General Media lookup under the same lock.

I used xa_lock() instead of RCU freeing. The cached records are copies
of the spec-defined event structures, so kfree_rcu() would need a
wrapper struct with an rcu_head, and the lookups only compare a few
fields. Taking device_lock() in the repair path, as the event path does,
looked like it could deadlock against unbind, where the EDAC sysfs
files are removed with device_lock() held.

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)

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, QEMU master (b574eaf) with a CXL type 3 device, region
committed, KASAN and PROVE_LOCKING on:

- Same test as v1/v2: DRAM events injected at 0x1001, 0x2000 and
  0x3002, then a repair requested through mem_repair0..4 for six DPAs.
  Results match v2 for all five repair types.
- Race test: the host injects DRAM events every 20 ms (the same DPA
  again and again, so xa_store() replaces and frees records, plus enough
  distinct DPAs to hit the 200-record overflow path) while the guest
  requests repairs in a loop on all five mem_repair devices, 300 s.
  A test-only mdelay(20) after xa_load() widens the window.
  Unpatched: KASAN slab-use-after-free in cxl_find_rec_dram(), called
  from cxl_do_mem_sparing(). v3, delay inside the locked section: 3780
  repair requests, no KASAN, lockdep or WARN output.
  Without the delay neither kernel reported anything in 600 s.

This series was developed with Claude Code.

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

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

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


base-commit: cf15f9f2361dd4002203716b7b878393c0eee4bf
-- 
2.55.0

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

[Jongwon Lee]

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

The repair path looks records up with cxl_find_rec_dram() while holding
only cxl_rwsem for read, so the two 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 the event path has just freed.

Move the lookup into __cxl_find_rec_dram(), called with the xarray lock
held, and make cxl_find_rec_dram() return whether a record matched and
copy out the validity flags the sparing path needs. 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 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
Signed-off-by: Jongwon Lee <ai@linux.com>
---
 drivers/cxl/core/edac.c | 69 +++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 34b81e8dfb..f0c12871bd 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,32 @@ 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 what the caller needs.
+ */
+static bool cxl_find_rec_dram(struct cxl_memdev *cxlmd,
+			      struct cxl_mem_repair_attrbs *attrbs,
+			      u16 *validity_flags)
+{
+	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
+	struct cxl_event_dram *rec;
+	bool found;
+
+	if (!array_rec)
+		return false;
+
+	xa_lock(&array_rec->rec_dram);
+	rec = __cxl_find_rec_dram(&array_rec->rec_dram, attrbs);
+	found = rec;
+	if (rec && validity_flags)
+		*validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
+	xa_unlock(&array_rec->rec_dram);
+
+	return found;
+}
+
 #define CXL_MAX_STORAGE_DAYS 10
 #define CXL_MAX_STORAGE_TIME_SECS (CXL_MAX_STORAGE_DAYS * 24 * 60 * 60)
 
@@ -1298,9 +1323,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,
+				 u16 *validity_flags)
 {
 	struct cxl_mem_repair_attrbs attrbs = { 0 };
 
@@ -1332,10 +1357,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, validity_flags);
 }
 
 static int
@@ -1344,7 +1369,7 @@ 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;
+	bool rec_found = false;
 	u16 validity_flags = 0;
 	int ret;
 
@@ -1362,11 +1387,12 @@ 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,
+							 &validity_flags);
+			if (!rec_found)
 				return -EINVAL;
 
-			if (!get_unaligned_le16(rec->media_hdr.validity_flags))
+			if (!validity_flags)
 				return -EINVAL;
 		}
 	}
@@ -1376,13 +1402,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 +1422,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 +1837,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 v3 2/2] cxl/edac: Mask DPA flags in the event record cache key

[Jongwon Lee]

The low bits of the DPA in a General Media or DRAM event record are
flags: bit 0 is VOLATILE and bit 1 is NOT_REPAIRABLE. The trace events
report the DPA with CXL_DPA_MASK applied, but cxl_store_rec_gen_media()
and cxl_store_rec_dram() use the raw value as the xarray key.

User space reads the DPA from the trace event and writes it to the
mem_repair dpa attribute. If the record had any flag set, xa_load()
misses it and the repair fails with -EINVAL. The raw key also lets a
repair go through for a record the device marked NOT_REPAIRABLE, if the
DPA is written with bit 1 set.

Key the records by the masked DPA and skip records marked
NOT_REPAIRABLE. cxl_find_rec_gen_media() now reads the record, so do
the lookup under the xarray lock as cxl_find_rec_dram() does, and
return a bool since its only caller tests 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 | 43 ++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index f0c12871bd..94d44a3dc1 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -918,24 +918,37 @@ 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)
+/*
+ * 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 unsigned long cxl_rec_key(const struct cxl_event_media_hdr *hdr)
+{
+	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_MASK;
+}
+
+static 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;
+	found = rec && !cxl_rec_not_repairable(&rec->media_hdr);
+	xa_unlock(&array_rec->rec_gen_media);
 
-	if (attrbs->repair_type == CXL_PPR)
-		return rec;
-
-	return NULL;
+	return found;
 }
 
 /* Caller holds the xarray lock. The record may be freed once it is dropped. */
@@ -949,7 +962,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);
@@ -1130,8 +1143,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_key(&rec->media_hdr), rec, GFP_KERNEL);
 	if (xa_is_err(old_rec)) {
 		kfree(rec);
 		return xa_err(old_rec);
@@ -1160,8 +1172,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_key(&rec->media_hdr), rec, GFP_KERNEL);
 	if (xa_is_err(old_rec)) {
 		kfree(rec);
 		return xa_err(old_rec);
-- 
2.55.0

Re: [PATCH v3 0/2] cxl/edac: Fix event record lookups for memory repair

[Richard Cheng]

On Sun, Nov 01, 2026 at 10:34:18AM +0800, Jongwon Lee wrote:
> Patch 2 is the DPA flag masking fix from v2. Sashiko's review of v2
> pointed out that the repair path reads cached DRAM event records with
> no lock that keeps the event path from freeing them [1]. That race was
> already there in cxl_find_rec_dram() and cxl_mem_perform_sparing(); v2
> added one more read of a record, in the General Media lookup.
> 
> Patch 1 fixes the race for the DRAM lookup and the sparing path. Patch 2
> does its General Media lookup under the same lock.
> 
> I used xa_lock() instead of RCU freeing. The cached records are copies
> of the spec-defined event structures, so kfree_rcu() would need a
> wrapper struct with an rcu_head, and the lookups only compare a few
> fields. Taking device_lock() in the repair path, as the event path does,
> looked like it could deadlock against unbind, where the EDAC sysfs
> files are removed with device_lock() held.
>

Hi Jongwon,

Small nit, if you don't mind maybe place the description of patch 1
before patch 2, that will help us when reviewing.

Some comments in the following patches, thanks.

Best regards,
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)
> 
> 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, QEMU master (b574eaf) with a CXL type 3 device, region
> committed, KASAN and PROVE_LOCKING on:
> 
> - Same test as v1/v2: DRAM events injected at 0x1001, 0x2000 and
>   0x3002, then a repair requested through mem_repair0..4 for six DPAs.
>   Results match v2 for all five repair types.
> - Race test: the host injects DRAM events every 20 ms (the same DPA
>   again and again, so xa_store() replaces and frees records, plus enough
>   distinct DPAs to hit the 200-record overflow path) while the guest
>   requests repairs in a loop on all five mem_repair devices, 300 s.
>   A test-only mdelay(20) after xa_load() widens the window.
>   Unpatched: KASAN slab-use-after-free in cxl_find_rec_dram(), called
>   from cxl_do_mem_sparing(). v3, delay inside the locked section: 3780
>   repair requests, no KASAN, lockdep or WARN output.
>   Without the delay neither kernel reported anything in 600 s.
> 
> This series was developed with Claude Code.
> 
> [1] https://lore.kernel.org/r/sashiko-outbox-192042@kernel.org
> 
> Jongwon Lee (2):
>   cxl/edac: Read cached DRAM event records under the xarray lock
>   cxl/edac: Mask DPA flags in the event record cache key
> 
>  drivers/cxl/core/edac.c | 112 ++++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 39 deletions(-)
> 
> 
> base-commit: cf15f9f2361dd4002203716b7b878393c0eee4bf
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 1/2] cxl/edac: Read cached DRAM event records under the xarray lock

[Richard Cheng]

On Sun, Nov 01, 2026 at 10:34:19AM +0800, Jongwon Lee wrote:
> The event path caches DRAM event records in an xarray and frees a record
> when xa_store() replaces it, when it expires and when the cache
> overflows. It runs with device_lock() and cxl_rwsem held for read.
> 
> The repair path looks records up with cxl_find_rec_dram() while holding
> only cxl_rwsem for read, so the two 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 the event path has just freed.
> 
> Move the lookup into __cxl_find_rec_dram(), called with the xarray lock
> held, and make cxl_find_rec_dram() return whether a record matched and
> copy out the validity flags the sparing path needs. 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 take the same lock.
>

Hi Jongwon,

Did the bug actually happened ? if so , can you provide the error log ?

Seems like the root cause is that the cxl's rwsem was only protecting CXL
topology and DPA, while leaving memory problem's entry ( those xarray entry )
without protection.

Using xarray lock is fair for me, keeping the access short and the entry
already use xarray as bookkeeping, though I'm wonder if there're better
alternative for locking mechanism. we might want to discuss abit about that.

Another thing is about changing cxl_find_rec_dram()'s signature.
Since the pointer can't be gauranteed valid after the function lifetime ends,
return a boolean instead of pointer is fine.
However, copy of the flag doesn't seem like an elegant choice,
if caller needs other fields in the future or bookkeeping adds more info
in the future, the signature here will need to be altered again.

What if we copy the whole struct cxl_event_dram ? PPR can continue
passing NULL since it only needs matching result.

Something like the following

```
xa_lock(&array_rec->rec_dram);
rec = __cxl_find_rec_dram(&array_rec->rec_dram, attrbs);
found = rec;
if (rec && out)
	*out = *rec;
xa_unlock(&array_rec->rec_dram);

The sparing caller uses its own record:

struct cxl_event_dram rec;

rec_found = cxl_mem_get_rec_dram(cxlmd, cxl_sparing_ctx, &rec);
if (!rec_found)
	return -EINVAL;

validity_flags = get_unaligned_le16(rec.media_hdr.validity_flags);
```

What do you think ?

Best regards,
Richard Cheng.




> 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
> Signed-off-by: Jongwon Lee <ai@linux.com>
> ---
>  drivers/cxl/core/edac.c | 69 +++++++++++++++++++++++++++--------------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 34b81e8dfb..f0c12871bd 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,32 @@ 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 what the caller needs.
> + */
> +static bool cxl_find_rec_dram(struct cxl_memdev *cxlmd,
> +			      struct cxl_mem_repair_attrbs *attrbs,
> +			      u16 *validity_flags)
> +{
> +	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
> +	struct cxl_event_dram *rec;
> +	bool found;
> +
> +	if (!array_rec)
> +		return false;
> +
> +	xa_lock(&array_rec->rec_dram);
> +	rec = __cxl_find_rec_dram(&array_rec->rec_dram, attrbs);
> +	found = rec;
> +	if (rec && validity_flags)
> +		*validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
> +	xa_unlock(&array_rec->rec_dram);
> +
> +	return found;
> +}
> +
>  #define CXL_MAX_STORAGE_DAYS 10
>  #define CXL_MAX_STORAGE_TIME_SECS (CXL_MAX_STORAGE_DAYS * 24 * 60 * 60)
>  
> @@ -1298,9 +1323,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,
> +				 u16 *validity_flags)
>  {
>  	struct cxl_mem_repair_attrbs attrbs = { 0 };
>  
> @@ -1332,10 +1357,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, validity_flags);
>  }
>  
>  static int
> @@ -1344,7 +1369,7 @@ 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;
> +	bool rec_found = false;
>  	u16 validity_flags = 0;
>  	int ret;
>  
> @@ -1362,11 +1387,12 @@ 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,
> +							 &validity_flags);
> +			if (!rec_found)
>  				return -EINVAL;
>  
> -			if (!get_unaligned_le16(rec->media_hdr.validity_flags))
> +			if (!validity_flags)
>  				return -EINVAL;
>  		}
>  	}
> @@ -1376,13 +1402,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 +1422,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 +1837,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
> 

Re: [PATCH v3 2/2] cxl/edac: Mask DPA flags in the event record cache key

[Richard Cheng]

On Sun, Nov 01, 2026 at 10:34:20AM +0800, Jongwon Lee wrote:
> The low bits of the DPA in a General Media or DRAM event record are
> flags: bit 0 is VOLATILE and bit 1 is NOT_REPAIRABLE. The trace events
> report the DPA with CXL_DPA_MASK applied, but cxl_store_rec_gen_media()
> and cxl_store_rec_dram() use the raw value as the xarray key.
> 
> User space reads the DPA from the trace event and writes it to the
> mem_repair dpa attribute. If the record had any flag set, xa_load()
> misses it and the repair fails with -EINVAL. The raw key also lets a
> repair go through for a record the device marked NOT_REPAIRABLE, if the
> DPA is written with bit 1 set.
> 
> Key the records by the masked DPA and skip records marked
> NOT_REPAIRABLE. cxl_find_rec_gen_media() now reads the record, so do
> the lookup under the xarray lock as cxl_find_rec_dram() does, and
> return a bool since its only caller tests 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 | 43 ++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index f0c12871bd..94d44a3dc1 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -918,24 +918,37 @@ 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)
> +/*
> + * 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 unsigned long cxl_rec_key(const struct cxl_event_media_hdr *hdr)
> +{
> +	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_MASK;
> +}
> +
> +static bool cxl_rec_not_repairable(const struct cxl_event_media_hdr *hdr)
> +{
> +	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_NOT_REPAIRABLE;
> +}
> +

Hi Jongwon,

Maybe something like the following to follow the pattern of rb_parent
hiding the packed address and metadata encoding ?

```
#define CXL_EVENT_DPA(hdr) \
    (le64_to_cpu((hdr)->phys_addr) & CXL_DPA_MASK)
```

Or at least make it static inline.

Best regards,
Richard Cheng.



> +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;
> +	found = rec && !cxl_rec_not_repairable(&rec->media_hdr);
> +	xa_unlock(&array_rec->rec_gen_media);
>  
> -	if (attrbs->repair_type == CXL_PPR)
> -		return rec;
> -
> -	return NULL;
> +	return found;
>  }
>  
>  /* Caller holds the xarray lock. The record may be freed once it is dropped. */
> @@ -949,7 +962,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);
> @@ -1130,8 +1143,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_key(&rec->media_hdr), rec, GFP_KERNEL);
>  	if (xa_is_err(old_rec)) {
>  		kfree(rec);
>  		return xa_err(old_rec);
> @@ -1160,8 +1172,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_key(&rec->media_hdr), rec, GFP_KERNEL);
>  	if (xa_is_err(old_rec)) {
>  		kfree(rec);
>  		return xa_err(old_rec);
> -- 
> 2.55.0
> 

Re: [PATCH v3 1/2] cxl/edac: Read cached DRAM event records under the xarray lock

[Jongwon Lee]

Hi Richard,

Thanks for going through the whole series so carefully. v4 is out
with all three of your comments addressed:
https://lore.kernel.org/r/20261102123233.74598-1-ai@linux.com

On Mon, Nov 02, 2026 at 03:56:17PM +0800, Richard Cheng wrote:
> Did the bug actually happened ? if so , can you provide the error log ?

Only with a test-only delay. Without it, v4 ran for 1800 s and the
unpatched kernel for 900 s, with four repair loops and the host
injecting events with no gap, and neither reported anything. With
mdelay(20) added after xa_load() in cxl_find_rec_dram() and after the
lookup in the sparing path, the unpatched kernel hit this after 120 s
(trimmed):

  BUG: KASAN: slab-use-after-free in cxl_do_mem_sparing+0xed9/0x1000
  Read of size 2 at addr ff1100000670732b by task c1-repair.sh/153
  Call Trace:
   cxl_do_mem_sparing+0xed9/0x1000
   repair_store+0x1d9/0x340
   kernfs_fop_write_iter+0x337/0x510
   vfs_write+0x4a2/0xfc0
   ksys_write+0x111/0x250
   do_syscall_64+0x10e/0x5e0
  Allocated by task 68:
   kmemdup_noprof+0x20/0x40
   cxl_store_rec_dram+0xb0/0x360
   cxl_event_trace_record+0x617/0xd40
   __cxl_event_trace_record+0x14c/0x1d0
   cxl_mem_get_records_log+0x27d/0x540
   cxl_event_thread+0x14e/0x1f0
  Freed by task 68:
   kfree+0x1e3/0x5e0
   cxl_store_rec_dram+0x154/0x360
   cxl_event_trace_record+0x617/0xd40
   __cxl_event_trace_record+0x14c/0x1d0
   cxl_mem_get_records_log+0x27d/0x540
   cxl_event_thread+0x14e/0x1f0
  The buggy address is located 43 bytes inside of
   freed 128-byte region [ff11000006707300, ff11000006707380)

Offset 43 is media_hdr.validity_flags, the second read in the sparing
path, and the record was freed by cxl_store_rec_dram() in the event
thread. I can send the full log if it helps.

So the window is small in practice. The race was reported by Sashiko
from reading the code, and the delayed run confirms the free and the
read can overlap.

> Seems like the root cause is that the cxl's rwsem was only protecting CXL
> topology and DPA, while leaving memory problem's entry ( those xarray entry )
> without protection.

Yes, that matches what I found.

> Using xarray lock is fair for me, keeping the access short and the entry
> already use xarray as bookkeeping, though I'm wonder if there're better
> alternative for locking mechanism. we might want to discuss abit about that.

The other options I looked at were kfree_rcu(), which needs a wrapper
struct with an rcu_head around each cached record, and device_lock() in
the repair path, which can deadlock against unbind. A refcount per
record would also work, but it adds get/put to the event path for a
lookup that copies one small struct. I'm happy to switch if you or the
maintainers prefer one of these.

> What if we copy the whole struct cxl_event_dram ? PPR can continue
> passing NULL since it only needs matching result.

Agreed, that keeps the signature stable if callers need more fields
later. In v4 cxl_find_rec_dram() and cxl_mem_get_rec_dram() take a
struct cxl_event_dram *out, the sparing path reads validity_flags from
its own copy, and PPR passes NULL. The cover letter now describes
patch 1 first, as you asked.

Thanks,
Jongwon

Re: [PATCH v3 2/2] cxl/edac: Mask DPA flags in the event record cache key

[Jongwon Lee]

Hi Richard,

On Mon, Nov 02, 2026 at 04:30:57PM +0800, Richard Cheng wrote:
> Maybe something like the following to follow the pattern of rb_parent
> hiding the packed address and metadata encoding ?
>
> ```
> #define CXL_EVENT_DPA(hdr) \
>     (le64_to_cpu((hdr)->phys_addr) & CXL_DPA_MASK)
> ```
>
> Or at least make it static inline.

I'd like to keep it a function so the argument stays type checked, as
coding-style.rst prefers for function-like macros. I'll make both
helpers static inline, which is what v4 does.

cxl_event_trace_record() in mbox.c open-codes the same mask. If a shared
helper next to CXL_DPA_MASK in trace.h is wanted, I can do that as a
separate cleanup so this fix stays small for stable.

Thanks again for the review,
Jongwon