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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Thu, 2014-01-16 at 13:56 +0000, Jan Beulich wrote:
> >>> On 16.01.14 at 14:26, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> > The function that exchange the head is racy.
> > The cmpxchg and the compare are not atomic.
> > Assume two thread one (T1) inserting on committed list and another
> > trying to comsume it (T2).
> > T1 start inserting the element (A), set prev pointer (commit list use
> > mcte_prev) then is stop after the cmpxchg succeeded.
> > T2 get the list and change elements (A) and update the commit list
> > head.
> > T1 resume, read pointer to prev again and compare with result from
> > cmpxchg which succeeded but in the meantime prev changed in memory.
> > Not T1 assume the element was not inserted on the list and try to
> > insert again. Now A however is in another state and should not be
> > modified by T1.
> > To solve the race use temporary variable for prev pointer.
> > Note that compiler should not optimize the memory fetch as cmpxhg
> > do a full barrier.
> 
> This last sentence is pretty pointless, since there's an explicit
> wmb() between setting old and doing the cmpxchg. (Question is
> whether that wmb() actually is necessary; without a comment I
> can't easily see what it is supposed to fence - surely it's not the
> write to *oldp. And that's regardless of it being redundant with
> the barrier embedded with cmpxchgptr().)
> 
> But overall, while I think your analysis is correct, the description
> could do with some cleanup, also spelling-wise.
> 

Yes, sorry, I'm not native English. Suggestion accepted.

The race is here

if (cmpxchgptr(headp, *old, new) == *old)

You do the cmpxchg (which is atomic), then you read old pointer (*old)
then you compare cmpxchg result with the content of the pointer.

Yes, wmb is quite useless, cmpxchg should be enough.

> >  static void mctelem_xchg_head(struct mctelem_ent **headp,
> > -                           struct mctelem_ent **old,
> > +                           struct mctelem_ent **oldp,
> >                             struct mctelem_ent *new)
> >  {
> > +   struct mctelem_ent *old;
> > +
> >     for (;;) {
> > -           *old = *headp;
> > +           *oldp = old = *headp;
> >             wmb();
> > -           if (cmpxchgptr(headp, *old, new) == *old)
> > +           if (cmpxchgptr(headp, old, new) == old)
> >                     break;
> >     }
> >  }
> 
> Now that you use a temporary, it would make sense (and make
> the code easier to read) if you set *oldp to old just once, after
> the loop.
> 
> Jan
> 

No, this will create another race. After cmpxchg you must assume that
the list element can be own by somebody else.

Frediano




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