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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>  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


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