Re: [PATCH v8 3/4] iommu/arm-smmu-v3: Standardize ATS enablement failure reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly introduced WARN_ON() in arm_smmu_enable_ats() can be triggered from userspace for a Virtual Function (VF) due to an ignored probe-time error, potentially crashing the kernel.
- [Low] The newly added inline comment and commit message contain factual inaccuracies regarding STE programming and probe-time validation.
--

commit e98cc7d134471b7c205436036caf90a8f69d9088
Author: Pranjal Shrivastava <praan@google.com>

iommu/arm-smmu-v3: Standardize ATS enablement failure reporting

Update arm_smmu_enable_ats() to wrap the pci_enable_ats() call in
WARN_ON(). Since probe-time checks now preclude configuration errors
any failure during hardware enablement is considered a kernel bug.

> Since probe-time checks now preclude configuration errors
> any failure during hardware enablement is considered a kernel bug.

[Severity: Low]
This isn't a bug, but the commit message states that probe-time checks
now preclude configuration errors. As noted below, the return value of
pci_prepare_ats() isn't checked during probe, so this description doesn't
seem to match the actual driver behavior.

> 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 e8d7dbe495f03..a9bec5c7cbf98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3065,7 +3065,12 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
>  	 * ATC invalidation of PASID 0 causes the entire ATC to be flushed.
>  	 */
>  	arm_smmu_atc_inv_master(master, IOMMU_NO_PASID);
> -	if (pci_enable_ats(pdev, stu))
> +
> +	 /*
> +	  * Since pci_prepare_ats() has already verified the HW capability
> +	  * and programmed the STE, pci_enable_ats() should not fail here.
> +	  */

[Severity: Low]
This isn't a bug, but there's a typo in the comment here. pci_prepare_ats()
programs the STU (Smallest Translation Unit) in the PCI ATS capability, not
the STE (Stream Table Entry), which is SMMU-specific.

> +	if (WARN_ON(pci_enable_ats(pdev, stu)))
>  		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
>  }

[Severity: High]
Is it possible for pci_enable_ats() to fail here and trigger this warning from
userspace?

Looking at arm_smmu_probe_device(), the return value of pci_prepare_ats()
appears to be ignored:

arm_smmu_probe_device() {
	...
	pci_prepare_ats(to_pci_dev(dev), stu);
	...
}

If a Virtual Function (VF) is probed but its Physical Function (PF) has a
mismatched STU, pci_prepare_ats() would fail silently during probe. Later,
when userspace triggers a domain attach (e.g., via VFIO),
arm_smmu_enable_ats() would call pci_enable_ats() on the VF, which fails and
triggers this newly added WARN_ON.

Also, if the error is ignored here and the driver continues as if ATS is
enabled, could this trigger a second WARN_ON(!dev->ats_enabled) in
pci_disable_ats() during domain detachment?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604182116.3179005-1-praan@google.com?part=3