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

[sashiko-bot@kernel.org]

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]?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-fix_two_issues_introduced_by_cxl_cancel_auto_attach-v1-0-5d94ca06c4e4@zohomail.com?part=2