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

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

Pre-existing issues:
- [High] cxl_get_poison_by_memdev() fails to break the loop on error, potentially overwriting a failing return code with a success return code from a subsequent partition.
--

--- Patch [3]: [PATCH v4 3/6] cxl/region: Don't leak tolerated RAM -EFAULT from unmapped poison scan ---
commit a0d57b2595f3317a70e0887bd131f7b1df706fef
Author: Richard Cheng <[email protected]>

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 b03f30c04d350..ddf12075b95c0 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2930,8 +2930,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 isn't a bug introduced by this patch, but was introduced in an earlier
commit be5cbd0840275 ("cxl: Kill enum cxl_decoder_mode"). Seeing the loop
here made me look at similar code in drivers/cxl/core/memdev.c.

Does cxl_get_poison_by_memdev() fail to break the loop on error? 

drivers/cxl/core/memdev.c:cxl_get_poison_by_memdev() {
	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;
	}
}

If cxl_mem_get_poison() fails for an early partition (like partition 0) due to
a hardware mailbox timeout and returns an error like -EIO, rc is set to the
error code. Since the loop lacks an if (rc) break; statement, it proceeds to
the next partition.

If the subsequent partition succeeds, rc is overwritten with 0, and the
function returns success. This masks the failure on partition 0 and leaves the
kernel unaware of poisoned memory.

Could this missing break statement lead to unhandled machine checks and
potential data corruption if that poisoned memory is accessed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3