[PATCH v2 0/4] vfio: Fix cdev second-open and harden selftests

[PATCH v2 0/4] vfio: Fix cdev second-open and harden selftests

[Alex Williamson]

v2:
 - Re-try on -EBUSY reworked to harness issue and posted separately[1],
   dropped here.
 - vfio selftests subjects updated for consistency, Reviewed-bys
   incorporated, Assisted-bys updated to current standards.
 - Added the selftest suggested by David, but split it into two tests,
   one that validates vf_token is not clobbered regardless of the 2nd
   bind errno, and another that enforces the -EBUSY expected errno.
   This allows us to observe that an old kernel fails both and avoids
   conflating the uAPI expectation vs the underlying data clobber.

[1]https://lore.kernel.org/all/20260910230254.1198094-1-alex.williamson@nvidia.com/

v1:

In porting some testing infrastructure to a different system I found
the igb selftest failing in mix_and_match and generating a cascade
failure through the remaining tests.  The difference in the new system
is the firmware error handling.  When the igb device gets wedged due
to bad DMA in mix_and_match, we need to FLR the device.  The igb holds
transaction pending asserted for the full duration of
pci_wait_for_pending().  Meanwhile, firmware based error handling is
triggering SMIs and stealing time, such that the 700ms total delay
in pci_wait_for_pending() turns into several seconds.  With 10 cases
in mix_and_match generating bad DMAs, the transaction pending delays
alone push us close to the 30s per-test timeout.  The worst case I
observed for the total test was ~45s.  Increasing the mix_and_match
timeout to 90s provides plenty of headroom to get a passing test and
avoid the cascade failure.

When the process is killed via timeout, the release occurs through a
scheduled fput(), which is also delayed by the SMI storm.  Each
subsequent test then sees a non-zero open_count (for the group open
in legacy mode or on the iommufd bind in the cdev mode), resulting in
the cascade failure.  Group mode already uses -EBUSY when the group
fd open count is elevated, which allows selftests to interpret the
failure as potentially transient and implement a bounded retry.  The
cdev path instead returns -EINVAL for this case.  -EBUSY seems
justified here and allows userspace to have compatible retry flows
for group open and cdev bind operations.

A local sashiko review then found two existing issues.  First, in
analyzing the exit flow from the iommufd bind, we can see that the
vf_token and kvm pointers are clobbered by the second process before
the open count test.  The open_count test in vfio_df_open() is only
for the cdev path (!df->group) and is called under the dev_set lock,
so we really only need to relocate the test to
vfio_df_ioctl_bind_iommufd() prior to vf_token/kvm manipulation.

The second existing issue is that when executed via the kselftest
runner, all tests have a 45s timeout, which is the cumulative time
across each sub-test of the execution.  The pci_driver test already
fails this with ioatdma, nv_falcon, and obviously with physical igb.
Running in parallel across both ports of an igb on the system prone
to SMI overhead, the worst case I saw was 450s.  Therefore, we not
only need to extend the mix_and_match timeout to 90s to handle the
extra transaction pending delay, we need to extend the default
timeout to allow the full pci-driver test to complete when executed
via the runner.  600s is picked here as a "sufficient" margin.

Please review and comment.  Thanks,

Alex

Alex Williamson (4):
  vfio: Reject a second cdev open before mutating shared device state
  vfio: selftests: Verify a failed second open preserves the vf_token
  vfio: selftests: Extend mix_and_match timeout to 90s
  vfio: selftests: Extend timeout for runner executions

 drivers/vfio/device_cdev.c                    | 12 ++++
 drivers/vfio/vfio_main.c                      |  7 ---
 tools/testing/selftests/vfio/.gitignore       |  1 +
 tools/testing/selftests/vfio/settings         |  5 ++
 .../selftests/vfio/vfio_pci_driver_test.c     |  2 +-
 .../selftests/vfio/vfio_pci_sriov_uapi_test.c | 57 +++++++++++++++++++
 6 files changed, 76 insertions(+), 8 deletions(-)
 create mode 100644 tools/testing/selftests/vfio/settings


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.53.0

[PATCH v2 1/4] vfio: Reject a second cdev open before mutating shared device state

[Alex Williamson]

The cdev single-open check lives in vfio_df_open(), which runs at the
end of the bind ioctl, after vfio_df_ioctl_bind_iommufd() has already
updated state shared across all opens: vfio_df_check_token() can set
the PF vf_token and vfio_df_get_kvm_safe() records the caller's KVM
pointer in device->kvm and takes a reference.

A second cdev bind of an already-open device runs both, only to be
rejected in vfio_df_open().  The error path clears device->kvm and
drops the reference, tearing down the current opener's KVM association
and potentially resulting in an unbalanced reference on close or
premature release, while the vf_token remains clobbered.

Move the single-open check into vfio_df_ioctl_bind_iommufd() ahead of
both mutations, so a bind that cannot complete leaves the current
opener's state untouched.  df->group is NULL on this path, so a
non-zero open_count is exactly what vfio_df_open() rejected.  The test
in vfio_df_open() becomes redundant and is removed.

Return -EBUSY rather than -EINVAL here.  The arguments are not invalid,
the device is in use, which could be a transient condition due to a
delayed fput if the prior user is terminated.  This provides
compatibility with the group path, where a group open returns -EBUSY,
and users may choose bounded polling to detect such a transient
condition.

Fixes: 839e692fa4eb ("vfio: Make vfio_df_open() single open for device cdev path")
Fixes: 5fcc26969a16 ("vfio: Add VFIO_DEVICE_BIND_IOMMUFD")
Fixes: 86624ba3b522 ("vfio/pci: Do vf_token checks for VFIO_DEVICE_BIND_IOMMUFD")
Assisted-by: LLM
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
 drivers/vfio/device_cdev.c | 12 ++++++++++++
 drivers/vfio/vfio_main.c   |  7 -------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
index 1d9515c967b0..30362936c7b5 100644
--- a/drivers/vfio/device_cdev.c
+++ b/drivers/vfio/device_cdev.c
@@ -130,6 +130,18 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file *df,
 		goto out_unlock;
 	}
 
+	/*
+	 * The cdev path allows only a single open.  Reject a second open here,
+	 * before the VF token and device->kvm updates below would clobber the
+	 * current opener's state on a bind that cannot complete.  Return -EBUSY
+	 * rather than -EINVAL since a delayed release of the prior opener can
+	 * make this transient.
+	 */
+	if (device->open_count) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
 	ret = vfio_df_check_token(device, &bind);
 	if (ret)
 		goto out_unlock;
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 423ead48aafe..cb3deb5a4857 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -603,13 +603,6 @@ int vfio_df_open(struct vfio_device_file *df)
 
 	lockdep_assert_held(&device->dev_set->lock);
 
-	/*
-	 * Only the group path allows the device to be opened multiple
-	 * times.  The device cdev path doesn't have a secure way for it.
-	 */
-	if (device->open_count != 0 && !df->group)
-		return -EINVAL;
-
 	device->open_count++;
 	if (device->open_count == 1) {
 		ret = vfio_df_device_first_open(df);
-- 
2.53.0

[PATCH v2 2/4] vfio: selftests: Verify a failed second open preserves the vf_token

[Alex Williamson]

The cdev path enforces a single open per device and rejects a second
bind of an already open device.  That rejection must happen before the
bind can mutate state shared across opens, notably the PF vf_token, so
that a bind which cannot complete leaves the current opener's state
untouched.

Add a regression test that binds a PF with one token, attempts a
second bind of the same PF with a different token, then initializes a
VF with the original token.  The VF init succeeds only if the second
bind left the PF vf_token intact; a regression that clobbered it to
the second token would make the VF init fail.

Additionally add a second separate test that enforces the -EBUSY
errno on second open so that the vf_token clobber and errno testing
are independent.

These hazards are specific to the cdev/iommufd single-open path, so
the tests run only in iommufd mode.

Suggested-by: David Matlack <dmatlack@google.com>
Assisted-by: LLM
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
 .../selftests/vfio/vfio_pci_sriov_uapi_test.c | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
index 19d657d00b75..b57e4498443f 100644
--- a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
+++ b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
@@ -157,6 +157,63 @@ TEST_F(vfio_pci_sriov_uapi_test, override_token)
 	ASSERT_COND_VF_CREATION(ret);
 }
 
+TEST(failed_second_open_does_not_clobber_token)
+{
+	struct vfio_pci_device *pf = NULL, *pf_second_fd = NULL, *vf = NULL;
+	struct iommu *iommu;
+	int ret;
+
+	iommu = iommu_init("iommufd");
+
+	/* Create and bind PF using UUID_1 */
+	ret = device_init(pf_bdf, iommu, UUID_1, &pf);
+	ASSERT_EQ(ret, 0);
+
+	/*
+	 * Attempt to open the same PF again and bind it with a *different*
+	 * token (UUID_2).  Return value intentionally unenforced.
+	 */
+	device_init(pf_bdf, iommu, UUID_2, &pf_second_fd);
+
+	/*
+	 * Attempt to initialize a VF using the original PF token (UUID_1).
+	 * If the failed open above clobbered the PF's token (i.e. updated it to
+	 * UUID_2), this VF initialization will fail.
+	 */
+	ret = device_init(vf_bdf, iommu, UUID_1, &vf);
+	ASSERT_EQ(ret, 0);
+
+	device_cleanup(vf);
+	device_cleanup(pf_second_fd);
+	device_cleanup(pf);
+	iommu_cleanup(iommu);
+}
+
+TEST(failed_second_open_returns_ebusy)
+{
+	struct vfio_pci_device *pf = NULL, *pf_second_fd = NULL;
+	struct iommu *iommu;
+	int ret;
+
+	iommu = iommu_init("iommufd");
+
+	/* Create and bind PF using UUID_1 */
+	ret = device_init(pf_bdf, iommu, UUID_1, &pf);
+	ASSERT_EQ(ret, 0);
+
+	/*
+	 * Attempt to open the same PF again and bind it with a *different*
+	 * token (UUID_2). This must fail with EBUSY because it's a second open.
+	 * Previously failed with EINVAL.
+	 */
+	ret = device_init(pf_bdf, iommu, UUID_2, &pf_second_fd);
+	ASSERT_EQ(ret, -EBUSY);
+
+	device_cleanup(pf_second_fd);
+	device_cleanup(pf);
+	iommu_cleanup(iommu);
+}
+
 static void vf_teardown(void)
 {
 	/*
-- 
2.53.0

[PATCH v2 3/4] vfio: selftests: Extend mix_and_match timeout to 90s

[Alex Williamson]

Systems with firmware first error handling can see heavy SMM load
as a result of the mix_and_match induced IOMMU errors.  For devices
like igb that get wedged on these errors, the resulting FLR is delayed
waiting for the transaction pending register to clear, which never
occurs.  However, due to the background SMM, the backoff delays in
PCI code wildly exceed their timeouts.  As a result, the whole test
can report failure.

Extend the mix_and_match timeout to a value shown to complete reliably
on such systems, with ~2x margin.

Assisted-by: LLM
Reviewed-by: David Matlack <dmatlack@google.com>
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
 tools/testing/selftests/vfio/vfio_pci_driver_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vfio/vfio_pci_driver_test.c b/tools/testing/selftests/vfio/vfio_pci_driver_test.c
index 761bf117d624..1d532f944cee 100644
--- a/tools/testing/selftests/vfio/vfio_pci_driver_test.c
+++ b/tools/testing/selftests/vfio/vfio_pci_driver_test.c
@@ -190,7 +190,7 @@ TEST_F(vfio_pci_driver_test, send_msi)
 	ASSERT_EQ(1, value);
 }
 
-TEST_F(vfio_pci_driver_test, mix_and_match)
+TEST_F_TIMEOUT(vfio_pci_driver_test, mix_and_match, 90)
 {
 	u64 value;
 	int i;
-- 
2.53.0

[PATCH v2 4/4] vfio: selftests: Extend timeout for runner executions

[Alex Williamson]

When executed via the runner, ie. run_kselftest.sh, tests are subject
to a default 45s timeout.  None of the igb, ioatdma, or nv_falcon
drivers can run the vfio_pci_driver_test within this window.

The igb driver is the worst due to the device getting wedged from bad
DMAs in mix_and_match and requiring the full transaction pending
delay prior to FLR.

This is even further exacerbated when the system implements
firmware-first error handling and the transaction pending scheduled
delays are preempted beyond expectations by SMIs.

Worst case observed runtime for the whole vfio_pci_driver_test on
such a setup is 450s.  Round up to 600s.

Assisted-by: LLM
Reviewed-by: David Matlack <dmatlack@google.com>
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
 tools/testing/selftests/vfio/.gitignore | 1 +
 tools/testing/selftests/vfio/settings   | 5 +++++
 2 files changed, 6 insertions(+)
 create mode 100644 tools/testing/selftests/vfio/settings

diff --git a/tools/testing/selftests/vfio/.gitignore b/tools/testing/selftests/vfio/.gitignore
index 7fadc19d3bca..fc14bc18bcd8 100644
--- a/tools/testing/selftests/vfio/.gitignore
+++ b/tools/testing/selftests/vfio/.gitignore
@@ -8,3 +8,4 @@
 !*.mk
 !.gitignore
 !Makefile
+!settings
diff --git a/tools/testing/selftests/vfio/settings b/tools/testing/selftests/vfio/settings
new file mode 100644
index 000000000000..c9abf1c19f19
--- /dev/null
+++ b/tools/testing/selftests/vfio/settings
@@ -0,0 +1,5 @@
+# Extend kselftest default 45s timeout based on worst case test runtime,
+# currently vfio:vfio_pci_driver_test running on physical NIC, battling
+# SMIs due to invoked firmware-first error handling from bad DMAs and
+# device wedging.  Observed worst case ~450s, extend to 600s.
+timeout=600
-- 
2.53.0