x86_64: Reset TID in TCB on clone that does not use CLONE_VM

  This eliminates a TCB layout dependency from the Chromium sandbox.
(Chromium checks the TID in a private field in a recursive mutex
and only tries to patch the TCB if the TID found there is different
from the value it obtains via gettid.)

Tested on x86_64-linux-gnu.  Confirmed to un-break Google Chrome.

---
 sysdeps/unix/sysv/linux/x86_64/clone.S | 12 ++++++++++++
 1 file changed, 12 insertions(+)


base-commit: cc74583f23657515b1d09d0765032422af71de52
  

On 03/01/25 12:54, Florian Weimer wrote:
> This eliminates a TCB layout dependency from the Chromium sandbox.
> (Chromium checks the TID in a private field in a recursive mutex
> and only tries to patch the TCB if the TID found there is different
> from the value it obtains via gettid.)
> 
> Tested on x86_64-linux-gnu.  Confirmed to un-break Google Chrome.
> 
> ---
>  sysdeps/unix/sysv/linux/x86_64/clone.S | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S
> index f3985e7f71..67a30405a2 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/clone.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S
> @@ -23,6 +23,9 @@
>  #include <bits/errno.h>
>  #include <asm-syntax.h>
>  
> +/* Neither <sched.h> nor <linux/sched.h> are usable in assembler files.  */
> +#define CLONE_VM 0x00000100

You can the constant with a *.sym file, like tcb-offsets.sym for some ABIs.

> +
>  /* The userland implementation is:
>     int clone (int (*fn)(void *arg), void *child_stack, int flags,
>  	      void *arg, pid_t *parent_tid, void *tls, pid_t *child_tid);
> @@ -94,6 +97,15 @@ L(thread_start):
>  	   the outermost frame obviously.  */
>  	xorl	%ebp, %ebp
>  
> +	/* If a fork-style clone is performed, reset the TID in the TCB.
> +	   Without CLONE_VM, the TID is not shared and safe to re-use.  */
> +	test	$CLONE_VM, %edi
> +	jnz	1f
> +	movl	$SYS_ify(gettid), %eax
> +	syscall
> +	movl	%eax, %fs:TID
> +1:
> +

Why Chrome check the TID field after call clone? I had the impression that
once a process calls clone() directly is undefined behavior whether calls
to pthread should succeed. Is the TID reset suffice to make pthread consistent
is this case, or is just to make recursive mutex to work?

In any case, we should make this change on all ABIs and not only on x86.
And I think we should proper document this with a bugzilla.

>  	/* Set up arguments for the function call.  */
>  	popq	%rax		/* Function to call.  */
>  	popq	%rdi		/* Argument.  */
> 
> base-commit: cc74583f23657515b1d09d0765032422af71de52
>
  

* Adhemerval Zanella Netto:

>> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S
>> index f3985e7f71..67a30405a2 100644
>> --- a/sysdeps/unix/sysv/linux/x86_64/clone.S
>> +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S
>> @@ -23,6 +23,9 @@
>>  #include <bits/errno.h>
>>  #include <asm-syntax.h>
>>  
>> +/* Neither <sched.h> nor <linux/sched.h> are usable in assembler files.  */
>> +#define CLONE_VM 0x00000100
>
> You can the constant with a *.sym file, like tcb-offsets.sym for some
> ABIs.

Makes sense.

>>  /* The userland implementation is:
>>     int clone (int (*fn)(void *arg), void *child_stack, int flags,
>>  	      void *arg, pid_t *parent_tid, void *tls, pid_t *child_tid);
>> @@ -94,6 +97,15 @@ L(thread_start):
>>  	   the outermost frame obviously.  */
>>  	xorl	%ebp, %ebp
>>  
>> +	/* If a fork-style clone is performed, reset the TID in the TCB.
>> +	   Without CLONE_VM, the TID is not shared and safe to re-use.  */
>> +	test	$CLONE_VM, %edi
>> +	jnz	1f
>> +	movl	$SYS_ify(gettid), %eax
>> +	syscall
>> +	movl	%eax, %fs:TID
>> +1:
>> +
>
> Why Chrome check the TID field after call clone? I had the impression that
> once a process calls clone() directly is undefined behavior whether calls
> to pthread should succeed. Is the TID reset suffice to make pthread consistent
> is this case, or is just to make recursive mutex to work?

Chrome needs a ForkWithFlags implementation.  They currently call our
clone (without CLONE_VM and CLONE_VFORK) and longjmp out of the
callback.  This results in a TCB that doesn't have the right TID, and as
a result, recursive mutexes may not work correctly due to TID reuse
(I assume that's the bug they are trying to fix by patching the TID).

> In any case, we should make this change on all ABIs and not only on x86.
> And I think we should proper document this with a bugzilla.

I would rather add a clone_fork implementation which requires CLONE_VM
or CLONE_VFORK for proper operation.  To fix the other architectures, we
can add back the old 24-pointer padding.  Or add the TID reset for the
architectures that have a Chromium port.

I tried to reach out to the Chromium developers.  I think I'm subscribed
to the list, but I don't see the posting in the archives yet:

From: Florian Weimer <fweimer@redhat.com>
Subject: Reducing dependencies on glibc internals in
 sandbox/linux/services/namespace_sandbox.cc, ForkWithFlags
To: chromium-dev@chromium.org
Date: Fri, 03 Jan 2025 17:04:14 +0100
Message-ID: <87bjwnlw8x.fsf@oldenburg.str.redhat.com>

Thanks,
Florian