Re: __synccall: deadlock and reliance on racy /proc/self/task

[Rich Felker <dalias@libc.org>]

On Fri, Feb 08, 2019 at 09:14:48PM +0300, Alexey Izbyshev wrote:
> On 2/7/19 9:36 PM, Rich Felker wrote:
> >>For some reason __synccall accesses the list without a barrier (line
> >>120), though I don't see why one wouldn't be necessary for correct
> >>observability of head->next. However, I'm testing on x86_64, so
> >>acquire/release semantics works without barriers.
> >
> >The formal intent in musl is that all a_* are full seq_cst barriers.
> >On x86[_64] this used to not be the case; we just used a normal store,
> >but that turned out to be broken because in some places (and
> >apparently here in __synccall) there was code that depended on a_store
> >having acquire semantics too. See commit
> >3c43c0761e1725fd5f89a9c028cbf43250abb913 and
> >5a9c8c05a5a0cdced4122589184fd795b761bb4a.
> >
> >If not for this fix, I could see this being related (but again, it
> >should see it after timeout anyway). But since the barrier is there
> >now, it shouldn't happen.
> 
> Thanks for the explanation about a_store(). I didn't know that it
> has seq_cst semantics. However, I was talking about a barrier
> between loads of head and cp->tid/cp->next:
> 
> for (cp = head; cp && cp->tid != tid; cp=cp->next);
> 
> In my understanding, we need consume semantics to observe correct
> values of tid and next after we load head. If we don't take Alpha
> into account,
> it probably works without barriers on most current architectures,
> however, I don't know what policy musl has for such cases.

I don't see how that's the case. The only stores to members of ch are
made before the a_cas_p (which has seq_cst order, but just release
would suffice) storing &ch into head and making it visible.

> >>Of course, the larger problem remains: if we may miss some threads
> >>because of /proc, we may fail to call setuid() syscall in those
> >>threads. And that's indeed easily happens in my second test
> >>(attached: test-setuid-mismatch.c; expected to be run as a suid
> >>binary; note that I tested both with and without "presignalling").
> >
> >Does it work if we force two iterations of the readdir loop with no
> >tasks missed, rather than just one, to catch the case of missed
> >concurrent additions? I'm not sure. But all this makes me really
> >uncomfortable with the current approach.
> 
> I've tested with 0, 1, 2 and 3 retries of the main loop if miss_cnt
> == 0. The test eventually failed in all cases, with 0 retries
> requiring only a handful of iterations, 1 -- on the order of 100, 2
> -- on the order of 10000 and 3 -- on the order of 100000.

Do you have a theory on the mechanism of failure here? I'm guessing
it's something like this: there's a thread that goes unseen in the
first round, and during the second round, it creates a new thread and
exits itself. The exit gets seen (again, it doesn't show up in the
dirents) but the new thread it created still doesn't. Is that right?

In any case, it looks like the whole mechanism we're using is
unreliable, so something needs to be done. My leaning is to go with
the global thread list and atomicity of list-unlock with exit.

Rich