Re: [RFC PATCH v1 1/1] vfio/pci: Revoke BARs and DMABUFs during sysfs-triggered PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Self-deadlock in config-space triggered FLR due to double acquisition of rw_semaphore.
- [Critical] Variant VFIO drivers bypass lock protection and DMABUF revocation during hardware resets.
- [Low] Unreachable `goto err_out` dead code in the lock unwinding path.
--

--- Patch [1]: [RFC PATCH v1 1/1] vfio/pci: Revoke BARs and DMABUFs during sysfs-triggered PCI reset ---
commit 40a15b781e5d64ee9f9f0c183f4f1df4fd3d6a7a
Author: Pranjal Shrivastava <praan@google.com>

vfio/pci: Revoke BARs and DMABUFs during sysfs-triggered PCI reset
    
Currently, vfio-pci does not implement the .reset_prepare and .reset_done
ops in its struct pci_error_handlers. During a sysfs-triggered PCI reset
(echo 1 > /sys/bus/pci/.../reset), the PCI core resets the device using
Function Level Reset (FLR) or Secondary Bus Reset (SBR), which isn't
propagated to the vfio-pci driver. Due to this, the exported DMABUFs and
BARs aren't zapped or revoked and the importer continues to use them.

Implement reset_prepare and reset_done hooks for the vfio-pci driver
that zap the BARs and revoke the exported DMABUFs while holding the
memory_lock.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a113c55845e10..e46b0b1d8a598 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1334,12 +1334,14 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
>  	 */
>  	vfio_pci_set_power_state(vdev, PCI_D0);
>  
> -	vfio_pci_dma_buf_move(vdev, true);
> -	ret = pci_try_reset_function(vdev->pdev);
> -	if (__vfio_pci_memory_enabled(vdev))
> -		vfio_pci_dma_buf_move(vdev, false);
> +	/*
> +	 * Drop the lock before entering the PCI core.
> +	 * The PCI core will re-acquire it via our .reset_prepare hook.
> +	 */
>  	up_write(&vdev->memory_lock);

[Severity: Critical]
Does this leave variant VFIO drivers unprotected during hardware resets?

The modified ioctl handlers now explicitly drop memory_lock and skip DMABUF
revocation, relying on the PCI core to invoke the new .reset_prepare hook
to handle these tasks.

However, vfio_pci_core_reset_prepare() is declared statically below, making
it unavailable to variant drivers like mlx5vf or nvgrace_gpu that define
their own pci_error_handlers.

When variant drivers undergo a reset, won't the PCI core invoke a NULL or
unrelated reset_prepare hook, resulting in devices being completely
unlocked and DMABUFs unrevoked during the reset?

>  
> +	ret = pci_try_reset_function(vdev->pdev);
> +
>  	return ret;
>  }
>  

[ ... ]

> @@ -2418,8 +2420,33 @@ int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
>  
> +static void vfio_pci_core_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> +
> +	if (vdev) {
> +		down_write(&vdev->memory_lock);

[Severity: Critical]
Could this cause a self-deadlock when a Function Level Reset is triggered
through the PCI config space?

In drivers/vfio/pci/vfio_pci_config.c, both vfio_exp_config_write() and
vfio_af_config_write() acquire the lock before initiating the reset:

vfio_exp_config_write() {
    ...
    vfio_pci_zap_and_down_write_memory_lock(vdev);
    vfio_pci_dma_buf_move(vdev, true);
    pci_try_reset_function(vdev->pdev);
    ...
}

Since pci_try_reset_function() propagates down to this new .reset_prepare
hook, won't calling down_write() again on the same memory_lock lead to
an indefinite hang, as rw_semaphores are not recursive?

> +		vfio_pci_set_power_state(vdev, PCI_D0);
> +		vfio_pci_zap_bars(vdev);
> +		vfio_pci_dma_buf_move(vdev, true);
> +	}
> +}

[ ... ]

> @@ -2561,55 +2588,52 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  
>  		if (!owned) {
>  			ret = -EINVAL;
> -			break;
> +			goto err_out;
>  		}
> +	}
>  
> +	/* Zap, and set power state */
> +	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list) {
>  		/*
> -		 * Take the memory write lock for each device and zap BAR
> -		 * mappings to prevent the user accessing the device while in
> -		 * reset.  Locking multiple devices is prone to deadlock,
> +		 * Take the memory write lock for each device, zap BAR
> +		 * mappings, and restore power state before reset.
> +		 * Locking multiple devices is prone to deadlock,
>  		 * runaway and unwind if we hit contention.
>  		 */
>  		if (!down_write_trylock(&vdev->memory_lock)) {
>  			ret = -EBUSY;
> -			break;
> +
> +			/*
> +			 * We failed to lock THIS device. We must step back one
> +			 * device so err_undo only unlocks devices that succeeded.
> +			 */
> +			if (!list_entry_is_head(vdev, &dev_set->device_list, vdev.dev_set_list)) {
> +				vdev = list_prev_entry(vdev, vdev.dev_set_list);
> +				goto err_undo;
> +			}
> +			goto err_out;
>  		}

[Severity: Low]
Is the goto err_out jump here unreachable dead code?

Because vdev is the current iterator inside the list_for_each_entry() loop,
it can never be the list head. The !list_entry_is_head() condition will
always evaluate to true, meaning the failure path will unconditionally
take the goto err_undo jump.

>  
> -		vfio_pci_dma_buf_move(vdev, true);
>  		vfio_pci_zap_bars(vdev);
> +		vfio_pci_set_power_state(vdev, PCI_D0);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807201405.3717430-2-praan@google.com?part=1