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

[sashiko-bot@kernel.org]

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