Re: [PATCH] PCI: Accept AtomicOps already enabled by the hypervisor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditionally returning success when ATOMIC_REQ is already set bypasses root port and bridge capability validation.
--

commit 174dddecfad7749ccba344bb23492b342569e86c
Author: Nikola Prica <nikola.prica@amd.com>

PCI: Accept AtomicOps already enabled by the hypervisor

This commit modifies pci_enable_atomic_ops_to_root() to check if the
PCI_EXP_DEVCTL2_ATOMIC_REQ bit is already set by a hypervisor. If it is, the
function returns success early to avoid failing in passthrough topologies
without a visible Root Port.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..7c62c7f317572 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3768,6 +3768,14 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Some hypervisors already set AtomicOp Requester Enable.  If it
> +	 * is already set, there is nothing more to do.
> +	 */
> +	pcie_capability_read_dword(dev, PCI_EXP_DEVCTL2, &ctl2);
> +	if (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_REQ)
> +		return 0;

[Severity: High]
Will this unconditional early return bypass the capability validation for newly
requested atomic sizes?

When drivers or subsystems sequentially check for atomic support, they call
pci_enable_atomic_ops_to_root() for different sizes. For example, in
drivers/vfio/pci/vfio_pci_core.c:vfio_pci_info_atomic_cap() (around line
1059), the code sequentially checks for 32-bit and 64-bit atomic support:

vfio_pci_core.c:vfio_pci_info_atomic_cap() {
    ...
    if ((devcap2 & PCI_EXP_DEVCAP2_ATOMIC_COMP64) &&
        !pci_enable_atomic_ops_to_root(pdev, PCI_EXP_DEVCAP2_ATOMIC_COMP64))
        cap.flags |= VFIO_PCI_ATOMIC_COMP64;
    ...
}

A similar sequential check occurs in
drivers/infiniband/hw/bnxt_re/qplib_res.c:bnxt_qplib_determine_atomics()
(around line 1001).

The first successful call (e.g., for COMP32) validates the topology and sets
the ATOMIC_REQ bit on the endpoint. If a subsequent call is made for a larger
size (like COMP64), this new logic sees the ATOMIC_REQ bit is already set and
instantly returns success.

By returning early, doesn't this bypass the cap_mask checks on the root port
and bridges for the larger atomic size?

Could this cause the kernel to incorrectly advertise 64-bit or 128-bit atomic
support to VMs or drivers when the root port only supports 32-bit? If a driver
or VM attempts to use these unsupported sizes, the root port would treat them
as an Unsupported Request (UR), potentially triggering a fatal AER error and
crashing the system or terminating the VM.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903121218.4955-1-nikprica@amd.com?part=1