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

[Richard Cheng <icheng@nvidia.com>]

On Mon, Nov 02, 2026 at 09:32:34PM +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 the matching record into a buffer the caller passes in. The PPR
> path only needs the match and 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 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 | 72 ++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 34b81e8dfb..c5c8dda20e 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 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;
> +	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 && out)
> +		*out = *rec;
> +	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,
> +				 struct cxl_event_dram *rec)
>  {
>  	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, rec);
>  }
>  
>  static int
> @@ -1344,7 +1369,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 +1388,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 +1405,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 +1425,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 +1840,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
>


Reviewed-by: Richard Cheng <icheng@nvidia.com>