Re: [PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: Data race in pcie_set_readrq() corrupts hardware and saved state
--

commit 5cff2e9dcd50378ba11b9189a892c4623d8503cd
Author: Niklas Cassel <cassel@kernel.org>

PCI: Update saved Max Read Request Size in pcie_set_readrq()

This commit updates pcie_set_readrq() to also modify the kernel's saved copy
of the Device Control register when changing the Max Read Request Size. This
ensures that pci_restore_state() restores the intended MRRS value after a
device reset or error recovery.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5944,8 +5944,12 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>  
>  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>  						  PCI_EXP_DEVCTL_READRQ, v);
> +	if (ret)
> +		return pcibios_err_to_errno(ret);
>  
> -	return pcibios_err_to_errno(ret);
> +	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_READRQ, v);

[Severity: Medium]
Can this introduce a data race when called concurrently?

Looking at the newly added pcie_update_saved_devctl(), it performs an
unlocked read-modify-write on the saved state memory:

drivers/pci/pci.c:pcie_update_saved_devctl() {
        ...
        devctl = (u16 *)&save_state->cap.data[0];
        *devctl = (*devctl & ~clear) | set;
}

Both the hardware register write via pcie_capability_clear_and_set_word()
(which drops to the unlocked variant for PCI_EXP_DEVCTL) and this new state
update appear to happen without synchronization.

Concurrent access seems possible via VFIO, which allows parallel lockless
pwrite() calls to the PCI Express capability region. This routes through:

vfio_device_fops_write()
  vfio_pci_core_write()
    vfio_exp_config_write()
      pcie_set_readrq()

Could concurrent user-space writes corrupt both the hardware register and the
cached state memory, causing pci_restore_state() to mistakenly restore
invalid state later?

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(pcie_set_readrq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013195307.181647-2-cassel@kernel.org?part=1