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

[PATCH v2] 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.

Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
Signed-off-by: Jongwon Lee <ai@linux.com>
---

Notes:
    Changes in v2:
    - Moved the Claude Code note below the --- (Dave Jiang)
    
    This patch was developed with Claude Code.
    
    Tested on QEMU master (b574eaf) with a CXL type 3 device, region
    committed, KASAN and PROVE_LOCKING on. DRAM events injected with
    cxl-inject-dram-event at 0x1001, 0x2000 and 0x3002, then a repair
    requested through mem_repair0..4. Same result for all five:
    
      DPA written   before    after
      0x1000        -EINVAL   ok
      0x1001        ok        -EINVAL
      0x2000        ok        ok
      0x3000        -EINVAL   -EINVAL
      0x3002        ok        -EINVAL
      0x5000        -EINVAL   -EINVAL
    
    No KASAN, lockdep or WARN output. Only DRAM events were injected; the
    General Media path has the same change but was not exercised. Not
    tested on real CXL hardware.

 drivers/cxl/core/edac.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 34b81e8dfb..b3dfc33ca6 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -918,6 +918,21 @@ 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 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 struct cxl_event_gen_media *
 cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
 		       struct cxl_mem_repair_attrbs *attrbs)
@@ -929,7 +944,7 @@ cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
 		return NULL;
 
 	rec = xa_load(&array_rec->rec_gen_media, attrbs->dpa);
-	if (!rec)
+	if (!rec || cxl_rec_not_repairable(&rec->media_hdr))
 		return NULL;
 
 	if (attrbs->repair_type == CXL_PPR)
@@ -950,7 +965,7 @@ cxl_find_rec_dram(struct cxl_memdev *cxlmd,
 		return NULL;
 
 	rec = xa_load(&array_rec->rec_dram, 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);
@@ -1105,8 +1120,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);
@@ -1135,8 +1149,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 v2] cxl/edac: Mask DPA flags in the event record cache key

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl/edac: use-after-free race in cxl_find_rec_gen_media() and cxl_find_rec_dram()
--

commit 5617c750af672417152a61a535a4548061ad0978
Author: Jongwon Lee <ai@linux.com>

cxl/edac: Mask DPA flags in the event record cache key

This commit masks the low flag bits from the event record DPA when storing
General Media and DRAM error records in the xarray cache so lookups using the
masked DPA from trace events succeed. It also rejects repair requests when the
cached event record has the CXL_DPA_NOT_REPAIRABLE flag set.

> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 34b81e8dfbbb6..b3dfc33ca6740 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -918,6 +918,21 @@ 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 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 struct cxl_event_gen_media *
>  cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
>  		       struct cxl_mem_repair_attrbs *attrbs)
> @@ -929,7 +944,7 @@ cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
>  		return NULL;
>  
>  	rec = xa_load(&array_rec->rec_gen_media, attrbs->dpa);
> -	if (!rec)
> +	if (!rec || cxl_rec_not_repairable(&rec->media_hdr))
>  		return NULL;
>  
>  	if (attrbs->repair_type == CXL_PPR)
> @@ -950,7 +965,7 @@ cxl_find_rec_dram(struct cxl_memdev *cxlmd,
>  		return NULL;
>  
>  	rec = xa_load(&array_rec->rec_dram, attrbs->dpa);
> -	if (!rec)
> +	if (!rec || cxl_rec_not_repairable(&rec->media_hdr))
>  		return NULL;

[Severity: High]
Can cxl_find_rec_gen_media() and cxl_find_rec_dram() race with concurrent
event record insertion and trigger a use-after-free when dereferencing rec?

On the event reporting path, drivers/cxl/core/mbox.c:cxl_event_trace_record()
acquires device_lock(&cxlmd->dev) and shared read locks on cxl_rwsem.region
and cxl_rwsem.dpa before storing and freeing records:

drivers/cxl/core/mbox.c:cxl_event_trace_record()
    guard(device)(&cxlmd->dev);
    guard(rwsem_read)(&cxl_rwsem.region);
    guard(rwsem_read)(&cxl_rwsem.dpa);
    ...
    cxl_store_rec_gen_media() / cxl_store_rec_dram()
        old_rec = xa_store(..., cxl_rec_key(&rec->media_hdr), rec, GFP_KERNEL);
        ...
        kfree(old_rec);
        cxl_del_expired_gmedia_recs() / cxl_del_expired_dram_recs()
        cxl_del_overflow_old_recs()

On the EDAC sysfs repair path, cxl_do_ppr() -> cxl_mem_perform_ppr() and
cxl_do_mem_sparing() -> cxl_mem_perform_sparing() only take shared read locks
on cxl_rwsem.region and cxl_rwsem.dpa, without holding device_lock(&cxlmd->dev)
or the xarray lock.

Because both paths hold cxl_rwsem.region and cxl_rwsem.dpa in shared read
mode, they can run concurrently. xa_load() drops rcu_read_lock() before
returning rec, and the store path frees displaced, expired, or overflowed
records immediately with kfree().

Previously, cxl_find_rec_gen_media() and its caller cxl_mem_perform_ppr()
only checked whether the returned pointer was non-NULL without dereferencing
it. With this change, cxl_find_rec_gen_media() now dereferences
rec->media_hdr.phys_addr in cxl_rec_not_repairable(), cxl_find_rec_dram() adds
another unlocked dereference of rec->media_hdr.phys_addr before its existing
field accesses, and masking the DPA flags in cxl_rec_key() makes same-DPA
events with different flag bits collide and free old_rec in xa_store().

Should the lookup and record inspection in cxl_find_rec_gen_media() and
cxl_find_rec_dram() be serialized with cxl_store_rec_gen_media() and
cxl_store_rec_dram() (for example using xa_lock or RCU-safe freeing)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031161121.15948-1-ai@linux.com?part=1