* Re: [RFC PATCH 1/1] vfio/pci: Disable sriov on PF device close
2026-08-05 0:33 ` [RFC PATCH 1/1] " Samiullah Khawaja
@ 2026-08-05 1:03 ` sashiko-bot
2026-08-05 9:34 ` Tian, Kevin
2026-08-05 15:03 ` Alex Williamson
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-08-05 1:03 UTC (permalink / raw)
To: Samiullah Khawaja; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Acquiring `device_lock()` inside `vfio_pci_core_close_device()` introduces a guaranteed AB-BA deadlock if the device is unbound from the driver while still in use by userspace.
- [High] Synchronously tearing down SR-IOV VFs from the PF's `close_device` path deadlocks if any VF is actively in use by VFIO.
- [High] Calling `pci_disable_sriov()` without holding `pci_rescan_remove_lock` causes an immediate lockdep splat and races with PCI bus enumeration.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) races around `device_lock()` allow VFs to be enabled immediately before hardware reset, defeating the purpose of the patch.
--
--- Patch [1]: [RFC PATCH 1/1] vfio/pci: Disable sriov on PF device close ---
commit 996eca93815cca91725d215b78730f993e057c49
Author: Samiullah Khawaja <skhawaja@google.com>
vfio/pci: Disable sriov on PF device close
When a user closes a VFIO device file descriptor for an SR-IOV Physical
Function, the device is reset. This reset disrupts the VFs without notifying
the core PCI driver model. This commit disables SR-IOV prior to issuing the
PF reset so VFs are torn down gracefully at the software level.
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a113c55845e10..b36c77eb3c407 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -826,6 +826,13 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> #if IS_ENABLED(CONFIG_EEH)
> eeh_dev_release(vdev->pdev);
> #endif
> +
> + if (pci_num_vf(vdev->pdev)) {
[Severity: Medium]
Is it possible for VFs to be enabled concurrently right after this check?
Since pci_num_vf() is evaluated locklessly before acquiring the device lock, a
concurrent sysfs write to sriov_numvfs could enable VFs immediately after this
returns 0, causing the teardown to be skipped.
Additionally, because the device lock is dropped before the hardware reset in
vfio_pci_core_disable(), a concurrent sysfs write can acquire the lock,
successfully enable SR-IOV, and release the lock before the reset occurs.
Could this recreate the exact hardware state mismatch the patch attempts to fix?
> + device_lock(&vdev->pdev->dev);
[Severity: High]
Does acquiring the device lock here introduce an AB-BA deadlock during device
unbind?
When a device is unbound, the PCI core acquires the device lock in
device_release_driver() and invokes the driver's remove callback. For
vfio-pci, this calls vfio_unregister_group_dev(), which blocks indefinitely
waiting for userspace to close all active file descriptors.
When userspace closes the device fd, vfio_pci_core_close_device() is invoked.
Because this patch tries to acquire the device lock, the fd release thread
will block forever waiting for the unbind thread, and the unbind thread will
wait forever for the fd release thread to finish.
> + vfio_pci_core_sriov_configure(vdev, 0);
[Severity: High]
Can tearing down VFs from the PF's close path deadlock if a VF is actively in
use by VFIO?
When vfio_pci_core_close_device() calls pci_disable_sriov(), the PCI core
synchronously removes all VF devices from the bus. If a VF is also bound to
vfio-pci and currently opened by userspace, removing the VF triggers
vfio_pci_remove(), which blocks indefinitely until the VF file descriptor is
closed.
If a single process holds both the PF and VF open and exits, exit_files()
schedules the fput() for both file descriptors sequentially. If the PF is
closed first, the thread hangs here waiting for the VF to close, preventing
the VF's fput() from ever running.
[Severity: High]
Does this code trigger a lockdep warning by manipulating the PCI bus topology
without the required lock?
Calling vfio_pci_core_sriov_configure() here ultimately calls
pci_stop_and_remove_bus_device() to remove VFs from the PCI bus. That function
enforces a strict locking requirement via
lockdep_assert_held(&pci_rescan_remove_lock).
Because this global mutex is omitted, closing a PF with active VFs could race
against concurrent PCI hotplug or sysfs rescan operations.
> + device_unlock(&vdev->pdev->dev);
> + }
> +
> vfio_pci_dma_buf_cleanup(vdev);
>
> vfio_pci_core_disable(vdev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805003355.728299-2-skhawaja@google.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [RFC PATCH 1/1] vfio/pci: Disable sriov on PF device close
2026-08-05 0:33 ` [RFC PATCH 1/1] " Samiullah Khawaja
2026-08-05 1:03 ` sashiko-bot
@ 2026-08-05 9:34 ` Tian, Kevin
2026-08-05 15:03 ` Alex Williamson
2 siblings, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2026-08-05 9:34 UTC (permalink / raw)
To: Samiullah Khawaja, Alex Williamson, Jason Gunthorpe,
bhelgaas@google.com
Cc: Leon Romanovsky, dmatlack@google.com, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org
> From: Samiullah Khawaja <skhawaja@google.com>
> Sent: Wednesday, August 5, 2026 8:34 AM
>
> When userspace closes a VFIO device file descriptor, the vfio driver
> performs a hardware reset on the PCIe device to ensure it is returned to
> a clean state. However, if the closed device is an SR-IOV Physical
> Function (PF), it may have instantiated Virtual Functions (VFs) that are
> actively bound to host kernel drivers (or other vfio instances).
>
> When the PF is hardware-reset via VFIO, it implicitly disrupts SR-IOV
> operations at the device level. Because this reset happens without notifying
> the core PCI driver model, the kernel drivers bound to the VFs remain loaded
> and operate under the assumption that the VF hardware is still functional.
>
> This creates a state mismatch between the kernel's view of the hardware
> and the actual device state. Subsequent attempts by the host OS or bound
> drivers to interact with the VFs will fail, leading to unexpected
> errors.
>
> Disable SR-IOV on the device prior to issuing the PF reset so that the
> VFs can teardown and remove at the software level also.
>
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> ---
> drivers/vfio/pci/vfio_pci_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a113c55845e1..b36c77eb3c40 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -826,6 +826,13 @@ void vfio_pci_core_close_device(struct vfio_device
> *core_vdev)
> #if IS_ENABLED(CONFIG_EEH)
> eeh_dev_release(vdev->pdev);
> #endif
> +
> + if (pci_num_vf(vdev->pdev)) {
> + device_lock(&vdev->pdev->dev);
> + vfio_pci_core_sriov_configure(vdev, 0);
> + device_unlock(&vdev->pdev->dev);
> + }
> +
Sashiko reported several locking issues with this change:
https://sashiko.dev/#/patchset/20260805003355.728299-2-skhawaja%40google.com
Actually it is a discouraged usage allowing kernel drivers bound to VFs
which belongs to a PF owned by vfio userspace.
vfio_pci_bus_notifier prints a warning message upon
BUS_NOTIFY_BOUND_DRIVER:
"VF %s bound to driver %s while PF bound to driver %s\n"
Fixing this alone is insufficient to remove that warning...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/1] vfio/pci: Disable sriov on PF device close
2026-08-05 0:33 ` [RFC PATCH 1/1] " Samiullah Khawaja
2026-08-05 1:03 ` sashiko-bot
2026-08-05 9:34 ` Tian, Kevin
@ 2026-08-05 15:03 ` Alex Williamson
2026-08-06 19:47 ` Jason Gunthorpe
2 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2026-08-05 15:03 UTC (permalink / raw)
To: Samiullah Khawaja
Cc: Jason Gunthorpe, bhelgaas, Kevin Tian, Leon Romanovsky, dmatlack,
kvm, linux-kernel, alex
On Wed, 5 Aug 2026 00:33:55 +0000
Samiullah Khawaja <skhawaja@google.com> wrote:
> When userspace closes a VFIO device file descriptor, the vfio driver
> performs a hardware reset on the PCIe device to ensure it is returned to
> a clean state. However, if the closed device is an SR-IOV Physical
> Function (PF), it may have instantiated Virtual Functions (VFs) that are
> actively bound to host kernel drivers (or other vfio instances).
Wait, what? We actively try to prevent VFs from a vfio-pci owned PF
from being bound to host drivers other than vfio-pci. You need to
overwrite the imposed driver_override to make this happen and you're in
a very precarious security model to have the VF owned by a trusted
in-kernel driver while the PF is owned by userspace.
> When the PF is hardware-reset via VFIO, it implicitly disrupts SR-IOV
> operations at the device level. Because this reset happens without notifying
> the core PCI driver model, the kernel drivers bound to the VFs remain loaded
> and operate under the assumption that the VF hardware is still functional.
And this is a) why we actively try to prevent VFs from being bound to
native host kernel drivers and b) part of the vf_token negotiation of
trust opt-in that must be performed by userspace drivers of the VFs.
> This creates a state mismatch between the kernel's view of the hardware
> and the actual device state. Subsequent attempts by the host OS or bound
> drivers to interact with the VFs will fail, leading to unexpected
> errors.
The described model is not a supported use case, there are fundamental
security issues with a userspace vfio-pci driver owning the PF with VFs
bound to in-kernel drivers.
> Disable SR-IOV on the device prior to issuing the PF reset so that the
> VFs can teardown and remove at the software level also.
The below only accounts for the close reset path, not reset in general.
Also note that the SR-IOV model in vfio-pci does have a path by which a
PF can be re-opened with existing VFs. Whether that's actually used is
a separate question, but this fundamentally disallows that design.
It's possible there are gaps that closing the PF can interrupt the VFs
and we need to defer a reset until the VFs are closed, but the behavior
here seems counter to the original design, and the use case of
in-kernel VF drivers is certainly counter to that design and guardrails
installed to prevent it. Thanks,
Alex
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> ---
> drivers/vfio/pci/vfio_pci_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a113c55845e1..b36c77eb3c40 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -826,6 +826,13 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> #if IS_ENABLED(CONFIG_EEH)
> eeh_dev_release(vdev->pdev);
> #endif
> +
> + if (pci_num_vf(vdev->pdev)) {
> + device_lock(&vdev->pdev->dev);
> + vfio_pci_core_sriov_configure(vdev, 0);
> + device_unlock(&vdev->pdev->dev);
> + }
> +
> vfio_pci_dma_buf_cleanup(vdev);
>
> vfio_pci_core_disable(vdev);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/1] vfio/pci: Disable sriov on PF device close
2026-08-05 15:03 ` Alex Williamson
@ 2026-08-06 19:47 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2026-08-06 19:47 UTC (permalink / raw)
To: Alex Williamson
Cc: Samiullah Khawaja, bhelgaas, Kevin Tian, Leon Romanovsky,
dmatlack, kvm, linux-kernel
On Wed, Aug 05, 2026 at 09:03:23AM -0600, Alex Williamson wrote:
> On Wed, 5 Aug 2026 00:33:55 +0000
> Samiullah Khawaja <skhawaja@google.com> wrote:
>
> > When userspace closes a VFIO device file descriptor, the vfio driver
> > performs a hardware reset on the PCIe device to ensure it is returned to
> > a clean state. However, if the closed device is an SR-IOV Physical
> > Function (PF), it may have instantiated Virtual Functions (VFs) that are
> > actively bound to host kernel drivers (or other vfio instances).
>
> Wait, what? We actively try to prevent VFs from a vfio-pci owned PF
> from being bound to host drivers other than vfio-pci. You need to
> overwrite the imposed driver_override to make this happen and you're in
> a very precarious security model to have the VF owned by a trusted
> in-kernel driver while the PF is owned by userspace.
Maybe, it really depends on the device. I can easially see someone
using a device where this would be safe. mlx5 for instance is pretty
OK.
So I don't really mind someone doing this, we should block it and warn
it and so on, but like noiommu and the other vfio insecure modes, why
not give an opt in?
> It's possible there are gaps that closing the PF can interrupt the VFs
> and we need to defer a reset until the VFs are closed,
Oh definately, when running in a SRIOV mode it is really problematic
for the PF to reset while there are any active VFs. The PF controls a
number of shared items (MMIO, ATS, etc) and when it blips everyone is
at risk of unexpected fairly catastrophic system crashing errors
related to the shared items going away.
So resetting the PF device unconditionally when vfio closes is
definately wrong in principal. I can see it maybe working for simple
systems, especially ones that don't MCE..
I don't think we can skip the PF reset on close because of dev_set
reasons and leave a rouge device for the next user, so the thing looks
somewhat troubled?
Adding the sriov disable here at least makes it defensibly safe, that
we do still clear the PF on close, and we don't take risks that the
active VFs will crash the system during the FLR blip.
Though I understand it was not the original intention, I feel we have
ended up in a strange place with the SRIOV PF feature ..
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread