mce: fix race condition in mctelem_xchg_head

The function (mctelem_xchg_head()) used to exchange mce telemetry
list heads is racy.  It may write to the head twice, with the second
write linking to an element in the wrong state.

If there are two threads, T1 inserting on committed list; and T2
trying to consume it.

1. T1 starts inserting an element (A), sets prev pointer (mcte_prev).
2. T1 is interrupted after the cmpxchg succeeded.
3. T2 gets the list and changes element A and updates the commit list
   head.
4. T1 resumes, reads pointer to prev again and compare with result
   from the cmpxchg which succeeded but in the meantime prev changed
   in memory.
5. T1 thinks the cmpxchg failed and goes around the loop again,
   linking head to A again.

To solve the race use temporary variable for prev pointer.

*linkp (which point to a field in the element) must be updated before
the cmpxchg() as after a successful cmpxchg the element might be
immediately removed and reinitialized.

The wmb() prior to the cmpchgptr() call is not necessary since it is
already a full memory barrier.  This wmb() is thus removed.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Liu Jinsong <jinsong.liu@intel.com>

diff --git a/xen/arch/x86/cpu/mcheck/mctelem.c b/xen/arch/x86/cpu/mcheck/mctelem.c
index 37d830f3b7f9a8676b42b00b00a0486afc7173d0..895ce1adf23de8b09f1cf88c12335f993e61c092 100644 (file)
--- a/xen/arch/x86/cpu/mcheck/mctelem.c
+++ b/xen/arch/x86/cpu/mcheck/mctelem.c
@@ -127,13 +127,14 @@ static DEFINE_PER_CPU(struct mc_telem_cpu_ctl, mctctl);
 static DEFINE_SPINLOCK(processing_lock);
 
 static void mctelem_xchg_head(struct mctelem_ent **headp,
-                               struct mctelem_ent **old,
+                               struct mctelem_ent **linkp,
                                struct mctelem_ent *new)
 {
        for (;;) {
-               *old = *headp;
-               wmb();
-               if (cmpxchgptr(headp, *old, new) == *old)
+               struct mctelem_ent *old;
+
+               *linkp = old = *headp;
+               if (cmpxchgptr(headp, old, new) == old)
                        break;
        }
 }