Skip to contentSkip to conversation

iommu/tegra241-cmdqv: Fix error-interrupt races and VINTF lifecycle bugs

PATCH In review Nicolin Chen 2 participants 17 messages Jul 14 – Jul 28

Subsystems (4)
Tegra architecture support IOMMU Arm SMMU drivers Tegra IOMMU drivers
lore
Share & tools
lore

Nicolin Chen

author
#0 lore raw

v2 cover iommu/tegra241-cmdqv: Fix error-interrupt races and VINTF lifecycle bugs

These fix a cluster of bugs reported by Sashiko during patch reviews. The patches are ordered roughly most-critical-first, so some later ones fix smaller pre-existing issues in the same functions that earlier patches touch.

Issues fixed:
- the error ISR racing VINTF (de)init and reading a NULL, freed, or not yet fully initialized slot
- the probe fallback dereferencing an smmu freed by devm_krealloc()
- a guest vSID programmed without validating its width or the device's Stream ID count
- VINTF0 leaked on an init-failure path
- error-map index/bounds handling and a VCMDQ base above the 48-bit limit
- the error ISR flooding the kernel log under repeated guest errors

False positives raised by Sashiko:
- a viommu outliving an SMMU unbind and touching freed memory on close: a physical IOMMU is not a pluggable device, so iommufd holds no reference on the one behind a viommu, and this teardown cannot arise.
- the ISR running after cmdqv is freed on probe failure: free_irq() runs first from tegra241_cmdqv_remove(), the devm device_remove action, which devres invokes before the cmdqv allocation is released.
- a guest never acking its VCMDQ error wedging the shared interrupt: the interrupt is edge-signaled per event, and the host ISR only snapshots the error map into the guest's bounded vEVENTQ, never depending on a guest-side GERRORN ack.
- the ISR accessing a de-assigned LVCMDQ page after a VINTF hw_init()
failure: the page remains a mapped MMIO region backed by empty registers, so reads are benign and writes are dropped.

In parallel to Shameer's Tegra241 CMDQV CMD_SYNC use-after-free fix:
https://lore.kernel.org/all/[email protected]/

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

Changelog
v2
* Return -EOPNOTSUPP in tegra241_vintf_init_vsid
v1
https://lore.kernel.org/all/[email protected]/

Nicolin Chen (11):
iommu/tegra241-cmdqv: Publish an LVCMDQ only after it is fully
initialized
iommu/tegra241-cmdqv: Synchronize the error ISR against VINTF (de)init iommu/tegra241-cmdqv: Harden error-map index handling in the error ISR iommu/tegra241-cmdqv: Don't run the error ISR before probe sets up
vintfs
iommu/tegra241-cmdqv: Don't fall back to a freed smmu after
devm_krealloc()
iommu/tegra241-cmdqv: Free the error IRQ before tearing down VINTFs iommu/tegra241-cmdqv: Reject a vSID wider than the SID_MATCH field iommu/tegra241-cmdqv: Require exactly one Stream ID for a vSID iommu/tegra241-cmdqv: Fix VINTF0 leak on the init-failure path iommu/tegra241-cmdqv: Warn on a VCMDQ base above the 48-bit hardware
limit
iommu/tegra241-cmdqv: Rate-limit the error ISR's log message

 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 224 ++++++++++++------
 1 file changed, 156 insertions(+), 68 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v2 1/11 iommu/tegra241-cmdqv: Publish an LVCMDQ only after it is fully initialized

tegra241_vintf_init_lvcmdq() stores the freshly allocated vcmdq pointer to the vintf->lvcmdqs[] array, before tegra241_vcmdq_alloc_smmu_cmdq() builds the vcmdq->cmdq. The error ISR dereferences that cmdq, so a latched LVCMDQ error (e.g. one inherited across a kexec) firing in this window would make tegra241_vintf0_handle_error() pass the still-zeroed arm_smmu_cmdq down to __arm_smmu_cmdq_skip_err(), dereferencing NULL queue register pointers.

Drop the store from tegra241_vintf_init_lvcmdq() and publish the vcmdq at the end of the allocation instead, with an smp_store_release() that pairs with an smp_load_acquire() in the ISR, which can see a fully built LVCMDQ or NULL.

The user-owned LVCMDQ allocation moves accordingly, publishing the vcmdq once tegra241_vcmdq_hw_init_user() succeeds, using a plain store since a user VINTF's lvcmdqs[] has no lockless reader -- the error ISR only walks the VINTF0 array.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Assisted-by Claude:claude-fable-5
Signed-off-by Nicolin Chen <[email protected]>
---
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +17 -8
index 67be62a6e7640..f843d607c8134 100644
@@ -318,12 +318,19 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
 
 		while (map) {
 			unsigned long lidx = __ffs64(map);
-			struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[lidx];
-			u32 gerror = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERROR));
+			struct tegra241_vcmdq *vcmdq;
+			u32 gerror;
 
+			map &= ~BIT_ULL(lidx);
+
+			/* Pairs with smp_store_release() publishing it */
+			vcmdq = smp_load_acquire(&vintf->lvcmdqs[lidx]);
+			if (!vcmdq)
+				continue;
+
+			gerror = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERROR));
 			__arm_smmu_cmdq_skip_err(&vintf->cmdqv->smmu, &vcmdq->cmdq);
 			writel(gerror, REG_VCMDQ_PAGE0(vcmdq, GERRORN));
-			map &= ~BIT_ULL(lidx);
 		}
 	}
 }

@@ -666,7 +673,6 @@ static int tegra241_vintf_init_lvcmdq(struct tegra241_vintf *vintf, u16 lidx,
 	vcmdq->page0 = cmdqv->base + TEGRA241_VINTFi_LVCMDQ_PAGE0(idx, lidx);
 	vcmdq->page1 = cmdqv->base + TEGRA241_VINTFi_LVCMDQ_PAGE1(idx, lidx);
 
-	vintf->lvcmdqs[lidx] = vcmdq;
 	return 0;
 }
 

@@ -705,14 +711,15 @@ tegra241_vintf_alloc_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
 	/* Build an arm_smmu_cmdq for each LVCMDQ */
 	ret = tegra241_vcmdq_alloc_smmu_cmdq(vcmdq);
 	if (ret)
-		goto deinit_lvcmdq;
+		goto free_vcmdq;
+
+	/* Pairs with the smp_load_acquire() in the error ISR */
+	smp_store_release(&vintf->lvcmdqs[lidx], vcmdq);
 
 	dev_dbg(cmdqv->dev,
 		"%sallocated\n", lvcmdq_error_header(vcmdq, header, 64));
 	return vcmdq;
 
-deinit_lvcmdq:
-	tegra241_vintf_deinit_lvcmdq(vintf, lidx);
 free_vcmdq:
 	kfree(vcmdq);
 	return ERR_PTR(ret);

@@ -1130,13 +1137,15 @@ static int tegra241_vintf_alloc_lvcmdq_user(struct iommufd_hw_queue *hw_queue,
 	if (ret)
 		goto unmap_lvcmdq;
 
+	/* No lockless reader of a user VINTF's lvcmdqs[]; mutex-serialized */
+	vintf->lvcmdqs[lidx] = vcmdq;
+
 	hw_queue->destroy = &tegra241_vintf_destroy_lvcmdq_user;
 	mutex_unlock(&vintf->lvcmdq_mutex);
 	return 0;
 
 unmap_lvcmdq:
 	tegra241_vcmdq_unmap_lvcmdq(vcmdq);
-	tegra241_vintf_deinit_lvcmdq(vintf, lidx);
 undepend_vcmdq:
 	if (vcmdq->prev)
 		iommufd_hw_queue_undepend(vcmdq, vcmdq->prev, core);
Signature · 1 line
2.43.0
author
#2 lore raw

PATCH v2 2/11 iommu/tegra241-cmdqv: Synchronize the error ISR against VINTF (de)init

In reply to Nicolin Chen

A user VINTF is torn down by tegra241_cmdqv_deinit_vintf(), which runs from the destroy callback and from the init-failure unwind in the alloc handler. It clears the cmdqv->vintfs[] slot and lets the iommufd core free it, but nothing serializes that against the error interrupt: tegra241_cmdqv_isr() reads cmdqv->vintfs[idx] and dereferences the vintf. A concurrent error can make the ISR read a slot mid-clear (a NULL deref) or use a vintf which is about to be freed (a use-after-free).

deinit_vintf() also returns idx to the IDA before clearing the slot, so a concurrent create that reuses idx can publish its new vintf into the slot, only for this teardown to erase it again with the stale NULL store.

On the other end, tegra241_cmdqv_init_vintf() publishes a new vintf with a plain store to the cmdqv->vintfs[] slot, and the ISR dereferences fields of a published vintf such as vintf->base. A plain store gives no ordering on a weakly-ordered CPU, and a stale VINTF_ERR_MAP bit on a reused idx can make the ISR pick a vintf the moment it is published, before its fields are set or tegra241_vintf_hw_init() runs.

The cmdqv->vintfs[0] slot stays NULL until tegra241_cmdqv_init_structures() first creates VINTF0, so the slot 0 read needs the same NULL check.

Publish every slot with an smp_store_release(), and read each slot in the ISR with an smp_load_acquire() under a NULL check, so the ISR always sees a fully built vintf or NULL. Also make deinit_vintf() clear the slot, and synchronize_irq() prior to returning idx to the IDA, so no vintf is freed under a running handler and no reused idx is clobbered.

Fixes 4dc0d12474f9 ("iommu/tegra241-cmdqv: Add user-space use support")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 37 +++++++++++++++++--
 1 file changed, 33 insertions(+), 4 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +33 -4
index f843d607c8134..5fbc1c85874a5 100644
@@ -335,6 +335,13 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
 	}
 }
 
+/*
+ * The CMDQV error interrupt is edge-triggered, so a pending VINTF error fires
+ * this ISR once and does not re-assert. An unacked guest therefore cannot
+ * storm the host. The HW latches and forwards each new error event on its
+ * own, so an already-set ERR_MAP bit does not suppress the interrupt for a
+ * new error.
+ */
 static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 {
 	struct tegra241_cmdqv *cmdqv = (struct tegra241_cmdqv *)devid;

@@ -357,16 +364,27 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 
 	/* Handle VINTF0 and its LVCMDQs */
 	if (vintf_map & BIT_ULL(0)) {
-		tegra241_vintf0_handle_error(cmdqv->vintfs[0]);
+		struct tegra241_vintf *vintf0;
+
 		vintf_map &= ~BIT_ULL(0);
+
+		/* NULL until tegra241_cmdqv_init_structures() publishes it */
+		vintf0 = smp_load_acquire(&cmdqv->vintfs[0]);
+		if (vintf0)
+			tegra241_vintf0_handle_error(vintf0);
 	}
 
 	/* Handle other user VINTFs and their LVCMDQs */
 	while (vintf_map) {
 		unsigned long idx = __ffs64(vintf_map);
+		struct tegra241_vintf *vintf;
 
-		tegra241_vintf_user_handle_error(cmdqv->vintfs[idx]);
 		vintf_map &= ~BIT_ULL(idx);
+
+		/* The slot may be published or torn down (NULL'd) concurrently */
+		vintf = smp_load_acquire(&cmdqv->vintfs[idx]);
+		if (vintf)
+			tegra241_vintf_user_handle_error(vintf);
 	}
 
 	return IRQ_HANDLED;

@@ -730,8 +748,18 @@ tegra241_vintf_alloc_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
 static void tegra241_cmdqv_deinit_vintf(struct tegra241_cmdqv *cmdqv, u16 idx)
 {
 	kfree(cmdqv->vintfs[idx]->lvcmdqs);
+	/*
+	 * Clear the slot and drain any in-flight ISR before returning idx to
+	 * the IDA, so a concurrent create that reuses idx cannot have its
+	 * freshly published VINTF erased here. A plain WRITE_ONCE() suffices
+	 * since clearing the slot publishes no data. This also covers the
+	 * init-failure unwind, which reaches deinit_vintf() without the
+	 * destroy callback.
+	 */
+	WRITE_ONCE(cmdqv->vintfs[idx], NULL);
+	if (cmdqv->irq > 0)
+		synchronize_irq(cmdqv->irq);
 	ida_free(&cmdqv->vintf_ids, idx);
-	cmdqv->vintfs[idx] = NULL;
 }
 
 static int tegra241_cmdqv_init_vintf(struct tegra241_cmdqv *cmdqv, u16 max_idx,

@@ -757,7 +785,8 @@ static int tegra241_cmdqv_init_vintf(struct tegra241_cmdqv *cmdqv, u16 max_idx,
 		return -ENOMEM;
 	}
 
-	cmdqv->vintfs[idx] = vintf;
+	/* Pairs with the smp_load_acquire() in tegra241_cmdqv_isr() */
+	smp_store_release(&cmdqv->vintfs[idx], vintf);
 	return ret;
 }
Signature · 1 line
2.43.0
author
#3 lore raw

PATCH v2 3/11 iommu/tegra241-cmdqv: Harden error-map index handling in the error ISR

In reply to Nicolin Chen

tegra241_vintf0_handle_error() reads both 64-bit LVCMDQ error-map registers but used the register-local __ffs64() bit directly as the vintf->lvcmdqs[] index. For the second register that selects the wrong queue instead of 64 * i + bit, clearing the wrong queue's error status.

The index is unbounded too: a bit at or beyond num_lvcmdqs_per_vintf would walk the read off vintf->lvcmdqs[].

tegra241_cmdqv_isr() has the same flaw one level up: a VINTF_ERR_MAP bit at or beyond num_vintfs would walk the read off cmdqv->vintfs[].

Use 64 * i + bit for the index and clear the snapshot with the local bit. In both handlers, WARN_ON_ONCE() and skip an out-of-bounds index. Only a malfunctioning device sets such a bit, so the _ONCE form keeps a wedged map from flooding the log.

Note that 64 * i + bit is not reachable with the current configuration as a VINTF is pre-assigned with 2 lvcmdqs, this is not treated as bug fix but an defensive hardening.

Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +11 -3
index 5fbc1c85874a5..b89f021ba0b86 100644
@@ -311,25 +311,30 @@ static void tegra241_vintf_user_handle_error(struct tegra241_vintf *vintf)
 
 static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
 {
+	struct tegra241_cmdqv *cmdqv = vintf->cmdqv;
 	int i;
 
 	for (i = 0; i < LVCMDQ_ERR_MAP_NUM_64; i++) {
 		u64 map = readq_relaxed(REG_VINTF(vintf, LVCMDQ_ERR_MAP_64(i)));
 
 		while (map) {
-			unsigned long lidx = __ffs64(map);
+			unsigned long map_bit = __ffs64(map);
+			unsigned long lidx = 64 * i + map_bit;
 			struct tegra241_vcmdq *vcmdq;
 			u32 gerror;
 
-			map &= ~BIT_ULL(lidx);
+			map &= ~BIT_ULL(map_bit);
 
+			/* A bit beyond the count means a HW error; skip it */
+			if (WARN_ON_ONCE(lidx >= cmdqv->num_lvcmdqs_per_vintf))
+				continue;
 			/* Pairs with smp_store_release() publishing it */
 			vcmdq = smp_load_acquire(&vintf->lvcmdqs[lidx]);
 			if (!vcmdq)
 				continue;
 
 			gerror = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERROR));
-			__arm_smmu_cmdq_skip_err(&vintf->cmdqv->smmu, &vcmdq->cmdq);
+			__arm_smmu_cmdq_skip_err(&cmdqv->smmu, &vcmdq->cmdq);
 			writel(gerror, REG_VCMDQ_PAGE0(vcmdq, GERRORN));
 		}
 	}

@@ -381,6 +386,9 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 
 		vintf_map &= ~BIT_ULL(idx);
 
+		/* A bit beyond the count means a HW error; skip it */
+		if (WARN_ON_ONCE(idx >= cmdqv->num_vintfs))
+			continue;
 		/* The slot may be published or torn down (NULL'd) concurrently */
 		vintf = smp_load_acquire(&cmdqv->vintfs[idx]);
 		if (vintf)
Signature · 1 line
2.43.0
author
#4 lore raw

PATCH v2 4/11 iommu/tegra241-cmdqv: Don't run the error ISR before probe sets up vintfs

In reply to Nicolin Chen

__tegra241_cmdqv_probe() requests the error IRQ before it has allocated the cmdqv->vintfs array and set cmdqv->num_vintfs. A CMDQV left enabled with a latched error across a kexec fires the IRQ as soon as it is requested, and tegra241_cmdqv_isr() then walks the uninitialized cmdqv->vintfs array.

Request the IRQ only after cmdqv->vintfs is allocated and zeroed, so that a latched interrupt firing early runs the ISR against a valid array of NULL slots that it safely skips.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 34 +++++++++++--------
 1 file changed, 19 insertions(+), 15 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +19 -15
index b89f021ba0b86..7e366030d9d70 100644
@@ -973,17 +973,6 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 	cmdqv->dev = smmu->impl_dev;
 	cmdqv->base_phys = res->start;
 
-	if (cmdqv->irq > 0) {
-		ret = request_threaded_irq(irq, NULL, tegra241_cmdqv_isr,
-					   IRQF_ONESHOT, "tegra241-cmdqv",
-					   cmdqv);
-		if (ret) {
-			dev_err(cmdqv->dev, "failed to request irq (%d): %d\n",
-				cmdqv->irq, ret);
-			goto iounmap;
-		}
-	}
-
 	regval = readl_relaxed(REG_CMDQV(cmdqv, PARAM));
 	cmdqv->num_vintfs = 1 << FIELD_GET(CMDQV_NUM_VINTF_LOG2, regval);
 	cmdqv->num_vcmdqs = 1 << FIELD_GET(CMDQV_NUM_VCMDQ_LOG2, regval);

@@ -994,10 +983,25 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 	cmdqv->vintfs =
 		kzalloc_objs(*cmdqv->vintfs, cmdqv->num_vintfs);
 	if (!cmdqv->vintfs)
-		goto free_irq;
+		goto iounmap;
 
 	ida_init(&cmdqv->vintf_ids);
 
+	/*
+	 * Request the IRQ only after cmdqv->vintfs is allocated and zeroed, so
+	 * the ISR would not walk an uninitialized array.
+	 */
+	if (cmdqv->irq > 0) {
+		ret = request_threaded_irq(irq, NULL, tegra241_cmdqv_isr,
+					   IRQF_ONESHOT, "tegra241-cmdqv",
+					   cmdqv);
+		if (ret) {
+			dev_err(cmdqv->dev, "failed to request irq (%d): %d\n",
+				cmdqv->irq, ret);
+			goto free_vintfs;
+		}
+	}
+
 #ifdef CONFIG_IOMMU_DEBUGFS
 	if (!cmdqv_debugfs_dir) {
 		cmdqv_debugfs_dir =

@@ -1012,9 +1016,9 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 
 	return new_smmu;
 
-free_irq:
-	if (cmdqv->irq > 0)
-		free_irq(cmdqv->irq, cmdqv);
+free_vintfs:
+	ida_destroy(&cmdqv->vintf_ids);
+	kfree(cmdqv->vintfs);
 iounmap:
 	iounmap(base);
 	return NULL;
Signature · 1 line
2.43.0
author
#5 lore raw

PATCH v2 5/11 iommu/tegra241-cmdqv: Don't fall back to a freed smmu after devm_krealloc()

In reply to Nicolin Chen

__tegra241_cmdqv_probe() uses devm_krealloc() to grow @smmu into the larger tegra241_cmdqv, which frees the original @smmu once it relocates. A failure after that returned NULL, and the caller then dereferenced the freed @smmu on its fallback path.

Return an int and take @smmu by reference instead, then update *smmu to the reallocated pointer after devm_krealloc() succeeds, so the caller and its fallback path both use the live @smmu rather than the freed original.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 54 +++++++++++--------
 1 file changed, 32 insertions(+), 22 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +32 -22
index 7e366030d9d70..262798cd6b8a8 100644
@@ -934,16 +934,22 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
 static struct dentry *cmdqv_debugfs_dir;
 #endif
 
-static struct arm_smmu_device *
-__tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
-		       int irq)
+/*
+ * Probe the CMDQV and reallocate @smmu into the larger cmdqv->smmu.
+ *
+ * devm_krealloc() may relocate and free the original @smmu, so update *smmu to
+ * the new pointer once it succeeds. The error paths after it do the same, so a
+ * caller falling back keeps a live @smmu instead of the freed original.
+ */
+static int __tegra241_cmdqv_probe(struct arm_smmu_device **smmu,
+				  struct resource *res, int irq)
 {
 	static const struct arm_smmu_impl_ops init_ops = {
 		.init_structures = tegra241_cmdqv_init_structures,
 		.device_remove = tegra241_cmdqv_remove,
 	};
-	struct tegra241_cmdqv *cmdqv = NULL;
-	struct arm_smmu_device *new_smmu;
+	struct device *dev = (*smmu)->dev;
+	struct tegra241_cmdqv *cmdqv;
 	void __iomem *base;
 	u32 regval;
 	int ret;

@@ -952,25 +958,28 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 
 	base = ioremap(res->start, resource_size(res));
 	if (!base) {
-		dev_err(smmu->dev, "failed to ioremap\n");
-		return NULL;
+		dev_err(dev, "failed to ioremap\n");
+		return -ENOMEM;
 	}
 
 	regval = readl(base + TEGRA241_CMDQV_CONFIG);
 	if (disable_cmdqv) {
-		dev_info(smmu->dev, "Detected disable_cmdqv=true\n");
+		dev_info(dev, "Detected disable_cmdqv=true\n");
 		writel(regval & ~CMDQV_EN, base + TEGRA241_CMDQV_CONFIG);
+		ret = -ENODEV;
 		goto iounmap;
 	}
 
-	cmdqv = devm_krealloc(smmu->dev, smmu, sizeof(*cmdqv), GFP_KERNEL);
-	if (!cmdqv)
+	cmdqv = devm_krealloc(dev, *smmu, sizeof(*cmdqv), GFP_KERNEL);
+	if (!cmdqv) {
+		ret = -ENOMEM;
 		goto iounmap;
-	new_smmu = &cmdqv->smmu;
+	}
+	*smmu = &cmdqv->smmu;
 
 	cmdqv->irq = irq;
 	cmdqv->base = base;
-	cmdqv->dev = smmu->impl_dev;
+	cmdqv->dev = (*smmu)->impl_dev;
 	cmdqv->base_phys = res->start;
 
 	regval = readl_relaxed(REG_CMDQV(cmdqv, PARAM));

@@ -982,8 +991,10 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 
 	cmdqv->vintfs =
 		kzalloc_objs(*cmdqv->vintfs, cmdqv->num_vintfs);
-	if (!cmdqv->vintfs)
+	if (!cmdqv->vintfs) {
+		ret = -ENOMEM;
 		goto iounmap;
+	}
 
 	ida_init(&cmdqv->vintf_ids);
 

@@ -1012,24 +1023,23 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 #endif
 
 	/* Provide init-level ops only, until tegra241_cmdqv_init_structures */
-	new_smmu->impl_ops = &init_ops;
+	cmdqv->smmu.impl_ops = &init_ops;
 
-	return new_smmu;
+	return 0;
 
 free_vintfs:
 	ida_destroy(&cmdqv->vintf_ids);
 	kfree(cmdqv->vintfs);
 iounmap:
 	iounmap(base);
-	return NULL;
+	return ret;
 }
 
 struct arm_smmu_device *tegra241_cmdqv_probe(struct arm_smmu_device *smmu)
 {
 	struct platform_device *pdev = to_platform_device(smmu->impl_dev);
-	struct arm_smmu_device *new_smmu;
 	struct resource *res;
-	int irq;
+	int irq, ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {

@@ -1042,15 +1052,15 @@ struct arm_smmu_device *tegra241_cmdqv_probe(struct arm_smmu_device *smmu)
 		dev_warn(&pdev->dev,
 			 "no interrupt. errors will not be reported\n");
 
-	new_smmu = __tegra241_cmdqv_probe(smmu, res, irq);
-	if (new_smmu)
-		return new_smmu;
+	ret = __tegra241_cmdqv_probe(&smmu, res, irq);
+	if (!ret)
+		return smmu;
 
 out_fallback:
 	dev_info(smmu->impl_dev, "Falling back to standard SMMU CMDQ\n");
 	smmu->options &= ~ARM_SMMU_OPT_TEGRA241_CMDQV;
 	put_device(smmu->impl_dev);
-	return ERR_PTR(-ENODEV);
+	return smmu;
 }
 
 /* User space VINTF and VCMDQ Functions */
Signature · 1 line
2.43.0
author
#6 lore raw

PATCH v2 6/11 iommu/tegra241-cmdqv: Free the error IRQ before tearing down VINTFs

In reply to Nicolin Chen

tegra241_cmdqv_remove() tears each VINTF down first, then calls free_irq(). Tearing a VINTF down frees vintf0 and clears cmdqv->vintfs[0]. An error in that window makes tegra241_cmdqv_isr() read the stale slot and hand it to tegra241_vintf0_handle_error(), which dereferences a NULL or freed pointer.

Free the IRQ before tearing the VINTFs down. free_irq() waits for in-flight handlers to finish and blocks new ones, so no ISR can observe a VINTF as it is torn down.

Note: a user-owned VINTF (viommu) could outlive this teardown, which unmaps cmdqv->base and frees cmdqv->vintfs, so a later viommu close then touches freed memory. This is neither introduced nor fixed here: a physical IOMMU is not a pluggable device, so iommufd by design holds no reference on the one behind a viommu, and this teardown is not expected while that viommu is still alive.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +8 -2
index 262798cd6b8a8..fbf7ecf043a8c 100644
@@ -829,6 +829,14 @@ static void tegra241_cmdqv_remove(struct arm_smmu_device *smmu)
 		container_of(smmu, struct tegra241_cmdqv, smmu);
 	u16 idx;
 
+	/*
+	 * Free the IRQ before tearing down the VINTFs. free_irq() waits for any
+	 * in-flight tegra241_cmdqv_isr() to finish and blocks new ones, so the
+	 * ISR cannot dereference a VINTF that is freed by the loop below.
+	 */
+	if (cmdqv->irq > 0)
+		free_irq(cmdqv->irq, cmdqv);
+
 	/* Remove VINTF resources */
 	for (idx = 0; idx < cmdqv->num_vintfs; idx++) {
 		if (cmdqv->vintfs[idx]) {

@@ -841,8 +849,6 @@ static void tegra241_cmdqv_remove(struct arm_smmu_device *smmu)
 	/* Remove cmdqv resources */
 	ida_destroy(&cmdqv->vintf_ids);
 
-	if (cmdqv->irq > 0)
-		free_irq(cmdqv->irq, cmdqv);
 	iounmap(cmdqv->base);
 	kfree(cmdqv->vintfs);
 	put_device(cmdqv->dev); /* smmu->impl_dev */
Signature · 1 line
2.43.0
author
#7 lore raw

PATCH v2 7/11 iommu/tegra241-cmdqv: Reject a vSID wider than the SID_MATCH field

In reply to Nicolin Chen

tegra241_vintf_init_vsid() programs the guest-provided vSID into SID_MATCH, whose VIRT_SID field spans bits [20:1] with bit 0 as the match-enable flag. The HW therefore matches only a 20-bit Stream ID.

The bound check rejects only virt_sid > UINT_MAX, which admits a value far wider than the field. The write "virt_sid << 1 | 0x1" then drops every bit above 20: a virt_sid of 0x80000000 lands as SID_MATCH = 0x1, a valid match on vSID 0, so the entry aliases the wrong Stream ID. Because vdev->virt_id is guest-controlled, a VMM can trigger it.

Validate virt_sid against the field width with FIELD_MAX(), and program the register with FIELD_PREP() so the value and the field stay consistent.

Fixes 4dc0d12474f9 ("iommu/tegra241-cmdqv: Add user-space use support")
Cc [email protected]
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +6 -2
index fbf7ecf043a8c..812cc500b4a1e 100644
@@ -56,6 +56,8 @@
 #define  VINTF_ENABLED			BIT(0)
 
 #define TEGRA241_VINTF_SID_MATCH(s)	(0x0040 + 0x4*(s))
+#define  VINTF_SID_MATCH_VIRT_SID	GENMASK(20, 1)
+#define  VINTF_SID_MATCH_ENABLE		BIT(0)
 #define TEGRA241_VINTF_SID_REPLACE(s)	(0x0080 + 0x4*(s))
 
 #define TEGRA241_VINTF_LVCMDQ_ERR_MAP_64(m) \

@@ -1244,7 +1246,7 @@ static int tegra241_vintf_init_vsid(struct iommufd_vdevice *vdev)
 	u64 virt_sid = vdev->virt_id;
 	int sidx;
 
-	if (virt_sid > UINT_MAX)
+	if (virt_sid > FIELD_MAX(VINTF_SID_MATCH_VIRT_SID))
 		return -EINVAL;
 
 	WARN_ON_ONCE(master->num_streams != 1);

@@ -1256,7 +1258,9 @@ static int tegra241_vintf_init_vsid(struct iommufd_vdevice *vdev)
 		return sidx;
 
 	writel(stream->id, REG_VINTF(vintf, SID_REPLACE(sidx)));
-	writel(virt_sid << 1 | 0x1, REG_VINTF(vintf, SID_MATCH(sidx)));
+	writel(FIELD_PREP(VINTF_SID_MATCH_VIRT_SID, virt_sid) |
+		       VINTF_SID_MATCH_ENABLE,
+	       REG_VINTF(vintf, SID_MATCH(sidx)));
 	dev_dbg(vintf->cmdqv->dev,
 		"VINTF%u: allocated SID_REPLACE%d for pSID=%x, vSID=%x\n",
 		vintf->idx, sidx, stream->id, (u32)virt_sid);
Signature · 1 line
2.43.0
author
#8 lore raw

PATCH v2 8/11 iommu/tegra241-cmdqv: Require exactly one Stream ID for a vSID

In reply to Nicolin Chen

tegra241_vintf_init_vsid() maps a guest vSID to a single physical Stream ID taken from master->streams[0], and only warns when the device does not have 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.

Reject the mapping with -EOPNOTSUPP if master->num_streams is not one.

Fixes 4dc0d12474f9 ("iommu/tegra241-cmdqv: Add user-space use support")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +2 -1
index 812cc500b4a1e..e3603cddde018 100644
@@ -1249,7 +1249,8 @@ static int tegra241_vintf_init_vsid(struct iommufd_vdevice *vdev)
 	if (virt_sid > FIELD_MAX(VINTF_SID_MATCH_VIRT_SID))
 		return -EINVAL;
 
-	WARN_ON_ONCE(master->num_streams != 1);
+	if (master->num_streams != 1)
+		return -EOPNOTSUPP;
 
 	/* Find an empty pair of SID_REPLACE and SID_MATCH */
 	sidx = ida_alloc_max(&vintf->sids, vintf->cmdqv->num_sids_per_vintf - 1,
Signature · 1 line
2.43.0
author
#9 lore raw

PATCH v2 9/11 iommu/tegra241-cmdqv: Fix VINTF0 leak on the init-failure path

In reply to Nicolin Chen

tegra241_cmdqv_init_structures() allocates VINTF0 with kzalloc_obj(), inits it, and preallocates its logical VCMDQs. Two of its error paths leak.

When tegra241_cmdqv_init_vintf() fails it returns before VINTF0 reaches the cmdqv->vintfs[] array, so the devres unwind on probe failure cannot reach it; free it directly there.

A later VCMDQ preallocation failure instead leaves VINTF0 published, and so this time the unwind does reach tegra241_cmdqv_remove_vintf(), which then frees it from vintf->hyp_own. But tegra241_vintf_hw_init() sets that flag only afterward, from a HW read-back, so the still-uninited VINTF0 reads as guest-owned and leaks, with mutex_destroy() and ida_destroy() run on fields it never set up.

Decide ownership from vintf->idx instead, the index assigned when its id is
allocated: idx 0 is the kernel-owned VINTF0, while idx >= 1 marks a guest VINTF. So the in-kernel free decision in tegra241_cmdqv_remove_vintf() and tegra241_vintf_free_lvcmdq() now keys on idx too, and hyp_own stays a pure HW-readback state.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +8 -2
index e3603cddde018..f93e59b7bd2db 100644
@@ -716,7 +716,7 @@ static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
 	dev_dbg(vintf->cmdqv->dev,
 		"%sdeallocated\n", lvcmdq_error_header(vcmdq, header, 64));
 	/* Guest-owned VCMDQ is free-ed with hw_queue by iommufd core */
-	if (vcmdq->vintf->hyp_own)
+	if (!vcmdq->vintf->idx)
 		kfree(vcmdq);
 }
 

@@ -816,7 +816,7 @@ static void tegra241_cmdqv_remove_vintf(struct tegra241_cmdqv *cmdqv, u16 idx)
 
 	dev_dbg(cmdqv->dev, "VINTF%u: deallocated\n", vintf->idx);
 	tegra241_cmdqv_deinit_vintf(cmdqv, idx);
-	if (!vintf->hyp_own) {
+	if (vintf->idx) {
 		mutex_destroy(&vintf->lvcmdq_mutex);
 		ida_destroy(&vintf->sids);
 		/* Guest-owned VINTF is free-ed with viommu by iommufd core */

@@ -921,6 +921,12 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
 	ret = tegra241_cmdqv_init_vintf(cmdqv, 0, vintf);
 	if (ret) {
 		dev_err(cmdqv->dev, "failed to init vintf0: %d\n", ret);
+		/*
+		 * tegra241_cmdqv_init_vintf() failed to publish the vintf0 to
+		 * cmdqv->vintfs[], so the probe unwind path that goes through
+		 * cmdqv->vintfs[] would miss it. Free it here.
+		 */
+		kfree(vintf);
 		return ret;
 	}
Signature · 1 line
2.43.0
author
#10 lore raw

PATCH v2 10/11 iommu/tegra241-cmdqv: Warn on a VCMDQ base above the 48-bit hardware limit

In reply to Nicolin Chen

tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask of DMA_BIT_MASK(smmu->oas). The architectural Q_BASE_ADDR_MASK is 52 bits, but the Tegra241 VCMDQ_BASE holds only 48 (VCMDQ_ADDR), so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48 and up.

A real Tegra241 never reports a 52-bit OAS alongside the 48-bit VCMDQ, so this cannot happen on correct hardware, but a buggy or hostile hypervisor could still advertise such an OAS to a guest. The user-VCMDQ path already rejects a base_addr_pa with bits set outside VCMDQ_ADDR; add the same guard to the kernel-allocated path, failing the queue init rather than silently truncating the base. Use dev_warn_once() rather than WARN_ON(): the OAS is hypervisor-controlled, and a WARN_ON() would let it panic a guest that is booted with panic_on_warn.

Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +13 -0
index f93e59b7bd2db..d27ee215032f7 100644
@@ -674,6 +674,19 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
 	if (ret)
 		return ret;
 
+	/*
+	 * The q->base_dma is bounded by DMA_BIT_MASK of SMMU's IDR5.OAS. On a
+	 * real hardware, VCMDQ_ADDR mask and IDR5.OAS are always aligned. But
+	 * a buggy VM might set a mismatched IDR5.OAS for SMMU. Spit a warning
+	 * instead of a silent truncation.
+	 */
+	if (q->base_dma & ~VCMDQ_ADDR) {
+		dev_warn_once(
+			vcmdq->cmdqv->dev,
+			"VCMDQ base %pad exceeds the 48-bit VCMDQ_ADDR limit\n",
+			&q->base_dma);
+		return -EINVAL;
+	}
 	/* ...override q_base to write VCMDQ_BASE registers */
 	q->q_base = q->base_dma & VCMDQ_ADDR;
 	q->q_base |= FIELD_PREP(VCMDQ_LOG2SIZE, q->llq.max_n_shift);
Signature · 1 line
2.43.0
maintainer
#11 lore raw

Re: [PATCH v2 10/11] iommu/tegra241-cmdqv: Warn on a VCMDQ base above the 48-bit hardware limit

Quoted 6 lines · Nicolin Chen

tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask of DMA_BIT_MASK(smmu->oas). The architectural Q_BASE_ADDR_MASK is 52 bits, but the Tegra241 VCMDQ_BASE holds only 48 (VCMDQ_ADDR), so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48 and up.

Quoted 8 lines

A real Tegra241 never reports a 52-bit OAS alongside the 48-bit VCMDQ, so this cannot happen on correct hardware, but a buggy or hostile hypervisor could still advertise such an OAS to a guest. The user-VCMDQ path already rejects a base_addr_pa with bits set outside VCMDQ_ADDR; add the same guard to the kernel-allocated path, failing the queue init rather than silently truncating the base. Use dev_warn_once() rather than WARN_ON(): the OAS is hypervisor-controlled, and a WARN_ON() would let it panic a guest that is booted with panic_on_warn.

So here you talk about "a buggy or hostile hypervisor". Is that really part of the threat model? Presumably such a thing could cause havoc in a million other ways?

Quoted 11 lines
Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +0 -0
index f93e59b7bd2db..d27ee215032f7 100644

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq) · line 682

Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +6 -0
@@ -674,6 +674,19 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
 	if (ret)
 		return ret;
 
+	/*
+	 * The q->base_dma is bounded by DMA_BIT_MASK of SMMU's IDR5.OAS. On a
+	 * real hardware, VCMDQ_ADDR mask and IDR5.OAS are always aligned. But
+	 * a buggy VM might set a mismatched IDR5.OAS for SMMU. Spit a warning
+	 * instead of a silent truncation.
+	 */

But then here you talk about "a buggy VM might set a mismatched IDR5.OAS". How can a VM write to that?

/confused

Will

author
#12 lore raw

Re: [PATCH v2 10/11] iommu/tegra241-cmdqv: Warn on a VCMDQ base above the 48-bit hardware limit

Quoted 16 lines · Will Deacon
Quoted 6 lines · Nicolin Chen

tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask of DMA_BIT_MASK(smmu->oas). The architectural Q_BASE_ADDR_MASK is 52 bits, but the Tegra241 VCMDQ_BASE holds only 48 (VCMDQ_ADDR), so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48 and up.

Quoted 8 lines

A real Tegra241 never reports a 52-bit OAS alongside the 48-bit VCMDQ, so this cannot happen on correct hardware, but a buggy or hostile hypervisor could still advertise such an OAS to a guest. The user-VCMDQ path already rejects a base_addr_pa with bits set outside VCMDQ_ADDR; add the same guard to the kernel-allocated path, failing the queue init rather than silently truncating the base. Use dev_warn_once() rather than WARN_ON(): the OAS is hypervisor-controlled, and a WARN_ON() would let it panic a guest that is booted with panic_on_warn.

Quoted 2 lines

So here you talk about "a buggy or hostile hypervisor". Is that really part of the threat model?

Yea, existing real chips have a matched 48-bit address field with OAS=5 (48-bit). The only possible scenario is a buggy VMM exposing a mismatched OAS value to the VM. Perhaps we could drop "hostile".

Quoted 2 lines

Presumably such a thing could cause havoc in
a million other ways?

Maybe, it's a defensive play here.

A buggy VMM can absorb the mismatch for the SMMU CMDQ, by reading the high-order bits of the Q_BASE_ADDR field, since the guest SMMU driver wouldn't truncate those bits. But, in VCMDQ case, it would get truncated silently, which can lead to VCMDQ errors (IRQs).

Quoted 4 lines
Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +0 -0
@@ -674,6 +674,19 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
 	if (ret)
 		return ret;
Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq) · line 682

Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +6 -0
+	/*
+	 * The q->base_dma is bounded by DMA_BIT_MASK of SMMU's IDR5.OAS. On a
+	 * real hardware, VCMDQ_ADDR mask and IDR5.OAS are always aligned. But
+	 * a buggy VM might set a mismatched IDR5.OAS for SMMU. Spit a warning
+	 * instead of a silent truncation.
+	 */

But then here you talk about "a buggy VM might set a mismatched IDR5.OAS". How can a VM write to that?

Oh, this is a typo; it should be "VMM".

Thanks
Nicolin

author
#13 lore raw

PATCH v2 11/11 iommu/tegra241-cmdqv: Rate-limit the error ISR's log message

In reply to Nicolin Chen

tegra241_cmdqv_isr() logs the error-map registers on every error interrupt. A malfunctioning device, or a guest deliberately faulting its own VCMDQs, can raise these interrupts rapidly, and the unconditional dev_warn() then floods the kernel log.

Rate-limit the message with dev_warn_ratelimited(), and pass the error-map registers straight to it so their four MMIO reads run only when the limiter prints, instead of building the string on every interrupt.

Assisted-by Claude:claude-opus-4-8
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +7 -9
index d27ee215032f7..6fa981574db9f 100644
@@ -353,21 +353,19 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 {
 	struct tegra241_cmdqv *cmdqv = (struct tegra241_cmdqv *)devid;
 	void __iomem *reg_vintf_map = REG_CMDQV(cmdqv, VINTF_ERR_MAP);
-	char err_str[256];
 	u64 vintf_map;
 
 	/* Use readl_relaxed() as register addresses are not 64-bit aligned */
 	vintf_map = (u64)readl_relaxed(reg_vintf_map + 0x4) << 32 |
 		    (u64)readl_relaxed(reg_vintf_map);
 
-	snprintf(err_str, sizeof(err_str),
-		 "vintf_map: %016llx, vcmdq_map %08x:%08x:%08x:%08x", vintf_map,
-		 readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(3))),
-		 readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(2))),
-		 readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(1))),
-		 readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(0))));
-
-	dev_warn(cmdqv->dev, "unexpected error reported. %s\n", err_str);
+	dev_warn_ratelimited(
+		cmdqv->dev,
+		"unexpected error reported. vintf_map: %016llx, vcmdq_map %08x:%08x:%08x:%08x\n",
+		vintf_map, readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(3))),
+		readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(2))),
+		readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(1))),
+		readl_relaxed(REG_CMDQV(cmdqv, CMDQ_ERR_MAP(0))));
 
 	/* Handle VINTF0 and its LVCMDQs */
 	if (vintf_map & BIT_ULL(0)) {
Signature · 1 line
2.43.0
author
#14 lore raw
In reply to Nicolin Chen
Quoted 6 lines · Nicolin Chen

False positives raised by Sashiko:
- a viommu outliving an SMMU unbind and touching freed memory on close: a physical IOMMU is not a pluggable device, so iommufd holds no reference on the one behind a viommu, and this teardown cannot arise.
- the ISR running after cmdqv is freed on probe failure: free_irq() runs first from tegra241_cmdqv_remove(), the devm device_remove action,

Quoted 8 lines

which devres invokes before the cmdqv allocation is released.
- a guest never acking its VCMDQ error wedging the shared interrupt: the interrupt is edge-signaled per event, and the host ISR only snapshots the error map into the guest's bounded vEVENTQ, never depending on a guest-side GERRORN ack.
- the ISR accessing a de-assigned LVCMDQ page after a VINTF hw_init()
failure: the page remains a mapped MMIO region backed by empty registers, so reads are benign and writes are dropped.

Adding one more false positive:

Sashiko reported against this v2 a critical issue stating that CMDQV is level-triggered and could cause trouble in kdump case.

But CMDQV is edge-triggered; one of the ISR patches noted very clearly.

Nicolin

maintainer
#15 lore raw
In reply to Nicolin Chen
Quoted 15 lines · Nicolin Chen

These fix a cluster of bugs reported by Sashiko during patch reviews. The patches are ordered roughly most-critical-first, so some later ones fix smaller pre-existing issues in the same functions that earlier patches touch.

Issues fixed:
- the error ISR racing VINTF (de)init and reading a NULL, freed, or not yet fully initialized slot
- the probe fallback dereferencing an smmu freed by devm_krealloc()
- a guest vSID programmed without validating its width or the device's Stream ID count
- VINTF0 leaked on an init-failure path
- error-map index/bounds handling and a VCMDQ base above the 48-bit limit
- the error ISR flooding the kernel log under repeated guest errors

Quoted 1 line

[...]

Applied (apart from patch 10) to iommu (arm/smmu/updates), thanks!

[01/11] iommu/tegra241-cmdqv: Publish an LVCMDQ only after it is fully initialized

        https://git.kernel.org/iommu/c/cbc41aacd49e
[02/11] iommu/tegra241-cmdqv: Synchronize the error ISR against VINTF (de)init
        https://git.kernel.org/iommu/c/a491be376abd
[03/11] iommu/tegra241-cmdqv: Harden error-map index handling in the error ISR
        https://git.kernel.org/iommu/c/a2ee315db426
[04/11] iommu/tegra241-cmdqv: Don't run the error ISR before probe sets up vintfs
        https://git.kernel.org/iommu/c/5acd67ceb38d
[05/11] iommu/tegra241-cmdqv: Don't fall back to a freed smmu after devm_krealloc()
        https://git.kernel.org/iommu/c/d4d05f55e9da
[06/11] iommu/tegra241-cmdqv: Free the error IRQ before tearing down VINTFs
        https://git.kernel.org/iommu/c/61f0d437988e
[07/11] iommu/tegra241-cmdqv: Reject a vSID wider than the SID_MATCH field
        https://git.kernel.org/iommu/c/4379610c79bd
[08/11] iommu/tegra241-cmdqv: Require exactly one Stream ID for a vSID
        https://git.kernel.org/iommu/c/fb292bfc9be9
[09/11] iommu/tegra241-cmdqv: Fix VINTF0 leak on the init-failure path
        https://git.kernel.org/iommu/c/f40f31444773
[11/11] iommu/tegra241-cmdqv: Rate-limit the error ISR's log message
        https://git.kernel.org/iommu/c/a86c36163c97

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
author
#16 lore raw
Quoted 16 lines · Will Deacon
Quoted 15 lines · Nicolin Chen

These fix a cluster of bugs reported by Sashiko during patch reviews. The patches are ordered roughly most-critical-first, so some later ones fix smaller pre-existing issues in the same functions that earlier patches touch.

Issues fixed:
- the error ISR racing VINTF (de)init and reading a NULL, freed, or not yet fully initialized slot
- the probe fallback dereferencing an smmu freed by devm_krealloc()
- a guest vSID programmed without validating its width or the device's Stream ID count
- VINTF0 leaked on an init-failure path
- error-map index/bounds handling and a VCMDQ base above the 48-bit limit
- the error ISR flooding the kernel log under repeated guest errors

Quoted 3 lines
Quoted 1 line

[...]

Applied (apart from patch 10) to iommu (arm/smmu/updates), thanks!

Should I respin PATCH-10? Or do you prefer dropping it?

Thanks
Nicolin