Re: [PATCH 3/3] cxl/region: Don't leak tolerated RAM -EFAULT from unmapped poison scan

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] cxl_get_poison_by_memdev() overwrites the return code on each loop iteration, silently dropping errors from earlier partitions.
- [High] cxl_get_poison_unmapped() ignores ctx->offset, causing the DPA range of uncommitted decoders to be entirely omitted from poison scans.
--

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

cxl/region: Don't leak tolerated RAM -EFAULT from unmapped poison scan

cxl_get_poison_unmapped() tolerates the -EFAULT a RAM partition returns
for Get Poison List by skipping that partition, but left rc holding the
error. If the tolerated RAM fault was the last poison query before the
loop ended, the function returned a spurious -EFAULT and the poison-list
read failed even though enumeration succeeded. Reset rc to 0 when
tolerating the fault, matching poison_by_decoder().

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index be246fb09c999..52ba8e9e42885 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2933,8 +2933,10 @@ static int cxl_get_poison_unmapped(struct cxl_memdev *cxlmd,
>  		if (!length)
>  			continue;
>  		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> -		if (rc == -EFAULT && cxlds->part[i].mode == CXL_PARTMODE_RAM)
> +		if (rc == -EFAULT && cxlds->part[i].mode == CXL_PARTMODE_RAM) {
> +			rc = 0;
>  			continue;
> +		}
>  		if (rc)
>  			break;
>  	}

[Severity: High]
This is a pre-existing issue, but does cxl_get_poison_unmapped() ignore
ctx->offset in the loop containing these lines?

Looking at the start of this loop, just above the changed lines:

	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;
		else
			offset = res->start;

It calculates the unmapped start offset using the resource tree's
last->end + 1 rather than the ctx->offset boundary.

In poison_by_decoder(), ctx->offset is explicitly set to indicate where the
unmapped scan should begin:

	/* 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;
	}

Because ctx->offset is bypassed, and the resource tree includes uncommitted
decoders, could this cause the DPA range of uncommitted decoders (and any
unmapped space before them) to be omitted from the poison scans?

[Severity: High]
This is a pre-existing issue, but I also noticed a similar error-handling
pattern in cxl_get_poison_by_memdev() in drivers/cxl/core/memdev.c:

	for (int i = 0; i < cxlds->nr_partitions; i++) {
		const struct resource *res = &cxlds->part[i].res;

		offset = res->start;
		length = resource_size(res);
		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
		/*
		 * Invalid Physical Address is not an error for
		 * volatile addresses. Device support is optional.
		 */
		if (rc == -EFAULT && cxlds->part[i].mode == CXL_PARTMODE_RAM)
			rc = 0;
	}
	return rc;

Does this loop continuously overwrite rc without checking for failure from
previous iterations?

If querying the poison list for the first partition (e.g., PMEM) fails,
the loop seems to continue to the next partition. If the next partition
succeeds, rc is overwritten with 0, which would silently swallow hardware
errors and potentially lead to machine check exceptions when unscanned memory
is accessed.

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