Re: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] DPC Trigger Status is cleared before relying on DPC containment, allowing uncontained fatal MMIO reads.
- [Critical] Bypassing driver unbind in `cxl_error_detected` exposes the system to MCE from concurrent MMIO accesses during SBR.
- [High] Ignoring `cxl_sbr_collect_regions()` return value leaves CXL regions silently bound or permanently detached.
- [High] Unguarded secondary bus reset and config space data race due to missing PCI config access lock.
- [High] Unbinding the CXL region driver without offlining its memory causes `dax_kmem` to leak physical memory reservations, preventing reprobe.
- [High] Regions are unconditionally re-enabled even if HDM decoder state restoration fails.
--

--- Patch [11]: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery ---
commit e04d0da5a53882402af54024d29f830953f6e9bc
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/DPC: Unbind regions for DPC recovery

This commit splits pci_bridge_secondary_bus_reset() to allow unbinding
CXL regions routed through a port during DPC recovery without offlining
their memory. It keeps the memdev driver bound during containment so the
endpoint and decoders can be restored once the link comes back up.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 2b6f840e22a7d..823b63012f45b 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
[ ... ]
> @@ -284,6 +308,36 @@ static int cxl_sbr_disable_regions(struct pci_dev *dport_pci)
>  	return rc;
>  }
>  
> +/*
> + * Unbind the regions routed through the Downstream Port being reset, leaving
> + * their memory online. Used on the DPC recovery path, where dpc_reset_link()
> + * clears DPC Trigger Status and enters the reset without waiting for the link,
> + * so the device may still be unreachable and the page migration that an offline
> + * performs would have no device to read from.
> + *
> + * Unbinding cannot fail, so unlike cxl_sbr_disable_regions() this never aborts
> + * the reset. The memory stays online across the reset with no region decoding
> + * it; cxl_sbr_enable_regions() reprograms the decoders on the way out.
> + */
> +static void cxl_sbr_unbind_regions(struct pci_dev *dport_pci)
> +{
> +	struct cxl_region *cxlr;
> +	struct xarray regions;
> +	unsigned long index;
> +
> +	if (cxl_sbr_save_hdm_state(dport_pci))
> +		pci_warn(dport_pci, "HDM state not saved, decode will not be restored\n");

[Severity: High]
What happens during recovery if cxl_sbr_save_hdm_state() fails here?

If the state fails to save (e.g., due to a memory allocation failure), we log
a warning but proceed. Later, during recovery in cxl_sbr_enable_regions(),
it appears the code will unconditionally call cxl_region_enable() on all
regions even if no HDM state was restored. Does this risk OS memory mappings
pointing to unprogrammed hardware, which could lead to Machine Check Exceptions
on access?

> +
> +	xa_init(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Does ignoring the return value of cxl_sbr_collect_regions() mean we might
operate on an incomplete list of regions?

If collection encounters an error (such as an allocation failure in xa_insert),
it aborts early and returns a partial list. Uncollected regions would then
be left bound during the reset, meaning any memory accesses to them might
generate uncontained errors. Similarly, when re-enabling, any regions that
failed to collect wouldn't get re-enabled.

> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_unbind(cxlr);

[Severity: High]
Could unbinding the region driver here without offlining its memory cause
dax_kmem to leak physical memory reservations?

If cxl_region_unbind() tears down the DAX devices while the memory is still
online, remove_memory() will fail. The driver intentionally leaks the
request_mem_region() allocation when this happens. Would this prevent
us from successfully reprobing the region after recovery completes?

> +
> +	cxl_sbr_put_regions(&regions);
> +}
[ ... ]
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 99fb00949c2fa..568a906f3b50f 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -323,6 +323,16 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  		}
>  		return PCI_ERS_RESULT_CAN_RECOVER;
>  	case pci_channel_io_frozen:
> +		/*
> +		 * A Port on the path in DPC means dpc_reset_link() is about to
> +		 * reset the link, and that path takes the CXL regions out of
> +		 * service and restores the HDM decode itself. Keep the memdev
> +		 * driver bound so the endpoint and its decoders are still there
> +		 * to restore.
> +		 */
> +		if (pci_dpc_containment_active(pdev))
> +			return PCI_ERS_RESULT_NEED_RESET;

[Severity: Critical]
Is it safe to bypass device_release_driver() here?

By returning PCI_ERS_RESULT_NEED_RESET without unbinding the memdev driver,
the device remains fully exposed to userspace. During the Secondary Bus Reset,
the device is unresponsive. If a user issues an ioctl (like CXL_MEM_SEND_COMMAND)
or reads sysfs attributes during this window, the driver will perform MMIO reads
to the mailbox registers.

Could these non-posted reads hit uncontained Unsupported Requests while the
link is in reset and crash the system?

> +
>  		dev_warn(&pdev->dev,
>  			 "%s: frozen state error detected, disable CXL.mem\n",
>  			 dev_name(dev));
[ ... ]
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 2b779bd1d861b..ad74086bf82c7 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
[ ... ]
> @@ -174,7 +213,12 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>  	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>  			      PCI_EXP_DPC_STATUS_TRIGGER);

[Severity: Critical]
Could clearing the DPC Trigger Status here lead to fatal MMIO reads?

Right after this clears PCI_EXP_DPC_STATUS_TRIGGER (which pulls the port
out of DPC containment), __pci_bridge_secondary_bus_reset() is called.
This eventually invokes cxl_sbr_save_hdm_state(), which performs non-posted
MMIO reads to downstream endpoints:

drivers/cxl/core/dport_sbr.c:cxl_sbr_save_hdm_state() {
    ...
    scoped_guard(rwsem_read, &cxl_rwsem.region)
        rc = cxl_port_save_hdm_state(port, hdm_state);
}

Since the link is still down but DPC containment has just been lifted,
would the Root Port treat these reads as uncontained Completer Aborts
or Unsupported Requests and trigger a system crash?

>  
> -	if (pci_bridge_wait_for_secondary_bus(pdev, "DPC")) {
> +	if (is_cxl_dport(pdev) && cxl_port_dvsec(pdev))
> +		rc = __pci_bridge_secondary_bus_reset(pdev, CXL_SBR_UNBIND);

[Severity: High]
Are we missing a lock for the PCI config space here?

Calling __pci_bridge_secondary_bus_reset() without holding
pci_cfg_access_lock(pdev) triggers a lock assertion warning:

drivers/pci/pci.c:__pci_bridge_secondary_bus_reset() {
    if (!dev->block_cfg_access)
        pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n", ...);
}

Could running this unlocked allow concurrent operations (like sysfs resets
or hotplug) to corrupt the PCI config space during a DPC event?

> +	else
> +		rc = pci_bridge_wait_for_secondary_bus(pdev, "DPC");
> +
> +	if (rc) {
>  		clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
>  		ret = PCI_ERS_RESULT_DISCONNECT;
>  	} else {

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