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

[sashiko-bot@kernel.org]

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?

> 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611182528.4004073-1-alex.williamson@nvidia.com?part=2