Re: [PATCH V3] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unintended Side-effect / Triggered DPC Escalation in Error Path
- [Medium] Race condition and lockless access of pdev->error_state

Pre-existing issues:
- [High] Incomplete Fix / Unhandled Configuration Space Access during DPC Containment
--

commit 1da77665cdebed45957e3d9c2c58ae00d2dbc9b0
Author: Vidya Sagar <vidyas@nvidia.com>

PCI/MSI: Don't touch the MSI-X table while the Link is contained
    
The MSI-X Table lives in device MMIO space behind a BAR, so it is only
reachable while the Link is up. While a Downstream Port has the Link
contained by DPC it completes accesses to the Table with Unsupported
Request, and reads return all ones.

[ ... ]

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 80a9db417dc87..975948502f8e8 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c

[ ... ]

> @@ -976,6 +976,15 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
>  	if (!msi_desc || msi_desc->pci.msi_attrib.is_virtual)
>  		return -ENXIO;
>  
> +	/*
> +	 * The tag update below is a write to the MSI-X Table followed by a
> +	 * flush read, neither of which can be completed while the Link is
> +	 * contained. Check as late as possible, i.e. under irq_desc::lock, as
> +	 * containment can begin at any point. Let the caller disable TPH.
> +	 */
> +	if (pci_msi_dev_inaccessible(pdev))
> +		return -EIO;

[Severity: High]
Does returning -EIO here actively trigger a fatal Configuration Space write
that escalates DPC containment?

When the link is contained by DPC, returning -EIO forces the caller
(pcie_tph_set_st_entry()) into an error path that invokes pcie_disable_tph().
This function unconditionally writes to the device's Configuration Space:

drivers/pci/tph.c:pcie_disable_tph() {
    ...
    pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, 0);
}

Additionally, the caller performs an unguarded config write before the MMIO
access via set_ctrl_reg_req_en(..., PCI_TPH_REQ_DISABLE).

Does this newly introduced early error return actively force the caller to
perform actions that reach the hardware and escalate the DPC event?

> +
>  	FIELD_MODIFY(PCI_MSIX_ENTRY_CTRL_ST, &msi_desc->pci.msix_ctrl, tag);
>  	pci_msix_write_vector_ctrl(msi_desc, msi_desc->pci.msix_ctrl);
>  	/* Flush the write */

[ ... ]

> diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
> index 0b420b319f50f..c3194d8425c88 100644
> --- a/drivers/pci/msi/msi.h
> +++ b/drivers/pci/msi/msi.h
> @@ -26,6 +26,20 @@ static inline void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
>  	return desc->pci.mask_base + desc->msi_index * PCI_MSIX_ENTRY_SIZE;
>  }
>  
> +/*
> + * The MSI-X Table lives in device MMIO space and the MSI Capability in
> + * Configuration Space, so both are only reachable while the Link is usable.
> + * While a Downstream Port has the Link contained by DPC it completes these
> + * accesses with Unsupported Request. If the upstream Root Port implements the
> + * RP Extensions for DPC, it reports that completion as an RP PIO error and
> + * answers with a DPC of its own, taking down every other device below it.
> + */

[Severity: High]
This isn't a bug introduced by this patch, but because the patch attempts to
fix DPC escalation by guarding MMIO, should it also guard Configuration Space
accesses during teardown?

For example, when pci_free_irq_vectors() initiates teardown during DPC
containment (error_state == pci_channel_io_frozen), it calls
pci_msix_shutdown(), pci_msi_shutdown(), or pci_msi_update_mask(). These
perform unguarded Configuration Space accesses. In pci_msi_update_mask():

drivers/pci/msi/msi.c:pci_msi_update_mask() {
    ...
    pci_write_config_dword(dev, desc->pci.mask_pos, desc->pci.msi_mask);
}

Crucially, Configuration Space accesses are not blocked when
error_state == pci_channel_io_frozen. As a result, the accesses reach the
hardware, trigger Unsupported Request (UR) completions, and escalate the DPC
event.

Is this fix fatally incomplete without similarly guarding Configuration Space
accesses?

> +static inline bool pci_msi_dev_inaccessible(struct pci_dev *pdev)
> +{
> +	return pdev->error_state != pci_channel_io_normal ||

[Severity: Medium]
Could this lockless read of pdev->error_state be subject to compiler
optimizations like load tearing or caching, leading to missed DPC
containment detection?

Concurrent atomic updates to pdev->error_state are performed in
pci_dev_set_io_state() via xchg()/cmpxchg(). However,
pci_msi_dev_inaccessible() reads it locklessly without using READ_ONCE().

According to the Linux kernel memory model, does this constitute a data race
where the compiler could cache the value, falsely returning that the device
is accessible and leading to fatal MMIO accesses during DPC containment?

> +	       pci_dev_is_disconnected(pdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825172719.4153402-1-vidyas@nvidia.com?part=1