iommufd: Fix vDEVICE allocation lifecycle bugs

PATCH Applied Nicolin Chen 3 participants 10 messages Jul 6 – Jul 14

Subsystems (3)

IOMMU Arm SMMU drivers Iommufd

lore

Share & tools

lore

Nicolin Chen

author

2026-07-06 05:36 UTC #0 lore raw

v2 cover iommufd: Fix vDEVICE allocation lifecycle bugs

Sashiko flagged a few bugs in how IOMMU_VDEVICE_ALLOC creates and validates a vDEVICE on a vIOMMU:

- the core publishes a vDEVICE into the vIOMMU xarray before the driver's vdevice_init() runs, so a concurrent invalidation can reach one it has not yet accepted;
- the undersized-vdevice_size guard returns holding the igroup mutex, deadlocking later vDEVICE operations on that group;
- the Arm SMMUv3 vIOMMU accepts a device without exactly one Stream ID:
an out-of-bounds streams[] read for none, stale ATC/IOTLB for several.

Fix each of them properly.

This is on Github:
https://github.com/nicolinc/iommufd/commits/fix_vdevice_sashiko-v2

Changelog
v2
* Add "Reviewed-by" from Kevin
* Patch-2: Add "Cc stable"; drop the out_release label by inlining the xa_release() call
* Patch-2: Note that the reserved slot is hidden only via xa_* helpers
* Patch-3: Return -EOPNOTSUPP instead of -EINVAL
v1
https://lore.kernel.org/all/[email protected]/

Nicolin Chen (3):
iommufd/viommu: Release the igroup lock on the vdevice_size error path iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 15 +++++++++++++
 drivers/iommu/iommufd/viommu.c                | 22 ++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

Signature · 1 line

2.43.0

Nicolin Chen

author

2026-07-06 05:36 UTC #1 lore raw

PATCH v2 1/3 iommufd/viommu: Release the igroup lock on the vdevice_size error path

iommufd_vdevice_alloc_ioctl() takes idev->igroup->lock, then validates the driver's vdevice_size against the core structure size with a WARN_ON_ONCE. On failure that guard jumps to out_put_idev, below out_unlock_igroup, so it skips the mutex_unlock(), leaving the igroup lock held and deadlocking the next vDEVICE operation on that group.

Jump to out_unlock_igroup instead.

Fixes ed42eee797ff3 ("iommufd/viommu: Add driver-defined vDEVICE support")

Cc [email protected]

Reviewed-by Kevin Tian <[email protected]>

Assisted-by Claude:claude-opus-4-8

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/viommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iommu/iommufd/viommu.c +1 -1

index 4081deda9b33d..0c12c7e352a14 100644
@@ -189,7 +189,7 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		if (WARN_ON_ONCE(viommu->ops->vdevice_size < vdev_size ||
 				 !viommu->ops->vdevice_init)) {
 			rc = -EOPNOTSUPP;
-			goto out_put_idev;
+			goto out_unlock_igroup;
 		}
 		vdev_size = viommu->ops->vdevice_size;
 	}

Signature · 1 line

2.43.0

Nicolin Chen

author

2026-07-06 05:36 UTC #2 lore raw

PATCH v2 2/3 iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds

In reply to Nicolin Chen

iommufd_vdevice_alloc_ioctl() adds the vDEVICE to the viommu->vdevs xarray with xa_cmpxchg() before the driver's vdevice_init() op runs. That op is where a driver validates the device and may reject it, but the xarray entry is already live by then: a concurrent IOMMU_HWPT_INVALIDATE can look it up with iommufd_viommu_find_dev() and run the driver invalidation path against a device that vdevice_init() would have refused.

Reserve the index with xa_insert(): it stores a zero entry that reads back as NULL, and returns -EBUSY on a duplicate virt_id. Run vdevice_init() and store the vDEVICE pointer only once it succeeds. A failed vdevice_init() releases the reservation, so lookups observe the vDEVICE only after it is fully initialized and accepted.

Fixes ed42eee797ff3 ("iommufd/viommu: Add driver-defined vDEVICE support")

Cc [email protected]

Reviewed-by Kevin Tian <[email protected]>

Assisted-by Claude:claude-opus-4-8

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/viommu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

drivers/iommu/iommufd/viommu.c +15 -5

index 0c12c7e352a14..fc13cf4737ea3 100644
@@ -143,7 +143,7 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj)
 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_vdevice_alloc *cmd = ucmd->cmd;
-	struct iommufd_vdevice *vdev, *curr;
+	struct iommufd_vdevice *vdev;
 	size_t vdev_size = sizeof(*vdev);
 	struct iommufd_viommu *viommu;
 	struct iommufd_device *idev;

@@ -218,18 +218,28 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	idev->vdev = vdev;
 
-	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
-	if (curr) {
-		rc = xa_err(curr) ?: -EEXIST;
+	/*
+	 * Reserve the slot with a zero entry (reads back as NULL) until the
+	 * vdevice_init() op accepts the vDEVICE. Only the xa_* helpers hide a
+	 * reserved entry, so never use a raw xas_* iterator on this xarray.
+	 */
+	rc = xa_insert(&viommu->vdevs, virt_id, NULL, GFP_KERNEL);
+	if (rc) {
+		if (rc == -EBUSY)
+			rc = -EEXIST;
 		goto out_abort;
 	}
 
 	if (viommu->ops && viommu->ops->vdevice_init) {
 		rc = viommu->ops->vdevice_init(vdev);
-		if (rc)
+		if (rc) {
+			xa_release(&viommu->vdevs, virt_id);
 			goto out_abort;
+		}
 	}
 
+	xa_store(&viommu->vdevs, virt_id, vdev, GFP_KERNEL);
+
 	cmd->out_vdevice_id = vdev->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)

Signature · 1 line

2.43.0

Jason Gunthorpe

maintainer

2026-07-06 17:24 UTC #3 lore raw

Re: [PATCH v2 2/3] iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds

Quoted 4 lines · Nicolin Chen

drivers/iommu/iommufd/viommu.c +0 -0

@@ -218,18 +218,28 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	idev->vdev = vdev;

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 227

Quoted 10 lines

drivers/iommu/iommufd/viommu.c +7 -3

-	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
-	if (curr) {
-		rc = xa_err(curr) ?: -EEXIST;
+	/*
+	 * Reserve the slot with a zero entry (reads back as NULL) until the
+	 * vdevice_init() op accepts the vDEVICE. Only the xa_* helpers hide a
+	 * reserved entry, so never use a raw xas_* iterator on this xarray.
+	 */
+	rc = xa_insert(&viommu->vdevs, virt_id, NULL, GFP_KERNEL);
+	if (rc) {

Why xa_insert() not xa_reserve() ?

Jason

Nicolin Chen

author

2026-07-06 18:06 UTC #4 lore raw

Re: [PATCH v2 2/3] iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds

Quoted 5 lines · Jason Gunthorpe

Quoted 4 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +0 -0
@@ -218,18 +218,28 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	idev->vdev = vdev;
 

Quoted 12 lines

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 227

Quoted 10 lines

drivers/iommu/iommufd/viommu.c +7 -3

-	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
-	if (curr) {
-		rc = xa_err(curr) ?: -EEXIST;
+	/*
+	 * Reserve the slot with a zero entry (reads back as NULL) until the
+	 * vdevice_init() op accepts the vDEVICE. Only the xa_* helpers hide a
+	 * reserved entry, so never use a raw xas_* iterator on this xarray.
+	 */
+	rc = xa_insert(&viommu->vdevs, virt_id, NULL, GFP_KERNEL);
+	if (rc) {

Why xa_insert() not xa_reserve() ?

xa_reserve's kdoc says:
"* If there is already something stored at @index, this function does
* nothing"

By its implementation, it wouldn't return an error on duplication?

Nicolin

Jason Gunthorpe

maintainer

2026-07-06 19:47 UTC #5 lore raw

Re: [PATCH v2 2/3] iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds

Quoted 19 lines · Nicolin Chen

Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +0 -0
@@ -218,18 +218,28 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	idev->vdev = vdev;
 
Quoted 12 lines
drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 227
Quoted 10 lines
drivers/iommu/iommufd/viommu.c +7 -3
-	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
-	if (curr) {
-		rc = xa_err(curr) ?: -EEXIST;
+	/*
+	 * Reserve the slot with a zero entry (reads back as NULL) until the
+	 * vdevice_init() op accepts the vDEVICE. Only the xa_* helpers hide a
+	 * reserved entry, so never use a raw xas_* iterator on this xarray.
+	 */
+	rc = xa_insert(&viommu->vdevs, virt_id, NULL, GFP_KERNEL);
+	if (rc) {
Why xa_insert() not xa_reserve() ?

Quoted 5 lines

xa_reserve's kdoc says:
"* If there is already something stored at @index, this function does
* nothing"
By its implementation, it wouldn't return an error on duplication?

Yes, OK that's a good reason

The way I usually write this pattern is two cmpxchgs, the first NULL -> XA_ZERO_ENTRY and the second XA_ZERO_ENTRY to the final value.

The latter failing would be a WARN_ON condition, but this is fine too

Jason

Nicolin Chen

author

2026-07-06 05:36 UTC #6 lore raw

PATCH v2 3/3 iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

In reply to Nicolin Chen

arm_vsmmu_vsid_to_sid() maps a guest's vSID to a single physical Stream ID taken from master->streams[0], assuming a device has exactly one stream. A device with several streams gets only its first one mapped, so a guest vSID invalidation cannot reach the others' ATC and IOTLB entries; a device with none makes master->streams a ZERO_SIZE_PTR, read out of bounds.

Add an arm_vsmmu_vdevice_init() op to reject the vDEVICE with -EOPNOTSUPP when master->num_streams is not one, rather than mapping it silently.

Fixes d68beb276ba26 ("iommu/arm-smmu-v3: Support IOMMU_HWPT_INVALIDATE using a VIOMMU object")

Reviewed-by Kevin Tian <[email protected]>

Assisted-by Claude:claude-opus-4-8

Signed-off-by Nicolin Chen <[email protected]>

---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c   | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +15 -0

index 1e9f7d2de3441..85ebfdb3d2a7a 100644
@@ -297,6 +297,20 @@ static int arm_vsmmu_vsid_to_sid(struct arm_vsmmu *vsmmu, u32 vsid, u32 *sid)
 	return ret;
 }
 
+static int arm_vsmmu_vdevice_init(struct iommufd_vdevice *vdev)
+{
+	struct device *dev = iommufd_vdevice_to_device(vdev);
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+	/*
+	 * arm_vsmmu_vsid_to_sid() maps a vSID to master->streams[0] alone, so
+	 * more streams would leave the rest stale and none reads out of bounds.
+	 */
+	if (master->num_streams != 1)
+		return -EOPNOTSUPP;
+	return 0;
+}
+
 /* This is basically iommu_viommu_arm_smmuv3_invalidate in u64 for conversion */
 struct arm_vsmmu_invalidation_cmd {
 	union {

@@ -403,6 +417,7 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
 static const struct iommufd_viommu_ops arm_vsmmu_ops = {
 	.alloc_domain_nested = arm_vsmmu_alloc_domain_nested,
 	.cache_invalidate = arm_vsmmu_cache_invalidate,
+	.vdevice_init = arm_vsmmu_vdevice_init,
 };
 
 size_t arm_smmu_get_viommu_size(struct device *dev,

Signature · 1 line

2.43.0

Jason Gunthorpe

maintainer Applied

2026-07-14 14:06 UTC #7 lore raw

In reply to Nicolin Chen

Quoted 26 lines · Nicolin Chen

Sashiko flagged a few bugs in how IOMMU_VDEVICE_ALLOC creates and validates a vDEVICE on a vIOMMU:
- the core publishes a vDEVICE into the vIOMMU xarray before the driver's vdevice_init() runs, so a concurrent invalidation can reach one it has not yet accepted;
- the undersized-vdevice_size guard returns holding the igroup mutex, deadlocking later vDEVICE operations on that group;
- the Arm SMMUv3 vIOMMU accepts a device without exactly one Stream ID:
an out-of-bounds streams[] read for none, stale ATC/IOTLB for several.
Fix each of them properly.
This is on Github:
https://github.com/nicolinc/iommufd/commits/fix_vdevice_sashiko-v2
Changelog
v2
* Add "Reviewed-by" from Kevin
* Patch-2: Add "Cc stable"; drop the out_release label by inlining the xa_release() call
* Patch-2: Note that the reserved slot is hidden only via xa_* helpers
* Patch-3: Return -EOPNOTSUPP instead of -EINVAL
v1
https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

Nicolin Chen (3):
iommufd/viommu: Release the igroup lock on the vdevice_size error path iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 15 +++++++++++++
 drivers/iommu/iommufd/viommu.c                | 22 ++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

Applied to for-rc

Thanks,
Jason

Pranjal Shrivastava

2026-07-14 14:23 UTC #8 lore raw

Quoted 32 lines · Jason Gunthorpe

Quoted 31 lines · Nicolin Chen
Sashiko flagged a few bugs in how IOMMU_VDEVICE_ALLOC creates and validates a vDEVICE on a vIOMMU:
- the core publishes a vDEVICE into the vIOMMU xarray before the driver's vdevice_init() runs, so a concurrent invalidation can reach one it has not yet accepted;
- the undersized-vdevice_size guard returns holding the igroup mutex, deadlocking later vDEVICE operations on that group;
- the Arm SMMUv3 vIOMMU accepts a device without exactly one Stream ID:
an out-of-bounds streams[] read for none, stale ATC/IOTLB for several.
Fix each of them properly.
This is on Github:
https://github.com/nicolinc/iommufd/commits/fix_vdevice_sashiko-v2
Changelog
v2
* Add "Reviewed-by" from Kevin
* Patch-2: Add "Cc stable"; drop the out_release label by inlining the xa_release() call
* Patch-2: Note that the reserved slot is hidden only via xa_* helpers
* Patch-3: Return -EOPNOTSUPP instead of -EINVAL
v1
https://lore.kernel.org/all/[email protected]/
Nicolin Chen (3):
iommufd/viommu: Release the igroup lock on the vdevice_size error path iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

Quoted 6 lines

Quoted 3 lines

 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 15 +++++++++++++
 drivers/iommu/iommufd/viommu.c                | 22 ++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

Applied to for-rc

Oops, sorry. I missed this while looking at patches in mutt. Feel free to ignore my R-b if I'm too late.

Thanks,
Praan

Pranjal Shrivastava

Reviewed-by on the v2 series

2026-07-14 14:20 UTC #9 lore raw

In reply to Nicolin Chen

Quoted 26 lines · Nicolin Chen

Sashiko flagged a few bugs in how IOMMU_VDEVICE_ALLOC creates and validates a vDEVICE on a vIOMMU:
- the core publishes a vDEVICE into the vIOMMU xarray before the driver's vdevice_init() runs, so a concurrent invalidation can reach one it has not yet accepted;
- the undersized-vdevice_size guard returns holding the igroup mutex, deadlocking later vDEVICE operations on that group;
- the Arm SMMUv3 vIOMMU accepts a device without exactly one Stream ID:
an out-of-bounds streams[] read for none, stale ATC/IOTLB for several.
Fix each of them properly.
This is on Github:
https://github.com/nicolinc/iommufd/commits/fix_vdevice_sashiko-v2
Changelog
v2
* Add "Reviewed-by" from Kevin
* Patch-2: Add "Cc stable"; drop the out_release label by inlining the xa_release() call
* Patch-2: Note that the reserved slot is hidden only via xa_* helpers
* Patch-3: Return -EOPNOTSUPP instead of -EINVAL
v1
https://lore.kernel.org/all/[email protected]/

Quoted 9 lines

Nicolin Chen (3):
iommufd/viommu: Release the igroup lock on the vdevice_size error path iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 15 +++++++++++++
 drivers/iommu/iommufd/viommu.c                | 22 ++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

Reviewed-by Pranjal Shrivastava <[email protected]>

Thanks,
Praan