From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by sourceware.org (Postfix) with ESMTPS id 0A91F3857C5F for ; Wed, 24 Mar 2021 13:56:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0A91F3857C5F Received: by mail-qt1-x831.google.com with SMTP id a11so17592803qto.2 for ; Wed, 24 Mar 2021 06:56:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=sHaCMy9j5wf8HarIv8zgtJeiBOyJy9bUFVoOD9sf3x8=; b=Wp/u73mcPpmhknfrqXookFO4PEzQNi/2vtXRVFeBX/VNacpiceBEIckEGSqo+b9udo 9oqFWNubaBAkgP4bJIIXtYrAKXj1UO61EKgE9MzoeYI1ngr6Rg5uuVnZHov79tCJJGeu QEUxZkS/UcYEFpxljfCz4TGASx2124wp7ePXH3IYVYJeksmDClMiOdkanFxflMvzC+FC hxXOT8KU2+IxCK/RJ519wUwsrw/XgVxlLDUIeE2Yk9VhEcWQ6MOfXFne0JrjCumuBhMf vE3+OQPZF/ib212ek8nWKzQBDjIhFDljSSm9J2+8XJO/9NiZATSpQEoMWVJW8P/spz8Y mNVA== X-Gm-Message-State: AOAM5310YSlb6yKLXgWPBK28zBseOSBg/n+jTXQoWLNMITdW40VTPmuB 8dUisb3p4KXIlg+47eUhhuSSSw== X-Google-Smtp-Source: ABdhPJybi38UBVK2r4KhLgaYOKLabqOPDtK3J5xW8/4kMFq/yB2BZsydlhcuRHqYLhKXLcmLp5moAw== X-Received: by 2002:ac8:5c07:: with SMTP id i7mr3117117qti.322.1616594202373; Wed, 24 Mar 2021 06:56:42 -0700 (PDT) Received: from [192.168.1.132] ([177.194.41.149]) by smtp.gmail.com with ESMTPSA id g3sm1458998qth.66.2021.03.24.06.56.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 24 Mar 2021 06:56:42 -0700 (PDT) Subject: Re: [PATCH v3 23/37] nptl: Move part of TCB initialization from libpthread to __tls_init_tp To: libc-alpha@sourceware.org, Florian Weimer References: <1d0192110720b4515f29fc46254ae92e6fe744e7.1615914632.git.fweimer@redhat.com> From: Adhemerval Zanella Message-ID: Date: Wed, 24 Mar 2021 10:56:39 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <1d0192110720b4515f29fc46254ae92e6fe744e7.1615914632.git.fweimer@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Mar 2021 13:56:45 -0000 On 16/03/2021 14:30, Florian Weimer via Libc-alpha wrote: > This initalization should only happen once for the main thread's TCB. > At present, auditors can achieve this by not linking against > libpthread. If libpthread becomes part of libc, doing this > initialization in libc would happen for every audit namespace, > or too late (if it happens from the main libc only). That's why > moving this code into ld.so seems the right thing to do, right after > the TCB initialization. Make sense. > > For !__ASSUME_SET_ROBUST_LIST ports, this also moves the symbol > __set_robust_list_avail into ld.so, as __nptl_set_robust_list_avail. > It also turned into a proper boolean flag. > > Inline the __pthread_initialize_pids function because it seems no > longer useful as a separate function. LGTM, with just a suggestion below. Reviewed-by: Adhemerval Zanella > --- > nptl/Versions | 6 +++++ > nptl/nptl-init.c | 36 ++----------------------- > nptl/pthread-pids.h | 29 -------------------- > nptl/pthreadP.h | 6 +++-- > nptl/pthread_create.c | 4 +-- > nptl/pthread_mutex_init.c | 2 +- > sysdeps/nptl/dl-tls_init_tp.c | 37 ++++++++++++++++++++++++++ > sysdeps/unix/sysv/linux/pthread-pids.h | 29 -------------------- > 8 files changed, 52 insertions(+), 97 deletions(-) > delete mode 100644 nptl/pthread-pids.h > delete mode 100644 sysdeps/unix/sysv/linux/pthread-pids.h > > diff --git a/nptl/Versions b/nptl/Versions > index b619df41fb..c50a5442af 100644 > --- a/nptl/Versions > +++ b/nptl/Versions > @@ -417,3 +417,9 @@ libpthread { > __pthread_initialize_minimal; > } > } > + > +ld { > + GLIBC_PRIVATE { > + __nptl_set_robust_list_avail; > + } > +} > \ No newline at end of file Ok. > diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c > index de64e34783..5913bf7272 100644 > --- a/nptl/nptl-init.c > +++ b/nptl/nptl-init.c > @@ -36,7 +36,6 @@ > #include > #include > #include > -#include > #include > > #ifndef TLS_MULTIPLE_THREADS_IN_TCB > @@ -48,15 +47,6 @@ int *__libc_multiple_threads_ptr attribute_hidden; > size_t __static_tls_size; > size_t __static_tls_align_m1; > > -#ifndef __ASSUME_SET_ROBUST_LIST > -/* Negative if we do not have the system call and we can use it. */ > -int __set_robust_list_avail; > -# define set_robust_list_not_avail() \ > - __set_robust_list_avail = -1 > -#else > -# define set_robust_list_not_avail() do { } while (0) > -#endif > - > /* Version of the library, used in libthread_db to detect mismatches. */ > static const char nptl_version[] __attribute_used__ = VERSION; > > @@ -194,31 +184,9 @@ static bool __nptl_initial_report_events __attribute_used__; > void > __pthread_initialize_minimal_internal (void) > { > - /* Minimal initialization of the thread descriptor. */ > + /* Partial initialization of the TCB already happened in TLS_INIT_TP > + and __tls_init_tp. */ > struct pthread *pd = THREAD_SELF; > - __pthread_initialize_pids (pd); > - THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]); > - THREAD_SETMEM (pd, user_stack, true); > - > - /* Initialize the robust mutex data. */ > - { > -#if __PTHREAD_MUTEX_HAVE_PREV > - pd->robust_prev = &pd->robust_head; > -#endif > - pd->robust_head.list = &pd->robust_head; > - pd->robust_head.futex_offset = (offsetof (pthread_mutex_t, __data.__lock) > - - offsetof (pthread_mutex_t, > - __data.__list.__next)); > - int res = INTERNAL_SYSCALL_CALL (set_robust_list, &pd->robust_head, > - sizeof (struct robust_list_head)); > - if (INTERNAL_SYSCALL_ERROR_P (res)) > - set_robust_list_not_avail (); > - } > - > - /* Set initial thread's stack block from 0 up to __libc_stack_end. > - It will be bigger than it actually is, but for unwind.c/pt-longjmp.c > - purposes this is good enough. */ > - THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end); > > /* Before initializing GL (dl_stack_user), the debugger could not > find us and had to set __nptl_initial_report_events. Propagate Ok. > diff --git a/nptl/pthread-pids.h b/nptl/pthread-pids.h > deleted file mode 100644 > index 1a0e9ade41..0000000000 > --- a/nptl/pthread-pids.h > +++ /dev/null > @@ -1,29 +0,0 @@ > -/* Initialize pid and tid fields of struct pthread. Stub version. > - Copyright (C) 2015-2021 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library; if not, see > - . */ > - > -#include > - > -/* Initialize PD->pid and PD->tid for the initial thread. If there is > - setup required to arrange that __exit_thread causes PD->tid to be > - cleared and futex-woken, then this function should do that as well. */ > -static inline void > -__pthread_initialize_pids (struct pthread *pd) > -{ > -#error "sysdeps pthread-pids.h file required" > - pd->pid = pd->tid = -1; > -} Ok. > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 4b486b3577..68f8ef5aa1 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -224,8 +224,10 @@ extern unsigned int __nptl_nthreads; > libc_hidden_proto (__nptl_nthreads) > > #ifndef __ASSUME_SET_ROBUST_LIST > -/* Negative if we do not have the system call and we can use it. */ > -extern int __set_robust_list_avail attribute_hidden; > +/* True if the set_robust_list system call works. Initialized in > + __tls_init_tp. */ > +extern bool __nptl_set_robust_list_avail; > +rtld_hidden_proto (__nptl_set_robust_list_avail) > #endif > > /* Thread Priority Protection. */ Now that you are touching it, maybe it would be better to move the __ASSUME_SET_ROBUST_LIST handling only to the dl-tls_init_tp.c and always define __nptl_set_robust_list_avail (at least one initialization case)? > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 58e10e7741..51340acc4d 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -289,7 +289,7 @@ START_THREAD_DEFN > __ctype_init (); > > #ifndef __ASSUME_SET_ROBUST_LIST > - if (__set_robust_list_avail >= 0) > + if (__nptl_set_robust_list_avail) > #endif > { > /* This call should never fail because the initial call in init.c > @@ -439,7 +439,7 @@ START_THREAD_DEFN > /* We let the kernel do the notification if it is able to do so. > If we have to do it here there for sure are no PI mutexes involved > since the kernel support for them is even more recent. */ > - if (__set_robust_list_avail < 0 > + if (!__nptl_set_robust_list_avail > && __builtin_expect (robust != (void *) &pd->robust_head, 0)) > { > do Ok. > diff --git a/nptl/pthread_mutex_init.c b/nptl/pthread_mutex_init.c > index 233cebc504..f5c3a4b464 100644 > --- a/nptl/pthread_mutex_init.c > +++ b/nptl/pthread_mutex_init.c > @@ -95,7 +95,7 @@ __pthread_mutex_init (pthread_mutex_t *mutex, > { > #ifndef __ASSUME_SET_ROBUST_LIST > if ((imutexattr->mutexkind & PTHREAD_MUTEXATTR_FLAG_PSHARED) != 0 > - && __set_robust_list_avail < 0) > + && !__nptl_set_robust_list_avail) > return ENOTSUP; > #endif > Ok. > diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c > index 8983808233..c5172b7613 100644 > --- a/sysdeps/nptl/dl-tls_init_tp.c > +++ b/sysdeps/nptl/dl-tls_init_tp.c > @@ -16,10 +16,17 @@ > License along with the GNU C Library; if not, see > . */ > > +#include > #include > #include > +#include > #include > > +#ifndef __ASSUME_SET_ROBUST_LIST > +bool __nptl_set_robust_list_avail; > +rtld_hidden_data_def (__nptl_set_robust_list_avail) > +#endif > + > void > __tls_init_tp (void) > { > @@ -27,4 +34,34 @@ __tls_init_tp (void) > INIT_LIST_HEAD (&GL (dl_stack_used)); > INIT_LIST_HEAD (&GL (dl_stack_user)); > list_add (&THREAD_SELF->list, &GL (dl_stack_user)); > + > + /* Early initialization of the TCB. */ > + struct pthread *pd = THREAD_SELF; > + pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->tid); > + THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]); > + THREAD_SETMEM (pd, user_stack, true); > + > + /* Initialize the robust mutex data. */ > + { > +#if __PTHREAD_MUTEX_HAVE_PREV > + pd->robust_prev = &pd->robust_head; > +#endif > + pd->robust_head.list = &pd->robust_head; > + pd->robust_head.futex_offset = (offsetof (pthread_mutex_t, __data.__lock) > + - offsetof (pthread_mutex_t, > + __data.__list.__next)); > + int res = INTERNAL_SYSCALL_CALL (set_robust_list, &pd->robust_head, > + sizeof (struct robust_list_head)); > + if (!INTERNAL_SYSCALL_ERROR_P (res)) > + { > +#ifndef __ASSUME_SET_ROBUST_LIST > + __nptl_set_robust_list_avail = true; > +#endif > + } > + } > + > + /* Set initial thread's stack block from 0 up to __libc_stack_end. > + It will be bigger than it actually is, but for unwind.c/pt-longjmp.c > + purposes this is good enough. */ > + THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end); > } Ok. > diff --git a/sysdeps/unix/sysv/linux/pthread-pids.h b/sysdeps/unix/sysv/linux/pthread-pids.h > deleted file mode 100644 > index 10b58899f5..0000000000 > --- a/sysdeps/unix/sysv/linux/pthread-pids.h > +++ /dev/null > @@ -1,29 +0,0 @@ > -/* Initialize pid and tid fields of struct pthread. Linux version. > - Copyright (C) 2015-2021 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library; if not, see > - . */ > - > -#include > -#include > - > -/* Initialize PD->pid and PD->tid for the initial thread. If there is > - setup required to arrange that __exit_thread causes PD->tid to be > - cleared and futex-woken, then this function should do that as well. */ > -static inline void > -__pthread_initialize_pids (struct pthread *pd) > -{ > - pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->tid); > -} > Ok.