[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1432075664.22607.12.camel@inria.fr>
Date: Wed, 20 May 2015 00:47:44 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: trouble spots for atomic access
Am Dienstag, den 19.05.2015, 18:07 -0400 schrieb Rich Felker:
> On Tue, May 19, 2015 at 03:57:00PM +0200, Jens Gustedt wrote:
> > Hello,
> > by forcing the compiler to detect consistency checks for
> > atomics as I mentioned earlier, I detected 5 trouble spots. The first
> > four are relatively clear:
> >
> > - a_and and a_or interfaces on i386 and friends are not consistent
> > with the remaining archs. They have `volatile void*` for the
> > arguments and then do a gratuitous cast to `int*`. As far as I can
> > see just using `volatile int*` as for the other archs works fine.
>
> Indeed, this is purely an oversight. The motivation was probably
> originally being able to operate on mismatching types (valid since the
> actual access happens via asm) but this isn't used anyway. I'll fix it.xs
great
> > - pthread_once_t should always be volatile
> > - pthread_spinlock_t should always be volatile
>
> These are C++ ABI changes. I'd like to make the change but I'm
> concerned it might break things.
Both are broken as they are now, if you fall into a compiler that
"knows" that the object itself isn't volatile qualified, and by that
excuse takes the liberty to optimize out loads. For both types there
is one point where there is a cast to (volatile int*) followed by a
load, that might not do what we want.
(For pthread_once_t, this on line 43 in pthread_once.c)
I think the safest would be to make the data types volatile. If that
is not possible, do the load with some new function "a_load_rel" that
is guaranteed to be volatile, atomic and with memory_order relaxed.
> > - pthread_barrier needs atomic increment
>
> Where?
I think all uses of count in pthread_barrier_wait should be
atomic. The increments in lines 15 and 93 should be atomic_fetch_add.
Probably most archs do a ++ on a volatile as one memory operation, but
nothing enforces that.
> > The fifth troubles me a bit. It concerns __timedwait and
> > __timedwait_cp. These both are mostly used with a first argument addr
> > that is atomic. This makes sense, since addr then is passed to a call
> > to futex, which internally might do some atomic operations. Now there
>
> The volatile here is merely to make it legal to pass pointers to
> volatile objects. The only access happens from kernelspace where the
> volatility of the object in userspace is not visible.
>
> > is one call that doesn't pass something that is otherwise seen as
> > atomic, namely line 14 in pthread_join.c. It reads as
> >
> > while ((tmp = t->tid)) __timedwait_cp(&t->tid, tmp, 0, 0, 0);
> >
> > So is the task id here to be seen as atomic, or not? Will updates to
> > that field that are not atomic (and maybe optimized in some sort) be
> > able to mess up the futex call?
>
> The only write to t->tid is by the kernel; it writes a zero and futex
> wakes when the thread exits. No writes whatsoever happen before the
> thread exits, so I don't see any way this code could give rise to an
> early return for pthread_join (which would be very dangerous).
> However, it seems plausible that the compiler could load t->tid twice,
> see the running thread's tid the first time but zero the second time,
> and thereby call __timedwait_cp with a value of 0, resulting in a wait
> operation that never returns.
>
> I don't know a really good solution to this. The vast majority of uses
> of tid are self->tid, from which perspective tid is not volatile or
> even mutable. We don't want to pessimize them with volatile access.
> Probably the best approach is to split it out into separate tid and
> exit_futex fields.
Yes, this would probably be safer and cleaner.
Jens
--
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536 ::
:: :::::::::::::::::::::: gsm France : +33 651400183 ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
