Re: [PATCH v3] 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] A race condition between PCI error handler and port driver unbind can cause a Use-After-Free or NULL pointer dereference on the `cxlhdm` structure.
- [High] Concurrent non-atomic modifications to `cxld->flags` in `cxl_endpoint_decoder_clear_reset_flags()` can lead to lost updates and state corruption.
--

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

cxl/pci: Skip reset detection for DVSEC emulated decoders

This commit skips the reset detection check for CXL decoders emulated from
DVSEC ranges. It addresses a crash and false warnings during PCI reset when
HDM decoder registers are unused or non-existent, as these decoders do not
have a commit state to lose.

[ ... ]

> - cxld->flags is updated with a plain read-modify-write in
>   cxl_endpoint_decoder_clear_reset_flags(), while cxl_decoder_commit() and
>   cxl_decoder_reset() update the same word under cxl_rwsem.region, which
>   __commit() and the region reset paths hold across those calls. Having the
>   reset handler take that rwsem too looks like the natural fix, but it
>   already holds the memdev device lock at that point, so the lock ordering
>   wants review first.

[Severity: High]
This isn't a bug introduced by this patch, but since it is discussed here, can
the non-atomic modification to cxld->flags in
cxl_endpoint_decoder_clear_reset_flags() lead to lost updates?

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

During a PCI reset, cxl_reset_done() calls this function which performs the
unprotected read-modify-write on cxld->flags. Concurrently, operations like
cxl_decoder_commit() modify the same flags word under the cxl_rwsem.region
lock.

Because the reset handler does not hold this semaphore, can these updates race
and overwrite each other, potentially dropping the CXL_DECODER_F_ENABLE or
lock flags and leaving the decoder in an inconsistent state?

[ ... ]

> 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)
>  	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>  		return 0;
>  
> +	/*
> +	 * 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 isn't a bug introduced by this patch, but does this code risk a
use-after-free or a NULL pointer dereference on the cxlhdm structure?

A PCI reset (like FLR or SBR) triggers cxl_reset_done(), which walks endpoint
decoders without holding locks that would prevent the cxl_port driver from
unbinding from the endpoint port.

If the port unbinds concurrently, devres frees the cxl_hdm structure.
The device walk here in __cxl_endpoint_decoder_reset_detected() then calls
dev_get_drvdata() on the unbound port device, retrieves a dangling pointer to
cxlhdm, and immediately dereferences it at cxlhdm->regs.hdm_decoder.

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

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