[PATCH v1 0/3] iommufd: Fix vDEVICE allocation lifecycle bugs

[PATCH v1 0/3] iommufd: Fix vDEVICE allocation lifecycle bugs

[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-v1

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

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


base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.43.0

[PATCH v1 1/3] iommufd/viommu: Release the igroup lock on the vdevice_size error path

[Nicolin Chen]

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: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/viommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 4081deda9b33d..0c12c7e352a14 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -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;
 	}
-- 
2.43.0

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

[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")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/viommu.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 0c12c7e352a14..5b40e924f0782 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -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,21 @@ 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;
+	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)
-			goto out_abort;
+			goto out_release;
 	}
 
+	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)
@@ -237,6 +240,8 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	iommufd_object_finalize(ucmd->ictx, &vdev->obj);
 	goto out_unlock_igroup;
 
+out_release:
+	xa_release(&viommu->vdevs, virt_id);
 out_abort:
 	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
 out_unlock_igroup:
-- 
2.43.0

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

[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 -EINVAL 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")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c   | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 1e9f7d2de3441..2ba08df75af8b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -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 -EINVAL;
+	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,
-- 
2.43.0

RE: [PATCH v1 1/3] iommufd/viommu: Release the igroup lock on the vdevice_size error path

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 30, 2026 5:16 AM
> 
> 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: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 30, 2026 5:16 AM
> 
> 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")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

cc stable

> 
>  	if (viommu->ops && viommu->ops->vdevice_init) {
>  		rc = viommu->ops->vdevice_init(vdev);
>  		if (rc)
> -			goto out_abort;
> +			goto out_release;
>  	}

though correct, this causes an counter-intuitive error unwind
pattern with the following goto jumps to an outer label (out_abort).

It's cleaner to remove the new label by:

		if (rc) {
			xa_release(&viommu->vdevs, virt_id);
			goto out_abort;
		}

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v1 3/3] iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 30, 2026 5:16 AM
>  }
> 
> +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 -EINVAL;
> +	return 0;
> +}

- EOPNOTSUPP, given the comment seems to indicate it as a current
limitation hence may be removed in future? otherwise with -EINVAL
this may be documented somewhere to get attention from userspace.

arm_vsmmu_vsid_to_sid()

	/* At this moment, iommufd only supports PCI device that has one SID */
	if (sid)
		*sid = master->streams[0].id;

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v1 3/3] iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

[Nicolin Chen]

On Fri, Jul 03, 2026 at 06:40:22AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, June 30, 2026 5:16 AM
> >  }
> > 
> > +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 -EINVAL;
> > +	return 0;
> > +}
> 
> - EOPNOTSUPP, given the comment seems to indicate it as a current
> limitation hence may be removed in future? otherwise with -EINVAL
> this may be documented somewhere to get attention from userspace.

OK. I will change it and other places too.

Thanks
Nicolin