[musl] Incorrect thread TID caching

[musl] Incorrect thread TID caching

[Dominic Chen]

I've been debugging a local port of Chrome using musl, and have noticed 
that musl is caching the thread TID in __pthread_self()->tid, which 
results in incorrect behavior if the application calls the clone() libc 
wrapper or the clone system call, and then calls libc functions which 
use the cached TID value, like raise().

 From a quick skim of other libc implementations, both bionic and glibc 
don't seem to cache TID, and directly call the gettid system call inside 
raise(). I also recall that glibc removed PID caching a few years ago 
due to similar issues there as well. So, it seems that musl should 
either not cache the TID, or at least update the cached value after 
returning from the system call inside the clone() wrapper (with special 
handling for CLONE_VM/CLONE_VFORK)?

Please CC me on replies.

Thanks,

Dominic

Re: [musl] Incorrect thread TID caching

[Florian Weimer]

* Dominic Chen:

> From a quick skim of other libc implementations, both bionic and glibc
> don't seem to cache TID, and directly call the gettid system call
> inside raise().

Of course glibc has a TID cache.  It doesn't use it in raise (mostly to
get abort working after vfork), but it's definitely needed in other
places.

If you use the clone system call wrapper in threading (not fork/vfork)
mode, you cannot call any libc functions afterwards, including the
syscall function.  Instead, you have to issue direct system calls.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Tue, Feb 02, 2021 at 11:04:23PM -0500, Dominic Chen wrote:
> I've been debugging a local port of Chrome using musl, and have
> noticed that musl is caching the thread TID in
> __pthread_self()->tid, which results in incorrect behavior if the
> application calls the clone() libc wrapper or the clone system call,
> and then calls libc functions which use the cached TID value, like
> raise().

Unfortunately it's really underdocumented and underexplored what a
child created with clone() can do. There are definitely limitations --
for example any usage with CLONE_VM or CLONE_THREAD is restricted not
to call into libc at all, and might not even be safe whatsoever.
However basic usage comparable in semantics to _Fork is probably
supposed to work at least as well as _Fork -- in particular calling
AS-safe libc functions should work.

BTW does Chrom{e,ium} itself do something with raw clone? If so this
could be a source of some of the bugs users hit, and it would be great
to get a clearer picture on what's happening.

> From a quick skim of other libc implementations, both bionic and
> glibc don't seem to cache TID, and directly call the gettid system
> call inside raise(). I also recall that glibc removed PID caching a
> few years ago due to similar issues there as well. So, it seems that
> musl should either not cache the TID, or at least update the cached
> value after returning from the system call inside the clone()
> wrapper (with special handling for CLONE_VM/CLONE_VFORK)?

I think the clone() function should be updated to provide whatever
contract we expect it to have in the cases where it's valid to use,
and this should include the same logic as in _Fork. I'm not sure what
we should have it do for unsafe/invalid usage.

> Please CC me on replies.

OK.

Rich

Re: [musl] Incorrect thread TID caching

[Dominic Chen]

On 2/3/2021 2:16 AM, Florian Weimer wrote:
> If you use the clone system call wrapper in threading (not fork/vfork)
> mode, you cannot call any libc functions afterwards, including the
> syscall function.  Instead, you have to issue direct system calls.
On 2/3/2021 2:21 PM, Rich Felker wrote:
> Unfortunately it's really underdocumented and underexplored what a
> child created with clone() can do. There are definitely limitations --
> for example any usage with CLONE_VM or CLONE_THREAD is restricted not
> to call into libc at all, and might not even be safe whatsoever.
> However basic usage comparable in semantics to _Fork is probably
> supposed to work at least as well as _Fork -- in particular calling
> AS-safe libc functions should work.

I wasn't aware of this behavior, and didn't see any documentation about 
this for the glibc clone() wrapper either. This seems to be a big 
footgun, and after looking through the history for this code in Chrome, 
it looks like they had a similar issue with glibc too.

> BTW does Chrom{e,ium} itself do something with raw clone? If so this
> could be a source of some of the bugs users hit, and it would be great
> to get a clearer picture on what's happening.

The code in question is a unittest for the sandbox, which manually calls 
clone with CLONE_NEWPID to fork a child in a PID namespace, then 
installs a signal handler and checks that it receives SIGTERM correctly: 
https://source.chromium.org/chromium/chromium/src/+/master:sandbox/linux/services/namespace_sandbox_unittest.cc;l=194 
. But under musl, raise() uses the cached TID value, so the test 
eventually times out.

I missed that the NamespaceSandbox::ForkInNewPidNamespace() function 
does manually update the cached TID for glibc after calling the 
ForkWithFlags wrapper, so I can just do the same for musl too.

Separately, it looks like glibc used to have a PID cache too, but was 
removed after a discussion that you were both involved with: 
http://sourceware-org.1504.n7.nabble.com/Caching-of-PID-TID-after-fork-td416394.html

Thanks,

Dominic

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Wed, Feb 03, 2021 at 03:21:06PM -0500, Dominic Chen wrote:
> 
> On 2/3/2021 2:16 AM, Florian Weimer wrote:
> >If you use the clone system call wrapper in threading (not fork/vfork)
> >mode, you cannot call any libc functions afterwards, including the
> >syscall function.  Instead, you have to issue direct system calls.
> On 2/3/2021 2:21 PM, Rich Felker wrote:
> >Unfortunately it's really underdocumented and underexplored what a
> >child created with clone() can do. There are definitely limitations --
> >for example any usage with CLONE_VM or CLONE_THREAD is restricted not
> >to call into libc at all, and might not even be safe whatsoever.
> >However basic usage comparable in semantics to _Fork is probably
> >supposed to work at least as well as _Fork -- in particular calling
> >AS-safe libc functions should work.
> 
> I wasn't aware of this behavior, and didn't see any documentation
> about this for the glibc clone() wrapper either. This seems to be a
> big footgun, and after looking through the history for this code in
> Chrome, it looks like they had a similar issue with glibc too.

Yes that's what I mean by underdocumented.

> >BTW does Chrom{e,ium} itself do something with raw clone? If so this
> >could be a source of some of the bugs users hit, and it would be great
> >to get a clearer picture on what's happening.
> 
> The code in question is a unittest for the sandbox, which manually
> calls clone with CLONE_NEWPID to fork a child in a PID namespace,
> then installs a signal handler and checks that it receives SIGTERM
> correctly: https://source.chromium.org/chromium/chromium/src/+/master:sandbox/linux/services/namespace_sandbox_unittest.cc;l=194
> .. But under musl, raise() uses the cached TID value, so the test
> eventually times out.

OK, raise should probably just be changed here to work even in vforked
child since it seems plausible someone will use it there. It's not
like saving the syscall actually matters here. But that's independent
of the clone() issue.

> I missed that the NamespaceSandbox::ForkInNewPidNamespace() function
> does manually update the cached TID for glibc after calling the
> ForkWithFlags wrapper, so I can just do the same for musl too.

This isn't valid; the location is not ABI. You could very well end up
clobbering a pointer or something unrelated. The issue should just be
fixed on the musl side.

Rich

Re: [musl] Incorrect thread TID caching

[Dominic Chen]

On 2/3/2021 4:01 PM, Rich Felker wrote:
> OK, raise should probably just be changed here to work even in vforked
> child since it seems plausible someone will use it there. It's not
> like saving the syscall actually matters here. But that's independent
> of the clone() issue.

Sounds good, thanks!

Dominic

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Wed, Feb 03, 2021 at 05:30:01PM -0500, Dominic Chen wrote:
> 
> On 2/3/2021 4:01 PM, Rich Felker wrote:
> >OK, raise should probably just be changed here to work even in vforked
> >child since it seems plausible someone will use it there. It's not
> >like saving the syscall actually matters here. But that's independent
> >of the clone() issue.
> 
> Sounds good, thanks!

Hm, looking at how to do this now, and if clone is going to behave
like _Fork, it needs to be able to run code in the child (to restore
the signals is masked, etc.), which means it needs to wrap the child
function passed to it. I think this is doable, but it's not entirely
trivial.

Rich

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Wed, Feb 03, 2021 at 05:55:18PM -0500, Rich Felker wrote:
> On Wed, Feb 03, 2021 at 05:30:01PM -0500, Dominic Chen wrote:
> > 
> > On 2/3/2021 4:01 PM, Rich Felker wrote:
> > >OK, raise should probably just be changed here to work even in vforked
> > >child since it seems plausible someone will use it there. It's not
> > >like saving the syscall actually matters here. But that's independent
> > >of the clone() issue.
> > 
> > Sounds good, thanks!
> 
> Hm, looking at how to do this now, and if clone is going to behave
> like _Fork, it needs to be able to run code in the child (to restore
> the signals is masked, etc.), which means it needs to wrap the child
> function passed to it. I think this is doable, but it's not entirely
> trivial.

Following up on this now, the code in _Fork is something I really
don't want to duplicate for clone() for risk of forgetting there's a
copy in the latter and letting it bitrot there. I'd rather refactor
things so the same logic can be shared.

In theory we don't need __clone and the above-mentioned callback
wrapper machinery for the non-CLONE_VM case (the only one that's safe
anyway) and can just use __syscall(SYS_clone, ...) from C, and then
the code from _Fork.c can just be generalized to take a function
pointer and arg pointer to call back to instead of directly making the
SYS_fork syscall. However, the fact that SYS_clone has arch-dependent
argument order makes this painful. The extended arguments whose order
vary are *mostly* invalid (you can't set a custom TLS pointer or exit
futex address, at least) so perhaps we could just EINVAL them in
clone().

The other option looks like it's to split the pre/post logic for _Fork
out into separate functions; then clone can call the pre logic in the
parent and the child function passed to __clone can call the post
logic for the child. This is uglier and more costly to _Fork, and I'd
like to avoid it, but not as much as I'd like to avoid duplicating
logic for per-arch argument order. Hopefully we can avoid both..

Rich

Re: [musl] Incorrect thread TID caching

[Dominic Chen]

On 2/15/2021 11:56 AM, Rich Felker wrote:
> Following up on this now, the code in _Fork is something I really
> don't want to duplicate for clone() for risk of forgetting there's a
> copy in the latter and letting it bitrot there. I'd rather refactor
> things so the same logic can be shared...

Thanks for the update. Can you use something like 
__attribute__((always_inline)) to just write the logic once but force it 
to be inlined into both library functions?

Dominic

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Wed, Feb 17, 2021 at 02:49:45PM -0500, Dominic Chen wrote:
> On 2/15/2021 11:56 AM, Rich Felker wrote:
> >Following up on this now, the code in _Fork is something I really
> >don't want to duplicate for clone() for risk of forgetting there's a
> >copy in the latter and letting it bitrot there. I'd rather refactor
> >things so the same logic can be shared...
> 
> Thanks for the update. Can you use something like
> __attribute__((always_inline)) to just write the logic once but
> force it to be inlined into both library functions?

Whether it's inlined isn't really a big deal; this is not a hot path.
It's more just a matter of how it needs to be split up at the source
level, and it seems to be messy whichever way we choose.

Trying to avoid calling __clone doesn't seem like such a good idea,
since the child has to run on a new stack -- if we did avoid it we'd
need a new way to switch stacks. The generic __unmapself has a hack
to do this already that we could reuse without needing new
arch-specific glue though.

I'll keep trying things and see if I come up with something not too
unreasonable.

Rich

Re: [musl] Incorrect thread TID caching

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

On Wed, Feb 17, 2021 at 03:11:57PM -0500, Rich Felker wrote:
> On Wed, Feb 17, 2021 at 02:49:45PM -0500, Dominic Chen wrote:
> > On 2/15/2021 11:56 AM, Rich Felker wrote:
> > >Following up on this now, the code in _Fork is something I really
> > >don't want to duplicate for clone() for risk of forgetting there's a
> > >copy in the latter and letting it bitrot there. I'd rather refactor
> > >things so the same logic can be shared...
> > 
> > Thanks for the update. Can you use something like
> > __attribute__((always_inline)) to just write the logic once but
> > force it to be inlined into both library functions?
> 
> Whether it's inlined isn't really a big deal; this is not a hot path.
> It's more just a matter of how it needs to be split up at the source
> level, and it seems to be messy whichever way we choose.
> 
> Trying to avoid calling __clone doesn't seem like such a good idea,
> since the child has to run on a new stack -- if we did avoid it we'd
> need a new way to switch stacks. The generic __unmapself has a hack
> to do this already that we could reuse without needing new
> arch-specific glue though.
> 
> I'll keep trying things and see if I come up with something not too
> unreasonable.

Attached is a draft of how clone() *could* work without refactoring
the pre/post logic from _Fork to use __clone. I don't particularly
like it, and CRTJMP abuse like this (which __unmapself also does, as
noted above) is not valid for FDPIC archs (it actually expects a code
address not a function pointer). I'll try a version the other way and
see how it looks.

Rich

[-- Attachment #2: clone_forklike.diff --]
[-- Type: text/plain, Size: 1357 bytes --]

diff --git a/src/linux/clone.c b/src/linux/clone.c
index 8c1af7d3..32d53a8f 100644
--- a/src/linux/clone.c
+++ b/src/linux/clone.c
@@ -4,6 +4,25 @@
 #include <sched.h>
 #include "pthread_impl.h"
 #include "syscall.h"
+#include "dynlink.h"
+
+extern int _Forklike(int (*)(void *), void *);
+
+struct clone_args {
+	int flags;
+	pid_t *ptid, *ctid;
+};
+
+static int do_clone(void *p)
+{
+	struct clone_args *args = p;
+	int mask = CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID;
+	int r = __syscall(SYS_clone, args->flags & ~mask, 0);
+	if (r>0 && (args->flags & CLONE_PARENT_SETTID)) *args->ptid = r;
+	if (!r && (args->flags & CLONE_CHILD_SETTID)) *args->ctid = __syscall(SYS_gettid);
+	if (!r && (args->flags & CLONE_CHILD_CLEARTID)) __syscall(SYS_set_tid_address, args->ctid);
+	return r;
+}
 
 int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
 {
@@ -17,5 +36,13 @@ int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
 	ctid = va_arg(ap, pid_t *);
 	va_end(ap);
 
+	if (!(flags & CLONE_VM)) {
+		struct clone_args args = { .flags = flags, .ptid = ptid, .ctid = ctid };
+		if (flags & CLONE_THREAD) return __syscall_ret(-EINVAL);
+		int r = _Forklike(do_clone, &args);
+		if (r) return r;
+		CRTJMP(func, stack);
+	}
+
 	return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
 }

Re: [musl] Incorrect thread TID caching

[Dominic Chen]

On 2/17/2021 4:07 PM, Rich Felker wrote:
>> Whether it's inlined isn't really a big deal; this is not a hot path.
>> It's more just a matter of how it needs to be split up at the source
>> level, and it seems to be messy whichever way we choose.
>>
>> Trying to avoid calling __clone doesn't seem like such a good idea,
>> since the child has to run on a new stack -- if we did avoid it we'd
>> need a new way to switch stacks. The generic __unmapself has a hack
>> to do this already that we could reuse without needing new
>> arch-specific glue though.
>>
>> I'll keep trying things and see if I come up with something not too
>> unreasonable.
>>
>> Attached is a draft of how clone() *could* work without refactoring
>> the pre/post logic from _Fork to use __clone. I don't particularly
>> like it, and CRTJMP abuse like this (which __unmapself also does, as
>> noted above) is not valid for FDPIC archs (it actually expects a code
>> address not a function pointer). I'll try a version the other way and
>> see how it looks.

Sorry for the delay. I did end up needing robust mutex functionality, 
and ended up using a variant of your patch to fix-up. There were two 
issues that I noticed:

 1. Prior to CRTJMP, the 'arg' argument needs to be moved into the first
    argument register for the local calling convention. I spent a little
    time trying to get an architecture-neutral version working using a
    wrapper function, but gave up and used some inline assembly to
    populate %rdi on x86_64.
 2. If a thread exits without unlocking a robust mutex, the program will
    subsequently SIGSEGV since the thread's memory region has been
    unmapped, but a pointer to the robust mutex remains on the internal
    linked list. Not sure where the fault lies here, but just thought
    I'd mention it.

Thanks,

Dominic

Re: [musl] Incorrect thread TID caching

[Carlos O'Donell]

On 2/3/21 3:21 PM, Dominic Chen wrote:
> The code in question is a unittest for the sandbox, which manually
> calls clone with CLONE_NEWPID to fork a child in a PID namespace,
> then installs a signal handler and checks that it receives SIGTERM
> correctly:
> https://source.chromium.org/chromium/chromium/src/+/master:sandbox/linux/services/namespace_sandbox_unittest.cc;l=194
> . But under musl, raise() uses the cached TID value, so the test
> eventually times out.
> 
> I missed that the NamespaceSandbox::ForkInNewPidNamespace() function
> does manually update the cached TID for glibc after calling the
> ForkWithFlags wrapper, so I can just do the same for musl too.

As Rich said for musl, this is not ABI for glibc either.

I'm not sure why you need to reset the TID cache. In glibc we have
containerized tests and we do not need to change internals of the 
runtime e.g. unshare (CLONE_NEWUSER, CLONE_NEWPID, CLONE_NEWNS);
(see glibc/support/test-container.c).

-- 
Cheers,
Carlos.

Re: [musl] Incorrect thread TID caching

[Dominic Chen]

> On Feb 3, 2021, at 10:28 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> As Rich said for musl, this is not ABI for glibc either.
> 
> I'm not sure why you need to reset the TID cache. In glibc we have
> containerized tests and we do not need to change internals of the 
> runtime e.g. unshare (CLONE_NEWUSER, CLONE_NEWPID, CLONE_NEWNS);
> (see glibc/support/test-container.c).


Thanks for the advice; sounds like I should just modify my local musl to call gettid directly in raise().

As for Chrome, I’m not very familiar with the codebase, but it looks like that code landed back in 2016, as a workaround before the PID/TID cache changes in glibc.

Dominic

Re: [musl] Incorrect thread TID caching

[Rich Felker]

On Wed, Feb 03, 2021 at 11:22:18PM -0500, Dominic Chen wrote:
> 
> > On Feb 3, 2021, at 10:28 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> > 
> > As Rich said for musl, this is not ABI for glibc either.
> > 
> > I'm not sure why you need to reset the TID cache. In glibc we have
> > containerized tests and we do not need to change internals of the 
> > runtime e.g. unshare (CLONE_NEWUSER, CLONE_NEWPID, CLONE_NEWNS);
> > (see glibc/support/test-container.c).
> 
> 
> Thanks for the advice; sounds like I should just modify my local
> musl to call gettid directly in raise().

No, as noted earlier in this thread we should fix clone() to be like
_Fork() at least in the case where it's not doing things that are
inherently unsafe (CLONE_VM etc.) -- as long as you're making a new
process with normal process semantics, the child should be able to
safely call any AS-safe function in libc, or any function in libc at
all if the parent is not multi-threaded.

The possibility of changing raise() is just for the sake of vfork()
and is independent of the issue here.

Rich