Re: [PATCH 2/2] vfio/pci: Latch all module parameters per device

[Alex Williamson <alex.williamson@nvidia.com>]

On Thu, 11 Jun 2026 18:44:28 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] VGA Arbiter NULL Pointer Dereference on Error Path
> 
> Pre-existing issues:
> - [High] Bitfield data race breaking device interrupts and Runtime PM
> --
> 
> commit 26787ee0fbb65453cacc903b9b0170155208490a
> Author: Alex Williamson <alex.williamson@nvidia.com>
> 
> vfio/pci: Latch all module parameters per device
> 
> The vfio-pci module parameters of disable_idle_d3, nointxmask, and
> disable_vga latch vfio-pci policy into vfio-pci-core globals each time
> the vfio-pci module is initialized. The disable_idle_d3 parameter has
> already migrated to a per-device flag in order to provide consistency
> for refcounted PM operations for the lifetime of the device registration.
> 
> Pull the remaining vfio-pci module-parameter policy out of vfio-pci-core
> into per-device flags set at device initialization.
> 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index 9f71eae0cc947..61cf1a1996b0d 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c  
> 
> [ ... ]
> 
> > @@ -111,11 +107,12 @@ static inline bool vfio_vga_disabled(void)
> >   */
> >  static unsigned int vfio_pci_set_decode(struct pci_dev *pdev, bool single_vga)
> >  {
> > +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> >  	struct pci_dev *tmp = NULL;
> >  	unsigned char max_busnr;
> >  	unsigned int decodes;
> >  
> > -	if (single_vga || !vfio_vga_disabled() || pci_is_root_bus(pdev->bus))
> > +	if (single_vga || !vfio_vga_disabled(vdev) || pci_is_root_bus(pdev->bus))  
> 
> [Severity: High]
> If vfio_pci_core_register_device() fails on vfio_register_group_dev(), the
> error path skips calling vfio_pci_vga_uninit():
> 
> 	out_power:
> 		if (!vdev->disable_idle_d3)
> 			pm_runtime_get_noresume(dev);
> 
> 		pm_runtime_forbid(dev);
> 	out_vf:
> 		vfio_pci_vf_uninit(vdev);
> 		return ret;
> 
> This leaves the device registered with the VGA arbiter. After the probe fails,
> the driver core clears the drvdata.
> 
> Will the VGA arbiter later invoke this callback, causing dev_get_drvdata() to
> return NULL and vfio_vga_disabled(vdev) to panic when dereferencing vdev?

Tricky, preexisting lack of unwind becomes a NULL deref with per device
flags.  Will fix.
 
> > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> > index 27aab3fdbb91d..74b950d73aef0 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h  
> 
> [ ... ]
> 
> > @@ -128,6 +128,8 @@ struct vfio_pci_core_device {
> >  	bool			pm_intx_masked:1;
> >  	bool			pm_runtime_engaged:1;
> >  	bool			disable_idle_d3:1;
> > +	bool			nointxmask:1;
> > +	bool			disable_vga:1;
> >  	bool			sriov_active;  
> 
> [Severity: High]
> This is a pre-existing issue, but this patch extends the bitfield block that
> has a data race.
> 
> vdev->pm_intx_masked and vdev->pm_runtime_engaged are adjacent bitfields.
> 
> In vfio_pci_core_runtime_suspend(), pm_intx_masked is updated without holding
> vdev->memory_lock:
> 
> 	vdev->pm_intx_masked = ((vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX) &&
> 				vfio_pci_intx_mask(vdev));
> 
> Concurrently, userspace can call __vfio_pci_runtime_pm_exit(), which takes
> vdev->memory_lock and updates pm_runtime_engaged:
> 
> 	if (vdev->pm_runtime_engaged) {
> 		vdev->pm_runtime_engaged = false;
> 
> Because these bitfields share the same memory byte and are updated without a
> shared lock, will the read-modify-write operations race and potentially lose
> one of the updates, permanently breaking device interrupts or low-power state
> entry?
> 

Sigh, yes, I need to get back to this too.  Thanks,

Alex