Re: [PATCH v5 11/18] iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when CMD_SYNC times out

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Sleeping in atomic context during Service Failure Mode (SFM) error handling.
- [High] Race condition in teardown leading to UAF / NULL pointer dereference in the threaded ISR.
--

commit 3f49807deb6de6a8ccbe2ea880ec3f8f51819f03
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when CMD_SYNC times out

This patch adds a synchronous mechanism to drain pending CMDQ_ERRs when a
CMD_SYNC times out. It introduces cmdq_err_handler callbacks for both the
standard ARM SMMU-v3 command queue and the Tegra241 VCMDQ to prevent missing
ATC_INV timeouts if the generic ISR is delayed.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index c6e3d1be23403..4b4e8108d5944 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -2298,6 +2307,26 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)
>  
>  static int arm_smmu_device_disable(struct arm_smmu_device *smmu);
>  
> +/* Drain a pending CMDQ_ERR, used by arm_smmu_cmdq_issue_cmdlist() */
> +static void arm_smmu_cmdq_err_handler(struct arm_smmu_device *smmu,
> +				      struct arm_smmu_cmdq *cmdq)
> +{
> +	u32 gerror, gerrorn;
> +
> +	guard(raw_spinlock_irqsave)(&cmdq->cmdq_err_lock);
> +
> +	gerror = readl_relaxed(smmu->base + ARM_SMMU_GERROR);
> +	gerrorn = readl_relaxed(smmu->base + ARM_SMMU_GERRORN);
> +
> +	if (!((gerror ^ gerrorn) & GERROR_CMDQ_ERR))
> +		return;
> +
> +	__arm_smmu_cmdq_skip_err(smmu, cmdq);
> +
> +	/* Toggle only the CMDQ_ERR bit; other bits are left for the ISR. */
> +	writel(gerrorn ^ GERROR_CMDQ_ERR, smmu->base + ARM_SMMU_GERRORN);
> +}
> +
>  static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but could this hardirq handler
cause a scheduling while atomic bug when handling SFM errors?

Looking at arm_smmu_gerror_handler(), when the SMMU hardware reports a Service
Failure Mode (GERROR_SFM_ERR), it calls arm_smmu_device_disable(smmu).

This in turn calls arm_smmu_write_reg_sync(), which performs a polling loop:

	return readl_relaxed_poll_timeout(smmu->base + ack_off, reg, reg == val,
					  1, ARM_SMMU_POLL_TIMEOUT_US);

Because readl_relaxed_poll_timeout() uses a non-zero delay (1us), it
internally uses usleep_range() which can sleep.

Since arm_smmu_gerror_handler() executes in hardirq context, will this sleep
lead to a kernel panic?

>  	u32 gerror, gerrorn, active;
[ ... ]
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 666dd23b0c7ca..628a3a7cc0335 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -337,6 +337,28 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
>  	}
>  }
>  
> +/* Drain a pending CMDQ_ERR, used by arm_smmu_cmdq_issue_cmdlist() */
> +static void tegra241_vcmdq_handle_cmdq_err(struct arm_smmu_device *smmu,
> +					   struct arm_smmu_cmdq *cmdq)
> +{
> +	struct tegra241_vcmdq *vcmdq =
> +		container_of(cmdq, struct tegra241_vcmdq, cmdq);
> +	u32 gerror, gerrorn;
> +
> +	guard(raw_spinlock_irqsave)(&cmdq->cmdq_err_lock);
> +
> +	gerror = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERROR));
> +	gerrorn = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERRORN));
> +
> +	if (!((gerror ^ gerrorn) & GERROR_CMDQ_ERR))
> +		return;
> +
> +	__arm_smmu_cmdq_skip_err(smmu, cmdq);
> +
> +	/* Toggle only the CMDQ_ERR bit on this VCMDQ's GERRORN */
> +	writel(gerrorn ^ GERROR_CMDQ_ERR, REG_VCMDQ_PAGE0(vcmdq, GERRORN));
> +}
> +
>  static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
in teardown that could lead to a NULL pointer dereference or use-after-free
in this ISR?

In tegra241_cmdqv_remove(), the driver appears to free vintf objects and
nullify pointers before freeing the interrupt:

	tegra241_cmdqv_remove_vintf(cmdqv, idx);
	...
	free_irq(cmdqv->irq, cmdqv);

Since tegra241_cmdqv_isr() is a threaded IRQ handler, if a concurrent or
pending interrupt wakes the IRQ thread during this teardown window, could it
pass a NULL or freed cmdqv->vintfs[0] to tegra241_vintf0_handle_error()?

The tegra241_vintf0_handle_error() function immediately dereferences the
pointer:

	u64 map = readq_relaxed(REG_VINTF(vintf, LVCMDQ_ERR_MAP_64(i)));

Could this cause a kernel panic during device unbind or removal?

>  	struct tegra241_cmdqv *cmdqv = (struct tegra241_cmdqv *)devid;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783044582.git.nicolinc@nvidia.com?part=11