Re: [RFC PATCH 5/5] cxl/region: Register device poison before exposing RAM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cxl: probe abort due to strict -ERANGE return in cxl_poison_store_hpa()
- [High] cxl: infinite loop in cxl_poison_record_to_pfns() due to integer overflow
--

commit 9d79e4bd40f84f47cf37cc9109545f12829756e5
Author: Shaikh Kamaluddin <shaikhkamal2012@gmail.com>

cxl/region: Register device poison before exposing RAM

This commit retrieves device-reported poison lists during CXL RAM region probe
and registers them with the pre-online hwpoison registry to withhold bad
frames from being onlined.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c72524923b380..e3ffa77927df8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> +static int cxl_poison_store_hpa(struct cxl_region *cxlr, struct xarray *pfns,
> +				u64 hpa)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	int rc;
> +
> +	/* Outside this region: not ours to withhold. */
> +	if (!cxl_resource_contains_addr(p->res, hpa))
> +		return -ERANGE;

[Severity: High]
Does this error handling cause the entire region probe to abort when a poison
record falls outside the expected range?

The comment notes "not ours to withhold", which suggests this is an expected
condition that should simply be skipped. However, returning -ERANGE here
causes the caller cxl_poison_record_to_pfns() to return the error immediately,
which bubbles up through cxl_region_collect_decoder_poison() and aborts the
entire region probe.

Should this return 0 instead to avoid failing the region probe when devices
return poison records that span outside the queried DPA range?

> +
> +	rc = cxl_poison_store_pfn(pfns, hpa);
> +	if (rc || !p->cache_size)
> +		return rc;
[ ... ]
> +static int cxl_poison_record_to_pfns(struct cxl_region *cxlr,
> +				     struct cxl_memdev *cxlmd,
> +				     const struct cxl_poison_record *record,
> +				     struct xarray *pfns)
> +{
[ ... ]
> +	step = clamp_t(u64, p->interleave_granularity, CXL_POISON_LEN_MULT,
> +		       PAGE_SIZE);
> +
> +	for (cur = ALIGN_DOWN(dpa, step); cur < end; cur += step) {

[Severity: High]
Can this loop iterate infinitely due to an integer wrap-around?

If an untrusted device provides a dpa near U64_MAX and a small length, the
check_add_overflow() validation at the top of this function will pass. When
cur is initialized to ALIGN_DOWN(dpa, step), the cur += step increment could
overflow and wrap around to 0. Since 0 is less than end, the loop condition
remains true forever, potentially causing a lockup while holding the cxl_rwsem
locks.

> +		u64 hpa = cxl_dpa_to_hpa(cxlr, cxlmd, max(cur, dpa));
> +		int rc;
> +
> +		/* Not mapped by this region; nothing here to withhold. */
> +		if (hpa == ULLONG_MAX)
> +			continue;
> +
> +		rc = cxl_poison_store_hpa(cxlr, pfns, hpa);
> +		if (rc)
> +			return rc;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010162017.62506-1-shaikhkamal2012@gmail.com?part=5