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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Thu, 2014-01-16 at 14:31 +0000, Jan Beulich wrote:
> >>> On 16.01.14 at 15:07, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> > 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.
> 
> Neither am I.
> 
> > 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.
> 
> All understood.
> 
> > 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.
> > 
> > No, this will create another race. After cmpxchg you must assume that
> > the list element can be own by somebody else.
> 
> "The list element" being which? I simply don't see how storing to
> *oldp non-atomically is race free when done before the cmpxchg,
> but is a problem when done afterwards. Are there perhaps
> additional requirement put on this by some (but not all) of the
> callers? I ask because the function by itself doesn't seem to have
> a race other than the one you try to fix, no matter where *oldp
> is being stored to. If indeed there are further requirements, then
> adding a comment here would seem necessary (since I'm sure
> there'd be me or someone else stumbling across that inefficiency
> again in the future, and hence wanting to clean it up in the same
> way I suggested).
> 
> Jan
> 

This function is used to safely add a element to a list. You pass:
- pointer to pointer to head (headp);
- pointer to pointer to next/prev pointer (old/oldp);
- pointer to list element (new).
Now you own (nobody should allow to touch) the element till it get
inserted to the list.
Now assume we set the pointer after the loop. The head points to an
element which has no next/prev set so you break the list. Another thread
will see a corrupted or shortened list.

Frediano



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