From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: libc-alpha@sourceware.org, Florian Weimer <fweimer@redhat.com>
Subject: Re: [PATCH 03/18] nptl: Move legacy unwinding implementation into libc
Date: Mon, 15 Mar 2021 17:02:03 -0300 [thread overview]
Message-ID: <187e63bc-2f7f-caf2-1b9e-219b6a0bcac1@linaro.org> (raw)
In-Reply-To: <4485b54412a6fb871b58c694bf8b0efd098f1111.1615569355.git.fweimer@redhat.com>
On 12/03/2021 14:49, Florian Weimer via Libc-alpha wrote:
> It is still used internally. Since unwinding is now available
> unconditionally, avoid indirect calls through function pointers loaded
> from the stack by inlining the non-cancellation cleanup code. This
> avoids a regression in security hardening.
>
> The out-of-line __libc_cleanup_routine implementation is no longer
> needed because the inline definition is now static __always_inline.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> nptl/Versions | 2 +
> nptl/cleanup_defer_compat.c | 56 ++--------------------------
> nptl/libc-cleanup.c | 64 ++++++++++++++++++++++++++++++--
> nptl/nptl-init.c | 2 -
> sysdeps/nptl/libc-lock.h | 59 ++++++++++++++---------------
> sysdeps/nptl/libc-lockP.h | 26 +------------
> sysdeps/nptl/pthread-functions.h | 4 --
> 7 files changed, 97 insertions(+), 116 deletions(-)
>
> diff --git a/nptl/Versions b/nptl/Versions
> index f2db649f9d..e3eb686a04 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -86,6 +86,8 @@ libc {
> __futex_abstimed_wait_cancelable64;
> __libc_alloca_cutoff;
> __libc_allocate_rtsig_private;
> + __libc_cleanup_pop_restore;
> + __libc_cleanup_push_defer;
> __libc_current_sigrtmax_private;
> __libc_current_sigrtmin_private;
> __libc_dl_error_tsd;
Ok.
> diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
> index 49ef53ea60..1957318208 100644
> --- a/nptl/cleanup_defer_compat.c
> +++ b/nptl/cleanup_defer_compat.c
> @@ -17,41 +17,15 @@
> <https://www.gnu.org/licenses/>. */
>
> #include "pthreadP.h"
> -
> +#include <libc-lock.h>
>
> void
> _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
> void (*routine) (void *), void *arg)
> {
> - struct pthread *self = THREAD_SELF;
> -
> buffer->__routine = routine;
> buffer->__arg = arg;
> - buffer->__prev = THREAD_GETMEM (self, cleanup);
> -
> - int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> -
> - /* Disable asynchronous cancellation for now. */
> - if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> - while (1)
> - {
> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> - cancelhandling
> - & ~CANCELTYPE_BITMASK,
> - cancelhandling);
> - if (__glibc_likely (curval == cancelhandling))
> - /* Successfully replaced the value. */
> - break;
> -
> - /* Prepare for the next round. */
> - cancelhandling = curval;
> - }
> -
> - buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> - ? PTHREAD_CANCEL_ASYNCHRONOUS
> - : PTHREAD_CANCEL_DEFERRED);
> -
> - THREAD_SETMEM (self, cleanup, buffer);
> + __libc_cleanup_push_defer (buffer);
> }
> strong_alias (_pthread_cleanup_push_defer, __pthread_cleanup_push_defer)
>
Ok.
> @@ -60,31 +34,7 @@ void
> _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
> int execute)
> {
> - struct pthread *self = THREAD_SELF;
> -
> - THREAD_SETMEM (self, cleanup, buffer->__prev);
> -
> - int cancelhandling;
> - if (__builtin_expect (buffer->__canceltype != PTHREAD_CANCEL_DEFERRED, 0)
> - && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
> - & CANCELTYPE_BITMASK) == 0)
> - {
> - while (1)
> - {
> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> - cancelhandling
> - | CANCELTYPE_BITMASK,
> - cancelhandling);
> - if (__glibc_likely (curval == cancelhandling))
> - /* Successfully replaced the value. */
> - break;
> -
> - /* Prepare for the next round. */
> - cancelhandling = curval;
> - }
> -
> - CANCELLATION_P (self);
> - }
> + __libc_cleanup_pop_restore (buffer);
>
> /* If necessary call the cleanup routine after we removed the
> current cleanup block from the list. */
Ok.
> diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
> index 61f97eceda..14ccfe9285 100644
> --- a/nptl/libc-cleanup.c
> +++ b/nptl/libc-cleanup.c
> @@ -17,11 +17,69 @@
> <https://www.gnu.org/licenses/>. */
>
> #include "pthreadP.h"
> +#include <tls.h>
> +#include <libc-lock.h>
>
> +void
> +__libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer)
> +{
> + struct pthread *self = THREAD_SELF;
> +
> + buffer->__prev = THREAD_GETMEM (self, cleanup);
> +
> + int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> +
> + /* Disable asynchronous cancellation for now. */
> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> + while (1)
> + {
> + int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> + cancelhandling
> + & ~CANCELTYPE_BITMASK,
> + cancelhandling);
> + if (__glibc_likely (curval == cancelhandling))
> + /* Successfully replaced the value. */
> + break;
> +
> + /* Prepare for the next round. */
> + cancelhandling = curval;
> + }
> +
> + buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> + ? PTHREAD_CANCEL_ASYNCHRONOUS
> + : PTHREAD_CANCEL_DEFERRED);
> +
> + THREAD_SETMEM (self, cleanup, buffer);
> +}
> +libc_hidden_def (__libc_cleanup_push_defer)
>
> void
> -__libc_cleanup_routine (struct __pthread_cleanup_frame *f)
> +__libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
> {
> - if (f->__do_it)
> - f->__cancel_routine (f->__cancel_arg);
> + struct pthread *self = THREAD_SELF;
> +
> + THREAD_SETMEM (self, cleanup, buffer->__prev);
> +
> + int cancelhandling;
> + if (__builtin_expect (buffer->__canceltype != PTHREAD_CANCEL_DEFERRED, 0)
> + && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
> + & CANCELTYPE_BITMASK) == 0)
> + {
> + while (1)
> + {
> + int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> + cancelhandling
> + | CANCELTYPE_BITMASK,
> + cancelhandling);
> + if (__glibc_likely (curval == cancelhandling))
> + /* Successfully replaced the value. */
> + break;
> +
> + /* Prepare for the next round. */
> + cancelhandling = curval;
> + }
> +
> + CANCELLATION_P (self);
> + }
> }
> +libc_hidden_def (__libc_cleanup_pop_restore)
Ok.
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 865ee8db29..c2b563cc68 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -96,8 +96,6 @@ static const struct pthread_functions pthread_functions =
> .ptr___pthread_key_create = __pthread_key_create,
> .ptr___pthread_getspecific = __pthread_getspecific,
> .ptr___pthread_setspecific = __pthread_setspecific,
> - .ptr__pthread_cleanup_push_defer = __pthread_cleanup_push_defer,
> - .ptr__pthread_cleanup_pop_restore = __pthread_cleanup_pop_restore,
> .ptr_nthreads = &__nptl_nthreads,
> .ptr___pthread_unwind = &__pthread_unwind,
> .ptr__nptl_deallocate_tsd = __nptl_deallocate_tsd,
Ok.
> diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h
> index dea96121b3..56fe83f69f 100644
> --- a/sysdeps/nptl/libc-lock.h
> +++ b/sysdeps/nptl/libc-lock.h
> @@ -143,39 +143,40 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t;
> __libc_maybe_call (__pthread_mutex_unlock, (&(NAME).mutex), 0)
> #endif
>
> -/* Note that for I/O cleanup handling we are using the old-style
> - cancel handling. It does not have to be integrated with C++ since
> - no C++ code is called in the middle. The old-style handling is
> - faster and the support is not going away. */
> -extern void _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
> - void (*routine) (void *), void *arg);
> -extern void _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
> - int execute);
> +/* Put the unwind buffer BUFFER on the per-thread callback stack. The
> + caller must fill BUFFER->__routine and BUFFER->__arg before calling
> + this function. */
> +void __libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer);
> +libc_hidden_proto (__libc_cleanup_push_defer)
> +/* Remove BUFFER from the unwind callback stack. The caller must invoke
> + the callback if desired. */
> +void __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer);
> +libc_hidden_proto (__libc_cleanup_pop_restore)
>
Ok.
> /* Start critical region with cleanup. */
> -#define __libc_cleanup_region_start(DOIT, FCT, ARG) \
> - { struct _pthread_cleanup_buffer _buffer; \
> - int _avail; \
> - if (DOIT) { \
> - _avail = PTFAVAIL (_pthread_cleanup_push_defer); \
> - if (_avail) { \
> - __libc_ptf_call_always (_pthread_cleanup_push_defer, (&_buffer, FCT, \
> - ARG)); \
> - } else { \
> - _buffer.__routine = (FCT); \
> - _buffer.__arg = (ARG); \
> - } \
> - } else { \
> - _avail = 0; \
> - }
> +#define __libc_cleanup_region_start(DOIT, FCT, ARG) \
> + { bool _cleanup_start_doit; \
> + struct _pthread_cleanup_buffer _buffer; \
> + /* Non-addresable copy of FCT, so that we avoid indirect calls on \
s/addresable/addressable
> + the non-unwinding path. */ \
> + void (*_cleanup_routine) (void *) = (FCT); \
> + _buffer.__arg = (ARG); \
> + if (DOIT) \
> + { \
> + _cleanup_start_doit = true; \
> + _buffer.__routine = _cleanup_routine; \
> + __libc_cleanup_push_defer (&_buffer); \
> + } \
> + else \
> + _cleanup_start_doit = false;
>
> /* End critical region with cleanup. */
> -#define __libc_cleanup_region_end(DOIT) \
> - if (_avail) { \
> - __libc_ptf_call_always (_pthread_cleanup_pop_restore, (&_buffer, DOIT));\
> - } else if (DOIT) \
> - _buffer.__routine (_buffer.__arg); \
> - }
> +#define __libc_cleanup_region_end(DOIT) \
> + if (_cleanup_start_doit) \
> + __libc_cleanup_pop_restore (&_buffer); \
> + if (DOIT) \
> + _cleanup_routine (_buffer.__arg); \
> + } /* matches __libc_cleanup_region_start */
>
Ok.
>
> /* Hide the definitions which are only supposed to be used inside libc in
> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
> index 4a0b96e6d9..1a861b0d3f 100644
> --- a/sysdeps/nptl/libc-lockP.h
> +++ b/sysdeps/nptl/libc-lockP.h
> @@ -251,32 +251,12 @@ _Static_assert (LLL_LOCK_INITIALIZER == 0, "LLL_LOCK_INITIALIZER != 0");
> /* Get once control variable. */
> #define __libc_once_get(ONCE_CONTROL) ((ONCE_CONTROL) != PTHREAD_ONCE_INIT)
>
> -/* Note that for I/O cleanup handling we are using the old-style
> - cancel handling. It does not have to be integrated with C++ snce
> - no C++ code is called in the middle. The old-style handling is
> - faster and the support is not going away. */
> -extern void _pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer,
> - void (*routine) (void *), void *arg);
> -extern void _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
> - int execute);
> -extern void _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
> - void (*routine) (void *), void *arg);
> -extern void _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
> - int execute);
> -
> -/* Sometimes we have to exit the block in the middle. */
> -#define __libc_cleanup_end(DOIT) \
> - if (_avail) { \
> - __libc_ptf_call_always (_pthread_cleanup_pop_restore, (&_buffer, DOIT));\
> - } else if (DOIT) \
> - _buffer.__routine (_buffer.__arg)
> -
> /* __libc_cleanup_push and __libc_cleanup_pop depend on exception
> handling and stack unwinding. */
> #ifdef __EXCEPTIONS
>
Ok.
> /* Normal cleanup handling, based on C cleanup attribute. */
> -__extern_inline void
> +static __always_inline void
> __libc_cleanup_routine (struct __pthread_cleanup_frame *f)
> {
> if (f->__do_it)
> @@ -396,8 +376,6 @@ weak_extern (__pthread_once)
> weak_extern (__pthread_initialize)
> weak_extern (__pthread_atfork)
> weak_extern (__pthread_setcancelstate)
> -weak_extern (_pthread_cleanup_push_defer)
> -weak_extern (_pthread_cleanup_pop_restore)
> # else
> # pragma weak __pthread_mutex_init
> # pragma weak __pthread_mutex_destroy
> @@ -420,8 +398,6 @@ weak_extern (_pthread_cleanup_pop_restore)
> # pragma weak __pthread_initialize
> # pragma weak __pthread_atfork
> # pragma weak __pthread_setcancelstate
> -# pragma weak _pthread_cleanup_push_defer
> -# pragma weak _pthread_cleanup_pop_restore
> # endif
> #endif
>
> diff --git a/sysdeps/nptl/pthread-functions.h b/sysdeps/nptl/pthread-functions.h
> index 97a5c48939..4268084b66 100644
> --- a/sysdeps/nptl/pthread-functions.h
> +++ b/sysdeps/nptl/pthread-functions.h
> @@ -57,10 +57,6 @@ struct pthread_functions
> int (*ptr___pthread_key_create) (pthread_key_t *, void (*) (void *));
> void *(*ptr___pthread_getspecific) (pthread_key_t);
> int (*ptr___pthread_setspecific) (pthread_key_t, const void *);
> - void (*ptr__pthread_cleanup_push_defer) (struct _pthread_cleanup_buffer *,
> - void (*) (void *), void *);
> - void (*ptr__pthread_cleanup_pop_restore) (struct _pthread_cleanup_buffer *,
> - int);
> #define HAVE_PTR_NTHREADS
> unsigned int *ptr_nthreads;
> void (*ptr___pthread_unwind) (__pthread_unwind_buf_t *)
>
Ok.
next prev parent reply other threads:[~2021-03-15 20:02 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-12 17:48 [PATCH 00/18] Repost of pending libpthread removal patches Florian Weimer
2021-03-12 17:48 ` [PATCH 01/18] nptl: Move pthread_mutex_consistent into libc Florian Weimer
2021-03-15 19:30 ` Adhemerval Zanella
2021-03-16 5:42 ` Florian Weimer
2021-03-16 13:49 ` Adhemerval Zanella
2021-03-16 13:53 ` Florian Weimer
2021-03-16 14:07 ` Adhemerval Zanella
2021-03-12 17:49 ` [PATCH 02/18] nptl: Move __pthread_cleanup_routine " Florian Weimer
2021-03-15 19:53 ` Adhemerval Zanella
2021-03-16 7:09 ` Florian Weimer
2021-03-12 17:49 ` [PATCH 03/18] nptl: Move legacy unwinding implementation " Florian Weimer
2021-03-15 20:02 ` Adhemerval Zanella [this message]
2021-03-16 7:03 ` Florian Weimer
2021-03-12 17:49 ` [PATCH 04/18] nptl: Move legacy cancelation handling into libc as compat symbols Florian Weimer
2021-03-16 14:09 ` Adhemerval Zanella
2021-03-16 14:45 ` Florian Weimer
2021-03-16 18:14 ` Adhemerval Zanella
2021-03-12 17:49 ` [PATCH 05/18] nptl: Remove longjmp, siglongjmp from libpthread Florian Weimer
2021-03-16 14:13 ` Adhemerval Zanella
2021-03-16 14:39 ` Florian Weimer
2021-03-12 17:49 ` [PATCH 06/18] Legacy unwinder: Remove definition of _Unwind_GetCFA Florian Weimer
2021-03-16 14:14 ` Adhemerval Zanella
2021-03-12 17:49 ` [PATCH 07/18] nptl: Move __pthread_cleanup_upto into libc Florian Weimer
2021-03-16 14:35 ` Adhemerval Zanella
2021-03-12 17:49 ` [PATCH 08/18] nptl: Move pthread_once and __pthread_once " Florian Weimer
2021-03-15 19:24 ` Florian Weimer
2021-03-12 17:49 ` [PATCH 09/18] nptl: Move __pthread_unwind_next " Florian Weimer
2021-03-12 17:49 ` [PATCH 10/18] csu: Move calling main out of __libc_start_main_impl Florian Weimer
2021-03-12 17:49 ` [PATCH 11/18] nptl: Move internal __nptl_nthreads variable into libc Florian Weimer
2021-03-12 17:49 ` [PATCH 12/18] nptl_db: Introduce DB_MAIN_ARRAY_VARIABLE Florian Weimer
2021-03-12 17:50 ` [PATCH 13/18] nptl: Move __pthread_keys global variable into libc Florian Weimer
2021-03-12 17:50 ` [PATCH 14/18] nptl: Move __nptl_deallocate_tsd " Florian Weimer
2021-03-12 17:50 ` [PATCH 15/18] nptl: Move pthread_exit " Florian Weimer
2021-03-12 17:50 ` [PATCH 16/18] nptl: Move pthread_setcancelstate " Florian Weimer
2021-03-12 17:50 ` [PATCH 17/18] nptl: Move pthread_setcanceltype " Florian Weimer
2021-03-12 17:50 ` [PATCH 18/18] nptl: Invoke the set_robust_list system call directly in fork Florian Weimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=187e63bc-2f7f-caf2-1b9e-219b6a0bcac1@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=fweimer@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).