Re: [PATCH 2/2] cxl/region: Fill first free targets[] slot during auto-discovery

[Alison Schofield <alison.schofield@intel.com>]

On Mon, Jun 08, 2026 at 12:38:42PM +0800, Li Ming wrote:
> On 6/6/2026 4:11 PM, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] Loops iterating over the `p->targets` array up to `p->nr_targets` unconditionally dereference elements, leading to NULL pointer dereferences because the array can be sparse.
> > --
> >
> > commit bf78125f1b0b7a811b9b200d37ed57cc96619a00
> > Author: Li Ming <ming.li@zohomail.com>
> >
> > cxl/region: Fill first free targets[] slot during auto-discovery
> >
> > This commit adjusts the cxl_region_attach_auto logic to find the first free
> > target slot for endpoint decoder attachment, rather than appending to the tail.
> > This ensures NULL holes in the target array are filled to avoid accessing an
> > endpoint decoder via a NULL pointer and miscounting valid endpoint decoders.
> >
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index c4335ebf19f7f..532dac77bd003 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -1848,8 +1848,21 @@ static int cxl_region_attach_auto(struct cxl_region *cxlr,
> >>  	 * this means that userspace can view devices in the wrong position
> >>  	 * before the region activates, and must be careful to understand when
> >>  	 * it might be racing region autodiscovery.
> >> +	 *
> >> +	 * The endpoint decoder will be recorded into the first free slot of
> >> +	 * the target array.
> >>  	 */
> >> -	pos = p->nr_targets;
> >> +	for (pos = 0; pos < p->interleave_ways; pos++) {
> >> +		if (!p->targets[pos])
> >> +			break;
> >> +	}
> >> +
> >> +	if (pos == p->interleave_ways) {
> >> +		dev_err(&cxlr->dev, "%s: unable to find a free target slot\n",
> >> +			dev_name(&cxled->cxld.dev));
> >> +		return -ENXIO;
> >> +	}
> >> +
> >>  	p->targets[pos] = cxled;
> > [Severity: High]
> > This is a pre-existing issue, but since __cxl_decoder_detach() can leave
> > sparse holes in p->targets without compacting the array, are the loops
> > that iterate up to p->nr_targets still susceptible to NULL pointer
> > dereferences?
> >
> > For instance, in drivers/cxl/core/region.c:cxl_dpa_to_hpa():
> >
> >     for (int i = 0; i < p->nr_targets; i++) {
> >         if (cxlmd == cxled_to_memdev(p->targets[i])) {
> >
> > If there is a hole at an index less than p->nr_targets, wouldn't
> > cxled_to_memdev() dereference a NULL pointer?
> >
> > Similar unprotected iterations seem to exist in
> > cxl_scrub_get_attrbs_region(), unaligned_region_offset_to_dpa_result(),
> > and region_offset_to_dpa_result().
> >
> > Does the array need to be compacted upon detach, or should these loops be
> > updated to check for NULL pointers before dereferencing p->targets[i]?
> 
> It is not a bug here.
> 
> cxl driver always calls cxl_dpa_to_region() for a cxl region checking before involving cxl_dpa_to_hpa(), but if a cxl region is not bound to cxl region driver, driver will not involve cxl_dpa_to_hpa().
> 
> But maybe adding cxlr->driver checking in cxl_dpa_to_hpa() is a choice.
> 

Hi Ming, 
I'm in agreemnet that there is not a reachable NULL deref path here.
With this patch, the hole is gone by the time those loops run.

A hole only exists transiently between a detach and the next stage,
while the region is not comitted. The windows suggested aren't real.

I'm kind of against adding an explicit check in cxl_dpa_to_hpa(), as
'hardening' because that undercuts this whole point here.

-- Alison



> >
> >>  	cxled->pos = pos;
> >>  	cxled->state = CXL_DECODER_STATE_AUTO_STAGED;
> 
> 
>