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

[Niklas Cassel <cassel@kernel.org>]

On Tue, Oct 13, 2026 at 08:05:19PM +0000, sashiko-bot@kernel.org wrote:
> 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?

No.  As far as I can tell, concurrent pcie_set_readrq() calls on the
same device are only possible via VFIO, when user space races config
writes to Device Control of its own device.  That path already does
unlocked read-modify-writes of the same register in
vfio_default_config_write(), and pcie_cap_lock does not cover Device
Control, so that race is not new.

pcie_update_saved_devctl() only modifies the MRRS field of the saved
copy, with an aligned 16-bit store, so the worst case is that the saved
MRRS is the value of the other racing write, which also passed the
checks in pcie_set_readrq().  The saved state cannot become invalid.
Also, vfio-pci never restores the live saved state without first saving
it or loading its own stored copy.

The already queued patch "PCI: Update saved Max Payload Size in
pcie_set_mps()" is not affected: VFIO virtualizes MPS and never calls
pcie_set_mps(), and all other callers run during enumeration or from a
driver's probe().

So I don't think either patch needs additional locking.


Kind regards,
Niklas