iommu/amd: Wait for completion instead of returning early in iommu_completion_wait()

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@linux.alibaba.com>
Fixes: 815b33fdc279 ("x86/amd-iommu: Cleanup completion-wait handling")
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Will Deacon <will@kernel.org>

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 563f9c2672d57aa7ab09dffbc9fb4728e241f531..29dc18d3d22e538597cfdb5613466be413fce766 100644 (file)
--- 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)