Re: [PATCH v2 07/13] cxl/core: Add region disable and enable for a DPort SBR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] CPU caches are invalidated before the DAX region is unbound, leaving a TOCTOU window where caches can be re-dirtied, leading to fatal Machine Check Exceptions.
- [High] `cxl_region_disable()` blindly dereferences `p->res` before verifying that the region has an assigned resource, causing a guaranteed kernel panic on unconfigured regions.
- [Medium] `cxl_region_disable` offlines System RAM before checking if CPU cache invalidation is supported, leading to unnecessary and permanent memory loss on unsupported architectures.
- [Low] `cxl_region_enable()` misinterprets the return value of `device_attach()`, treating a failure to bind as a success.
--

--- Patch [7]: [PATCH v2 07/13] cxl/core: Add region disable and enable for a DPort SBR ---
commit d35a413a3760382cbad793ba438fa85c779b3677
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/core: Add region disable and enable for a DPort SBR

A Secondary Bus Reset of a CXL Downstream Port removes the downstream
component from the bus, so a CXL region reached through that Port has to
be disabled while the reset runs.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 55c35386d0cb1..0f398ab0d3a41 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
> @@ -1,11 +1,90 @@

[ ... ]

> +int cxl_region_disable(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	unsigned long block_size;
> +	u64 start, end;
> +	int rc;
> +
> +	/*
> +	 * Per CXL r4.0 sec 9.13.1 an Interleave Set has a Base HPA and a Size
> +	 * that are multiples of 256 MB, while a memory block spans up to 2 GB.
> +	 * A block overlapping either end of the range therefore also covers
> +	 * memory outside this region, so round the range inward to block
> +	 * granularity as dax_kmem did when it onlined the range. Offlining a
> +	 * straddling block would migrate pages that the reset does not affect.
> +	 */
> +	block_size = memory_block_size_bytes();
> +	start = ALIGN(p->res->start, block_size);
> +	end = ALIGN_DOWN(p->res->end + 1, block_size);

[Severity: High]
If a region is partially configured, such as when an administrator clears its
size via sysfs (free_hpa()), p->res can be NULL. 

SBR can still collect this region if it has endpoints attached.

Would dereferencing p->res->start and p->res->end here unconditionally cause a
kernel panic for unconfigured regions?

> +	if (start >= end) {
> +		dev_dbg(&cxlr->dev, "%s: HPA %pr spans no whole memory block, no System RAM to offline\n",
> +			__func__, p->res);
> +	} else {
> +		rc = cxl_offline_memory(start, end - start);
> +		if (rc) {
> +			dev_warn(&cxlr->dev, "offline System RAM failed before reset: %d\n",
> +				 rc);
> +			return rc;
> +		}
> +	}
> +
> +	rc = cxl_region_invalidate_memregion(cxlr);

[Severity: Medium]
Could this sequence permanently strand System RAM?

If the system lacks CPU cache invalidation support,
cxl_region_invalidate_memregion() will fail and return -ENXIO.

Because cxl_offline_memory() has already succeeded at this point, the memory
is left offline indefinitely. When the SBR is aborted, the subsequent
cxl_region_enable() will only rebind the driver and won't bring the memory
back online.

> +	if (rc) {
> +		dev_warn(&cxlr->dev, "CPU cache invalidate failed before reset: %d\n",
> +			 rc);
> +		return rc;
> +	}
> +
> +	device_release_driver(&cxlr->dev);

[Severity: Critical]
Is it safe to invalidate the CPU cache before unbinding the DAX driver?

Since cxl_offline_memory() is a no-op for DAX regions, userspace PTE mappings
remain fully active. By invalidating the cache before calling
device_release_driver(), a TOCTOU window is opened where an unprivileged
process could dirty cachelines again. 

If this happens, when the downstream port is reset via SBR, any CPU eviction
of those dirty cachelines will trigger a PCIe Unsupported Request and escalate
into a fatal Machine Check Exception, crashing the host.

> +	dev_dbg(&cxlr->dev, "%s: System RAM offline, region disabled before reset, HPA %pr\n",
> +		__func__, p->res);
> +
> +	return 0;
> +}
> +

[ ... ]

> +void cxl_region_enable(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +
> +	if (device_attach(&cxlr->dev) < 0) {

[Severity: Low]
Does this correctly handle the case when no driver was found? 

device_attach() returns 0 when no driver was found, 1 on success, and < 0 on
error. Checking for < 0 here effectively treats 0 as a success.

This could lead to a misleading debug log that says the region was successfully
re-enabled even when it failed to bind a driver.

> +		dev_dbg(&cxlr->dev, "driver re-attach failed after reset\n");
> +		return;
> +	}
> +
> +	dev_dbg(&cxlr->dev, "%s: region re-enabled after reset, HPA %pr, IW %d, IG %d\n",
> +		__func__, p->res, p->interleave_ways, p->interleave_granularity);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=7