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

[Vidya Sagar <vidyas@nvidia.com>]

On 25-08-2026 19:55, sashiko-bot@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The early `pci_msi_dev_inaccessible()` check in `pci_msix_write_tph_tag()` leaves a large race window across lock acquisitions, leading to an unconditional flush read on an inaccessible device.
The window is real and the placement was poor, so I will move the check.
Thanks.

msi_descs_lock is a mutex, so the caller can sleep between the check and
the flush read, which makes the window far wider than it needs to be.
Moving the check under irq_desc::lock, immediately before the tag update,
reduces it to a few instructions with interrupts disabled:

	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;
+
	FIELD_MODIFY(PCI_MSIX_ENTRY_CTRL_ST, &msi_desc->pci.msix_ctrl, tag);

To be clear about what this does and does not achieve: it narrows the
window, it does not close it. error_state is updated by
pci_dev_set_io_state() with a bare cmpxchg from the DPC handler, which
holds none of these locks, so containment can still begin between the
check and the access even when they are adjacent. This check is a filter
for the case where the kernel already knows the Link is down, not a
mutual exclusion primitive, and no placement can make it one. Closing the
race would require serialising every MSI-X access against containment.

> - [High] Silently skipping writes in `pci_msix_write_vector_ctrl()` exposes callers like `pci_write_msg_msix()` to partial MMIO updates and unconditional flush reads.
No. pci_write_msg_msix() has exactly one caller in the tree, and it is
already gated on the same predicate, so the function is not entered at
all when the device is inaccessible:

	if (dev->current_state != PCI_D0 || pci_msi_dev_inaccessible(dev)) {
		/* Don't touch the hardware now */
	} else if (entry->pci.msi_attrib.is_msix) {
		pci_write_msg_msix(entry, msg);

The inner check in pci_msix_write_vector_ctrl() is not redundant, because
that helper is also reached from pci_msix_unmask(),
__pci_restore_msix_state() and pci_msix_write_tph_tag(). But along the
pci_write_msg_msix() path it can only fire if error_state changes between
the outer and the inner check, which is the same unavoidable race as
above rather than anything this patch introduces.

Even then there is nothing to be partially updated. A partial update
would mean some writes reach the device and others do not, leaving the
Table inconsistent. Containment discards accesses uniformly: while the
Link is up every write lands, and once contained none of them do. The
transition only goes one way during an event, so there is no half-written
entry. On recovery __pci_restore_msix_state() rewrites address low,
address high, data and Vector Control from the cached descriptor, so the
entry is reconstructed in full:

	write_msg = arch_restore_msi_irqs(dev);

	scoped_guard (msi_descs_lock, &dev->dev) {
		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
			if (write_msg)
				__pci_write_msi_msg(entry, &entry->msg);
			pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl);
		}
	}

arch_restore_msi_irqs() returns true in the weak default and on s390, and
on x86 it is xen_initdom_restore_msi(), which only returns false under Xen
dom0 where Xen owns the restore. So on the platforms where native DPC
applies, write_msg is true and the message is rewritten along with the
control word.

I will send v3 with the TPH check moved.

Thanks,
Vidya Sagar
> --
> 
> --- Patch [1]: [PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained ---
> commit 170aedad1f36f8c51175a3888ba22b67ee4a98ef
> 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. This patch skips hardware accesses to
> the MSI-X table when the device is not in pci_channel_io_normal to prevent
> DPC escalations caused by Unsupported Requests. It also modifies
> pci_msix_write_tph_tag() to return -EIO to let the caller disable TPH.
> 
>> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
>> index 80a9db417dc87..0ebe153937ccc 100644
>> --- a/drivers/pci/msi/msi.c
>> +++ b/drivers/pci/msi/msi.c
> [ ... ]
>> @@ -954,6 +954,14 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
>>       if (!pdev->msix_enabled)
>>               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. Let the caller disable TPH instead.
>> +      */
>> +     if (pci_msi_dev_inaccessible(pdev))
>> +             return -EIO;
>> +
> 
> [Severity: High]
> Since this check happens before acquiring msi_descs_lock and irq_desc->lock
> in pci_msix_write_tph_tag(), does it leave a race window where the device
> could become inaccessible after the check?
> 
> If a DPC event occurs while waiting for the locks, it looks like the
> subsequent unconditional flush read:
> 
> drivers/pci/msi/msi.c:pci_msix_write_tph_tag() {
>     ...
>         /* Flush the write */
>         readl(pci_msix_desc_addr(msi_desc));
>     ...
> }
> 
> would still execute on an inaccessible device, potentially triggering the
> exact DPC escalation this patch aims to prevent.
> 
>>       virq = msi_get_virq(&pdev->dev, index);
>>       if (!virq)
>>               return -ENXIO;
>> 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
> [ ... ]
>> @@ -36,6 +50,9 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
>>  {
>>       void __iomem *desc_addr = pci_msix_desc_addr(desc);
>>
>> +     if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
>> +             return;
>> +
> 
> [Severity: High]
> If this silently skips the vector control write, does it expose callers like
> pci_write_msg_msix() to partial MMIO updates and unconditional flush reads?
> 
> Looking at pci_write_msg_msix(), if the mask write is silently skipped,
> would the subsequent address and data writes, as well as the flush read,
> still execute unconditionally:
> 
> drivers/pci/msi/msi.c:pci_write_msg_msix() {
>     ...
>         writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
>         writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
>         writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
> 
>         if (unmasked)
>                 pci_msix_write_vector_ctrl(desc, ctrl);
> 
>         /* Ensure that the writes are visible in the device */
>         readl(base + PCI_MSIX_ENTRY_DATA);
>     ...
> }
> 
> Could this trigger an Unsupported Request and the DPC escalation if the link
> is contained?
> 
>>       if (desc->pci.msi_attrib.can_mask)
>>               writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>>  }
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825140952.4066140-1-vidyas@nvidia.com?part=1