Re: [PATCH v9 3/4] cxl/region: Start unmapped poison scan at the committed decoder boundary

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cxl/region: broken ctx.part < 0 fallback in cxl_get_poison_by_endpoint()
--

commit 93c022ba94d2f493a95ecc17f926c1ee71ac336f
Author: Richard Cheng <icheng@nvidia.com>

cxl/region: Start unmapped poison scan at the committed decoder boundary

This commit updates cxl_get_poison_unmapped() to resume scanning from
ctx->offset on the starting partition and adds a fallback in
cxl_get_poison_by_endpoint() to reset ctx.part and ctx.offset to 0 when
ctx.part is negative.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ddf12075b95c0..18114d25662b3 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2907,7 +2907,6 @@ static int cxl_get_poison_unmapped(struct cxl_memdev *cxlmd,
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	const struct resource *res;
> -	struct resource *p, *last;
>  	u64 offset, length;
>  	int rc = 0;
>  
> @@ -2920,10 +2919,8 @@ static int cxl_get_poison_unmapped(struct cxl_memdev *cxlmd,
>  	 */
>  	for (int i = ctx->part; i < cxlds->nr_partitions; i++) {
>  		res = &cxlds->part[i].res;
> -		for (p = res->child, last = NULL; p; p = p->sibling)
> -			last = p;
> -		if (last)
> -			offset = last->end + 1;
> +		if (i == ctx->part)
> +			offset = ctx->offset;
>  		else
>  			offset = res->start;
>  		length = res->end - offset + 1;
> @@ -3001,9 +2998,14 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
>  	};
>  
>  	rc = device_for_each_child(&port->dev, &ctx, poison_by_decoder);
> -	if (rc == 1)
> +	if (rc == 1) {
> +		if (ctx.part < 0) {
> +			ctx.part = 0;
> +			ctx.offset = 0;
> +		}
>  		rc = cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev),
>  					     &ctx);
> +	}
>  
>  	return rc;
>  }

[Severity: Medium]
Can this ctx.part < 0 fallback in cxl_get_poison_by_endpoint() work as
intended when gated inside the rc == 1 branch?

Looking at poison_by_decoder() in drivers/cxl/core/region.c, it only
returns 1 when reaching port->commit_end on a decoder that has
cxled->dpa_res allocated:

drivers/cxl/core/region.c:poison_by_decoder() {
	...
	cxled = to_cxl_endpoint_decoder(dev);
	if (!cxled->dpa_res)
		return rc;

	cxlmd = cxled_to_memdev(cxled);
	cxlds = cxlmd->cxlds;
	mode = cxlds->part[cxled->part].mode;
	...
	offset = cxled->dpa_res->start;
	length = cxled->dpa_res->end - offset + 1;
	rc = cxl_mem_get_poison(cxlmd, offset, length, cxled->cxld.region);
	...
	/* Iterate until commit_end is reached */
	if (cxled->cxld.id == ctx->port->commit_end) {
		ctx->offset = cxled->dpa_res->end + 1;
		ctx->part = cxled->part;
		return 1;
	}
}

If the decoder at commit_end has !cxled->dpa_res, poison_by_decoder()
returns 0 before checking commit_end, so device_for_each_child() returns 0
and the rc == 1 block is never entered.

Conversely, if rc == 1 is reached with ctx.part < 0, then the decoder at
commit_end had a valid cxled->dpa_res with cxled->part < 0. By that point,
poison_by_decoder() has already indexed cxlds->part[cxled->part].mode out
of bounds with a negative index, queried poison for the committed
decoders' DPA ranges via cxl_mem_get_poison(), and recorded the handoff
boundary in ctx->offset = cxled->dpa_res->end + 1.

Resetting ctx.part = 0 and ctx.offset = 0 here discards that handoff
offset and causes cxl_get_poison_unmapped() to re-scan from DPA 0, issuing
duplicate cxl_mem_get_poison() calls over the already-scanned committed
decoder DPA ranges with region set to NULL.

In addition, because cxl_get_poison_by_endpoint() is the only caller of
cxl_get_poison_unmapped(), overwriting ctx.part < 0 before the call makes
the existing check at the start of cxl_get_poison_unmapped() unreachable:

drivers/cxl/core/region.c:cxl_get_poison_unmapped() {
	...
	if (ctx->part < 0)
		return 0;
}

Should poison_by_decoder() guard against cxled->part < 0 before indexing
cxlds->part[], and is the ctx.part < 0 fallback in
cxl_get_poison_by_endpoint() meant to handle a case where
device_for_each_child() returns 0 rather than 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020051403.14186-1-icheng@nvidia.com?part=3