Re: [PATCH v3 07/13] iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The system shutdown path disables the SMMU without quiescing queue IRQs, leading to an infinite loop in the kernel thread.
--

commit 723b797a0d5bc30d8469f53716a51e3b4d1bb03c
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

This commit addresses an issue where disabling the SMMU while queue IRQs are
running leads to commands being issued to a queue consuming nothing. It
ensures that the queue IRQ threads are quiesced by disabling the IRQs before
the SMMU is disabled in device reset and disable action paths.

> 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 64540cfb73244..d45d97f356cf0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -4992,10 +5001,22 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
>  	return ret;
>  }
>  
> +/* Quiesce the queue IRQ threads, e.g. before disabling the SMMU */
> +static void arm_smmu_disable_irqs(struct arm_smmu_device *smmu)
> +{
> +	if (smmu->combined_irq)
> +		disable_irq(smmu->combined_irq);
> +	if (smmu->evtq.q.irq)
> +		disable_irq(smmu->evtq.q.irq);
> +	if (smmu->priq.q.irq)
> +		disable_irq(smmu->priq.q.irq);
> +}
> +
>  static void arm_smmu_disable_action(void *data)
>  {
>  	struct arm_smmu_device *smmu = data;
>  
> +	arm_smmu_disable_irqs(smmu);

[Severity: High]
This is a pre-existing issue, but since this patch explicitly aims to fix
the vulnerability where the SMMU is disabled without quiescing the IRQs,
does the system shutdown path also need to be updated to call
arm_smmu_disable_irqs()?

Looking at arm_smmu_device_shutdown() which is called during system shutdown:

static void arm_smmu_device_shutdown(struct platform_device *pdev)
{
	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);

	arm_smmu_device_disable(smmu);
}

The shutdown path directly disables the SMMU without first quiescing the
threaded IRQs. Because devm callbacks do not run on shutdown, the IRQ handlers
remain active while the hardware is disabled.

If a pending PRIQ or EVTQ event triggers, the threaded handler
(e.g., arm_smmu_priq_thread) executes and calls arm_smmu_page_response(),
which attempts to insert a PRI_RESP command into the CMDQ without waiting for
a SYNC. Since the SMMU is hardware-disabled, it will not process commands and
the CMDQ software ring buffer quickly fills up. Once full,
__arm_smmu_cmdq_issue_cmdlist() enters a while (!queue_has_space(...)) loop
which either spins tightly or times out every second, looping indefinitely
without any breakout condition.

Could we also add a call to arm_smmu_disable_irqs(smmu) in
arm_smmu_device_shutdown() to prevent this infinite loop?

>  	if (smmu->impl_ops && smmu->impl_ops->device_disable)
>  		smmu->impl_ops->device_disable(smmu);
>  	arm_smmu_device_disable(smmu);

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