[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 11 Dec 2018 19:32:38 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: sem_wait and EINTR
On Sun, Dec 09, 2018 at 07:50:33AM +0100, Markus Wichmann wrote:
> On Sat, Dec 08, 2018 at 09:51:40PM -0500, Rich Felker wrote:
> > Conceptually the logic looks correct, but I'm trying to phase out the
> > __libc structure,
>
> Oh? Well, I suppose it does offer no benefit the linker doesn't already
> give you. I suppose we could make the flag a static int in sigaction(),
> with a hidden getter function.
For the record, the original purpose of the __libc structure was, in
the days of the first-gen dynamic linker, making it possible to access
the members in a pure PC-relative manner before relocations had taken
place, and without assuming the toolchain supported visibility. In the
fallback case, libc was #defined like errno, as (*__libc_location())
or similar, and the struct was static in the file where that function
was defined. It later got used for other globals we wanted to make
cheap to access, and, to some extent, to micro-optimize the placement
of atomics relative to cache lines in ways that didn't actually
matter.
Now that we just use hidden visibility for internal things, access is
just as efficient without putting them in the special hidden-vis libc
structure, and static linking is more efficient since the linker can
avoid pulling in members that don't actually get used.
> > and more importantly, introducing a bitfield here is
> > not safe unless access to all bitfield members is controlled by a
> > common mutex or other synchronization mechanism. There is a data race
> > if any access to the other fields is accessed when sigaction is
> > callable from another thread, and the impact of a data race on any of
> > these is critical.
> >
>
> The flags can_do_thrads, threaded, and secure are all set before the
> program becomes multi-threaded and they keep their values throughout the
> rest of the program. So the only accesses are read accesses.
>
> handling_sigs on the other hand might be written by multiple threads at
> the same time, but to the same value. Can that ever be an issue?
>
> Wait... I think I figured it out. On some architectures you can have
> incoherent caches. Threads need to synchronize to access shared
> ressources, and those synchronization functions take care of that
> problem, but I circumvented them. Now, if the caches are set to
> write-back mode (which is common), then writing to handling_sigs only
> writes to the cache line for the address of handling_sigs, but not to
> main memory. So another thread on another core will not see the update.
>
> Worse: If by a bad roll of the dice the writing thread's cache line is
> evicted before the reading thread's cache line is (it might have written
> somewhere else in that cache line), that latter eviction will overwrite
> handling_sigs back to 0.
>
> Am I close?
There might or might not be mechanisms on very weakly-ordered machines
by which this could blow up, but the reasoning here is more simple and
high-level: we just don't assume properties about the machine's memory
ordering properties, and instead treat data races as the undefined
behavior they are, with the assumption that "anything could happen".
> > Unfortunately, even without the bitfield, there is a data race between
> > access to the new flag from sem_timedwait and from sigaction. In
> > theory, this potentially results in a race window where sem_wait
> > retries on EINTR because it doesn't yet see the updated handling_sigs.
>
> In that case, the sem_wait() and the sigaction() would be unserialized,
> and any correct program cannot depend on the order of operations. In
> particular, the caller if the sem_wait() can't depend on the sigaction()
> having installed the signal handler yet.
Consider the case where thread A is blocked in sem_wait, and thread B
installs an interrupting signal handler then calls pthread_kill to
send the signal to thread A. Despite any operations that synchronize
memory, since the pthread_kill is ordered after the sigaction in
thread B, it seems a requirement that the signal generate EINTR. There
necessarily is some underlying synchronization in kernelspace that
would ensure this even without any effort by userspace, but it's not
part of the abstract model I'm working from. We could just assume it's
there or add a_barrier() before the check and make it explicit.
> > The "right" fix would be to use an AS-safe lock to protect the
> > handling_sigs object, or make it atomic (a_store on the sigaction
> > side, a_barrier before load on the sem side). Alternatively we could
> > assume the above reasoning about sequencing of sigaction before EINTR
> > and just use a lock on the sigaction side, but the atomic is probably
> > simplest and "safer".
> >
>
> Plus, a lock for a single int that can only ever go from 0 to 1 would be
> a bit overkill.
It's not a big deal, but I think a_store would work just as well and
be simpler.
> > A couple other small nits: comments especially should not exceed 80
> > columns (preferably <75 or so) assuming tab width 8; code should avoid
> > it too but I see it slightly does in a few places there. Spaces should
> > not be used for indention.
>
> Ah crap. I forgot the comment the first time I was in the file, and when
> I started vim again, I forgot to re-apply the musl settings.
>
> > And the commit message should reflect the
> > change made; it's not adding a workaround, but reducing the scope of a
> > previous workaround (which should be cited by commit id) to fix a
> > conformance bug. The name "handling_sigs" is also a bit misleading;
> > what it's indicating is that interrupting signal handlers are or have
> > been present.
> >
> > I'm happy to fix up these issues and commit if there aren't mistakes
> > in the above comments that still need to be addressed.
>
> Well, it certainly was a learning experience for me. Just goes to show
> that you never learn as much as when you're making a mistake and have to
> figure it out.
One other thought: would it be preferable for the EINTR suppression in
the absence of interruptible signal handlers to be in __timedwait
rather than sem_timedwait? Then any code using __timedwait would
benefit from it. I'm not sure if there are other callers where it
would help but it wouldn't hurt either.
Rich
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.
