[Xen-devel] [PATCH v2] mce: Fix race condition in mctelem_xchg_head

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
 xen/arch/x86/cpu/mcheck/mctelem.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Version 2:
- changed commit message;
- remove useless wmb().

diff --git a/xen/arch/x86/cpu/mcheck/mctelem.c 
b/xen/arch/x86/cpu/mcheck/mctelem.c
index 37d830f..895ce1a 100644
--- 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;
        }
 }
-- 
1.7.10.4



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel