Re: [PATCH v4] cxl/pci: Skip reset detection for DVSEC emulated decoders

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

Pre-existing issues:
- [High] Missing exclusion against `cxl_port` unbind in `__cxl_endpoint_decoder_reset_detected()` can lead to a Use-After-Free or NULL pointer dereference of `cxlhdm`.
- [High] Unsynchronized read-modify-write of `cxld->flags` in `cxl_endpoint_decoder_clear_reset_flags()` races with concurrent updates.
--

--- Patch [1]: [PATCH v4] cxl/pci: Skip reset detection for DVSEC emulated decoders ---
commit b6f7c04ddbcccfb809033bdab75f996fc7c78db7
Author: Guixin Liu <[email protected]>
cxl/pci: Skip reset detection for DVSEC emulated decoders

This commit skips the reset detection check for decoders emulated from the
CXL DVSEC range registers, as they lack HDM decoder registers and commit
callbacks. This prevents a kernel oops when checking for a reset on these
emulated decoders.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002ca..d8b07f86bab09 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -683,6 +683,13 @@ static int __cxl_endpoint_decoder_reset_detected(struct device *dev, void *data)
[ ... ]
> +	/*
> +	 * Decoders emulated from the DVSEC range registers have no commit
> +	 * callback and no HDM decoder registers to consult.
> +	 */
> +	if (!cxld->commit)
> +		return 0;
> +
>  	cxlhdm = dev_get_drvdata(&port->dev);
>  	hdm = cxlhdm->regs.hdm_decoder;

[Severity: High]
This is a pre-existing issue, but could this missing exclusion against a
cxl_port unbind lead to a Use-After-Free or NULL pointer dereference of
cxlhdm?

If a concurrent sysfs unbind of the cxl_port driver occurs while a PCI
reset handler iterates over the endpoint's children in cxl_reset_done(),
the devm-allocated memory for cxlhdm could be freed and driver data cleared
before it is accessed here.

>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
>  
>  	return !FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl);
>  }

For the pre-existing read-modify-write synchronization issue in
cxl_endpoint_decoder_clear_reset_flags():

drivers/cxl/pci.c:cxl_endpoint_decoder_clear_reset_flags() {
	...
	cxled = to_cxl_endpoint_decoder(dev);
	cxled->cxld.flags &= ~CXL_DECODER_F_RESET_MASK;
	...
}

[Severity: High]
This is a pre-existing issue, but does this function have an
unsynchronized read-modify-write of cxld->flags?

If a concurrent region setup or teardown operation occurs simultaneously with
a PCI reset, the plain bitwise update on cxld->flags in the asynchronous PCI
reset path ignores the cxl_rwsem.region semaphore used by region paths.

Could this race with concurrent updates and lead to state corruption due to
lost flag updates, potentially leaving decoders in an invalid state?

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