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

[PATCH v2 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-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/cover.1782767110.git.nicolinc@nvidia.com/

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(-)

-- 
2.43.0

[PATCH v2 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
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
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 v2 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")
Cc: stable@vger.kernel.org
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/viommu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 0c12c7e352a14..fc13cf4737ea3 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,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)
-- 
2.43.0

[PATCH v2 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 -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 <kevin.tian@intel.com>
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..85ebfdb3d2a7a 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 -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,
-- 
2.43.0

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

[Jason Gunthorpe]

On Sun, Jul 05, 2026 at 10:36:10PM -0700, Nicolin Chen wrote:
> @@ -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) {

Why xa_insert() not xa_reserve() ?

Jason

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

[Nicolin Chen]

On Mon, Jul 06, 2026 at 02:23:56PM -0300, Jason Gunthorpe wrote:
> On Sun, Jul 05, 2026 at 10:36:10PM -0700, Nicolin Chen wrote:
> > @@ -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) {
> 
> 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

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

[Jason Gunthorpe]

On Mon, Jul 06, 2026 at 11:05:51AM -0700, Nicolin Chen wrote:
> On Mon, Jul 06, 2026 at 02:23:56PM -0300, Jason Gunthorpe wrote:
> > On Sun, Jul 05, 2026 at 10:36:10PM -0700, Nicolin Chen wrote:
> > > @@ -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) {
> > 
> > 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?

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