Re: [PATCH v9 02/10] cxl/pci: Add BI topology enable/disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cxl/pci: TOCTOU race in BI setup against concurrent AER reset
--

commit 1daa4037cbac0ebc16630c3d4f45c358bd7890f9
Author: Davidlohr Bueso <dave@stgolabs.net>

cxl/pci: Add BI topology enable/disable

This patch implements setup and teardown of the Back-Invalidate (BI) topology
on CXL endpoints and upstream components. It also adds a reset detection
mechanism to verify whether a reset cleared the hardware BI Enable state.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index b8676a3d6ec90..f3ca9861a7ba1 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
[ ... ]
> +static int __cxl_bi_ctrl_endpoint(struct cxl_dev_state *cxlds, bool enable)
> +{
[ ... ]
> +	FIELD_MODIFY(CXL_BI_DECODER_CTRL_BI_ENABLE, &ctrl, enable);
> +	writel(ctrl, bi + CXL_BI_DECODER_CTRL_OFFSET);
> +	cxlds->bi = enable;

[Severity: Medium]
Could this lockless update of cxlds->bi race with a concurrent AER reset
handled by cxl_bi_reset_detected()?

If a user manually binds the cxl_port driver via sysfs concurrently with a
PCI AER reset (SBR), the manual sysfs bind takes the endpoint->dev lock,
while the AER recovery (cxl_reset_done()) takes the cxlmd->dev lock, allowing
them to execute concurrently.

There is a window between the writel() that sets the hardware bit and the
assignment of cxlds->bi. If the AER SBR wipes the hardware bit in this window,
could cxl_bi_reset_detected() check the still-false cxlds->bi and ignore the
reset?

> +
> +	dev_dbg(cxlds->dev, "BI requests %s\n", str_enabled_disabled(enable));
> +
> +	return 0;
> +}
[ ... ]
> +void cxl_bi_reset_detected(struct cxl_port *endpoint)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(endpoint->uport_dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	if (!cxlds->bi)
> +		return;

If cxl_bi_reset_detected() returns early here because cxlds->bi is false, and
then __cxl_bi_ctrl_endpoint() resumes and sets cxlds->bi = true, the software
state becomes permanently desynchronized from the hardware state
(BI_ENABLE = 0), breaking the fail-closed coherency recovery model.

> +
> +	if (cxl_bi_decoder_enabled(endpoint))
> +		return;
> +
> +	dev_warn(cxlds->dev, "BI disabled by reset\n");
> +	cxlds->bi = false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790103847.git.dave@stgolabs.net?part=2