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

[Vasant Hegde]

Hi Joerg, Will,

Can you please pick this patch for rc as it fixes important synchronization bug.



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.
> 
> Signed-off-by: Guanghui Feng <guanghuifeng@xxxxxxxxxxxxxxxxx>

We have reviewed/tested this patch. It looks good.

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

Reviewed-by: Vasant Hegde <vasant.hegde@xxxxxxx>


-Vasant






> ---
>  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)