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

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

[Pranjal Shrivastava]

Introduce PCI .reset_prepare and .reset_done handlers to safely revoke
active userspace mappings and exported DMABUFs during sysfs-triggered
device resets.

We are seeing a situation where system health and monitoring daemons 
(at times erroneously) issue device resets via sysfs for devices bound
to vfio-pci:

  echo 1 > /sys/bus/pci/devices/0000:01:00.0/reset

However, because vfio-pci does not implement the .reset_prepare and
.reset_done error handlers, this hardware reset occurs completely unnoticed
by the VFIO driver.

Consequently, active traditional userspace BAR mappings and exported DMABUFs
are never zapped or revoked. Importers of the DMABUFs (e.g., RDMA drivers)
continue to issue DMAs (such as PCIe Memory Writes) toward the Endpoint. 
These transactions are silently dropped by the root port or trigger CTOs 
while higher-level actions (e.g., RDMA reg_mr) continue to succeed.

We'd like to fix this by implementing the PCI reset ops for vfio-pci
that revoke the DMABUFs and zap the BARs while holding the memory lock
allowing concurrent user accesses to sleep and fault back in once the reset
completes.

Note: I've tried to handle the locking as a first attempt here, there
might've been some cases that were missed. Also, for the RFC, the
drivers that implement their own pci_error_handlers (like nvgrace) are 
not altered for now.

Quick Note about Matt's DMABUF mmap Series
==========================================
While this patch is aimed for the current upstream code, I believe with
Matt's refactor [1] these ops might change slightly. If we have consensus
on this patch, I'd send another patch based to Matt based on their series
for them to include it in their next version.

[1] https://lore.kernel.org/all/20260715174737.15287-1-matt@ozlabs.org/

Thanks,
Praan

Pranjal Shrivastava (1):
  vfio/pci: Revoke BARs and DMABUFs during sysfs-triggered PCI reset

 drivers/vfio/pci/vfio_pci_core.c | 88 ++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 32 deletions(-)

-- 
2.55.0.679.g6767b8d81c-goog

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

[Pranjal Shrivastava]

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.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 88 ++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index a113c55845e1..e46b0b1d8a59 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);
 
+	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);
+		vfio_pci_set_power_state(vdev, PCI_D0);
+		vfio_pci_zap_bars(vdev);
+		vfio_pci_dma_buf_move(vdev, true);
+	}
+}
+
+static void vfio_pci_core_reset_done(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
+
+	if (vdev) {
+		if (__vfio_pci_memory_enabled(vdev))
+			vfio_pci_dma_buf_move(vdev, false);
+		up_write(&vdev->memory_lock);
+	}
+}
+
 const struct pci_error_handlers vfio_pci_core_err_handlers = {
 	.error_detected = vfio_pci_core_aer_err_detected,
+	.reset_prepare = vfio_pci_core_reset_prepare,
+	.reset_done = vfio_pci_core_reset_done,
 };
 EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);
 
@@ -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;
 		}
 
-		vfio_pci_dma_buf_move(vdev, true);
 		vfio_pci_zap_bars(vdev);
+		vfio_pci_set_power_state(vdev, PCI_D0);
 	}
 
-	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;
-	}
-
-	/*
-	 * The pci_reset_bus() will reset all the devices in the bus.
-	 * The power state can be non-D0 for some of the devices in the bus.
-	 * For these devices, the pci_reset_bus() will internally set
-	 * the power state to D0 without vfio driver involvement.
-	 * For the devices which have NoSoftRst-, the reset function can
-	 * cause the PCI config space reset without restoring the original
-	 * state (saved locally in 'vdev->pm_save').
-	 */
+	/* Drop locks before crossing into PCI core */
 	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
-		vfio_pci_set_power_state(vdev, PCI_D0);
+		up_write(&vdev->memory_lock);
 
+	/* PCI core handles the reset and calls .reset hooks */
 	ret = pci_reset_bus(pdev);
 
-	vdev = list_last_entry(&dev_set->device_list,
-			       struct vfio_pci_core_device, vdev.dev_set_list);
+	goto err_out;
 
 err_undo:
+	/* Unwind locks cleanly for devices we successfully locked */
 	list_for_each_entry_from_reverse(vdev, &dev_set->device_list,
-					 vdev.dev_set_list) {
-		if (vdev->vdev.open_count && __vfio_pci_memory_enabled(vdev))
-			vfio_pci_dma_buf_move(vdev, false);
+					 vdev.dev_set_list)
 		up_write(&vdev->memory_lock);
-	}
 
+err_out:
 	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
 		pm_runtime_put(&vdev->pdev->dev);
 
-- 
2.55.0.679.g6767b8d81c-goog

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

[sashiko-bot]

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