Re: [PATCH] iommu/amd: Wait for completion instead of returning early in iommu_completion_wait()

[Vasant Hegde]

Gaunghui,

On 7/16/2026 7:46 PM, Guanghui Feng wrote:
> need_sync is a per-IOMMU flag shared by all domains and devices behind
> that IOMMU. It is set whenever a command is queued with sync == true and
> cleared when a completion-wait (CWAIT) command is queued. However, a
> cleared need_sync only means that a covering CWAIT has been queued, not
> that all previously queued commands have actually completed in hardware.
> 
> iommu_completion_wait() read need_sync locklessly and returned early
> when it was false. This breaks the "block until all previously queued
> commands have completed" contract in a multi-CPU scenario:
> 
>   CPU2: queue inv-B                  => need_sync = true
>   CPU1: queue CWAIT(N); need_sync = false; then wait_on_sem(N)
>   CPU2: read need_sync == false      => return 0 (no wait!)
> 
> CPU2 returns without waiting for any sequence number even though its
> inv-B may not have completed yet (CWAIT(N), queued after inv-B, has not
> been signaled). CPU2 then proceeds to, for example, free page-table
> pages while the IOMMU can still walk stale translations, opening a
> use-after-free window. This is a logical race in the meaning of the
> flag, not a memory-visibility issue, so barriers alone do not help.
> 
> Fix it without losing the optimization of avoiding redundant CWAIT
> commands: take iommu->lock before testing need_sync, and when it is
> false do not return early but wait for the last allocated sequence
> number (cmd_sem_val). Since need_sync == false implies no sync command
> was queued after the last CWAIT, that CWAIT is FIFO-ordered after every
> not-yet-completed command, so waiting for its sequence number guarantees
> all prior commands (possibly queued by another CPU) have completed. The
> common path with pending work is unchanged and no extra hardware command
> is issued.

This is really good finding/fix. Thank you.

We went back to investigated when we introduced this bug in kernel. Looks like
its very long standing issue in the kernel and for me its pointing to commit

815b33fdc279 ("x86/amd-iommu: Cleanup completion-wait handling")


We have to add Fixes tag and CC stable so that it gets picked up by stable branches.

Currently we are testing this patch internally and we will get back to you soon.

-Vasant

> 
> Signed-off-by: Guanghui Feng <guanghuifeng@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/amd/iommu.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 563f9c2672d5..29dc18d3d22e 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1450,11 +1450,23 @@ static int iommu_completion_wait(struct amd_iommu *iommu)
>  	int ret;
>  	u64 data;
>  
> -	if (!iommu->need_sync)
> -		return 0;
> -
>  	raw_spin_lock_irqsave(&iommu->lock, flags);
>  
> +	if (!iommu->need_sync) {
> +		/*
> +		 * No command has been queued since the last completion-wait.
> +		 * A concurrent CPU may have already queued that CWAIT and
> +		 * cleared need_sync; need_sync == false only means a covering
> +		 * CWAIT is queued, not that all prior commands have completed.
> +		 * Wait for the last allocated sequence number so that any
> +		 * command queued before this call (possibly on another CPU)
> +		 * is guaranteed to have completed before returning.
> +		 */
> +		data = iommu->cmd_sem_val;
> +		raw_spin_unlock_irqrestore(&iommu->lock, flags);
> +		return wait_on_sem(iommu, data);
> +	}
> +
>  	data = get_cmdsem_val(iommu);
>  	build_completion_wait(&cmd, iommu, data);
>  
> @@ -1464,9 +1476,7 @@ static int iommu_completion_wait(struct amd_iommu *iommu)
>  	if (ret)
>  		return ret;
>  
> -	ret = wait_on_sem(iommu, data);
> -
> -	return ret;
> +	return wait_on_sem(iommu, data);
>  }
>  
>  static void domain_flush_complete(struct protection_domain *domain)