[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.