This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 02/21] nptl: Fix Race conditions in pthread cancellation (BZ#12683)
* Adhemerval Zanella:
> This patch is the initial fix for race conditions in NPTL cancellation codei
> by redefining how cancellable syscalls are defined and handled. Current
> buggy approach is to enable asynchronous cancellation prior to making the
> syscall and restore the previous cancellation type once the syscall returns.
First sentence appears to be garbled.
> As decribed in BZ#12683, this approach shows 2 important problems:
>
> 1. Cancellation can act after the syscall has returned from kernel, but
> before userspace saves the return value. It might result in a resource
> leak if the syscall allocated a resource or a side effect (partial
> read/write), and there is no way to program handle it with cancellation
> handlers.
>
> 2. If a signal is handled while the thread is blocked at a cancellable
> syscall, the entire signal handler runs with asynchronous cancellation
> enabled. This can lead to issues if the signal handler call functions
> which are async-signal-safe but not async-cancel-safe.
Do you mean “the cancellation signal handler runs” or “any signal
handler runs”? I think the latter is the much larger issue.
(I think it is customary to wrapper commit messages at column 72 or
something like that.)
> For cancellation to work correctly, there are 5 points at which the
> cancellation signal could arrive:
>
> 1. Before the final "testcancel" and before the syscall is made.
> 2. Between the "testcancel" and the syscall.
> 3. While the syscall is blocked and no side effects have yet taken place.
> 4. While the syscall is blocked but with some side effects already having
> taken place (e.g. a partial read or write).
> 5. After the syscall has returned.
For clarity, I'd suggest to say whether we want unblocking to take place
(i.e., the cancellation should happen eventually without any further
external event, such as the arrival of more data).
> And GLIBC wants to act on cancellation in cases 1, 2, and 3 but not in case
> 4 or 5. The proposed solution follows:
>
> * Handling case 1 is trivial: do a conditional branch based on whether the
> thread has received a cancellation request;
>
> * Case 2 can be caught by the signal handler determining that the saved
> program counter (from the ucontext_t) is in some address range beginning
> just before the "testcancel" and ending with the syscall instruction.
>
> * In this case, except for certain syscalls that ALWAYS fail with EINTR
> even for non-interrupting signals, the kernel will reset the program
> counter to point at the syscall instruction during signal handling, so
> that the syscall is restarted when the signal handler returns. So, from
> the signal handler's standpoint, this looks the same as case 2, and thus
> it's taken care of.
>
> * In this case, the kernel cannot restart the syscall; when it's
> interrupted by a signal, the kernel must cause the syscall to return
> with whatever partial result it obtained (e.g. partial read or write).
>
> * In this case, the saved program counter points just after the syscall
> instruction, so the signal handler won't act on cancellation.
> This one is equal to 4. since the program counter is past the syscall
> instruction already.
I'd suggest to match these items to the numbers explicitly.
> Another case that needs handling is syscalls that fail with EINTR even
> when the signal handler is non-interrupting. In this case, the syscall
> wrapper code can just check the cancellation flag when the errno result
> is EINTR, and act on cancellation if it's set.
>
> The proposed GLIBC adjustments are:
>
> 1. Remove the enable_asynccancel/disable_asynccancel function usage in
> syscall definition and instead make them call a common symbol that will
> check if cancellation is enabled (__syscall_cancel at
> nptl/libc-cancellation.c), call the arch-specific cancellable
> entry-point (__syscall_cancel_arch) and cancel the thread when required.
>
> 2. Provide a arch-specific symbol that contains global markers. These
> markers will be used in SIGCANCEL handler to check if the interruption
> has been called in a valid syscall and if the syscalls has been
> completed or not.
An arch-specific generic system call wrapper function?
> A reference implementation sysdeps/unix/sysv/linux/syscall_cancel.c is
> provided. However the markers may not be set on correct expected places
> depeding of how INTERNAL_SYSCALL_NCS is implemented by the underlying
> architecture, and it is uses compiler-speficic construct (asm volatile)
> to place the required markers. It is expected that all architectures
> implement an arch-specific.
Truncated sentence.
> 3. Rewrite SIGCANCEL asynchronous handler to check for both cancelling type
> and if current IP from signal handler falls between the global markes
> and act accordingly (sigcancel_handler at nptl/nptl-init.c).
s/markes/markers/
> diff --git a/nptl/libc-cancellation.c b/nptl/libc-cancellation.c
> index 37654cfcfe..430e0b962e 100644
> --- a/nptl/libc-cancellation.c
> +++ b/nptl/libc-cancellation.c
> @@ -18,7 +18,46 @@
>
> #include "pthreadP.h"
>
> +/* Cancellation function called by all cancellable syscalls. */
> +long int
> +__syscall_cancel (__syscall_arg_t nr, __syscall_arg_t a1,
> + __syscall_arg_t a2, __syscall_arg_t a3,
> + __syscall_arg_t a4, __syscall_arg_t a5,
> + __syscall_arg_t a6)
> +{
long int as the return value may not be quite right on x32.
> + pthread_t self = (pthread_t) THREAD_SELF;
> + struct pthread *pd = (struct pthread *) self;
> + long int result;
I would use inline definitions for new code, but that's your choice.
> @@ -286,38 +277,49 @@ __pthread_initialize_minimal_internal (void)
> THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
>
> #if defined SIGCANCEL || defined SIGSETXID
> - struct sigaction sa;
> - __sigemptyset (&sa.sa_mask);
>
> # ifdef SIGCANCEL
> /* Install the cancellation signal handler. If for some reason we
> cannot install the handler we do not abort. Maybe we should, but
> it is only asynchronous cancellation which is affected. */
> - sa.sa_sigaction = sigcancel_handler;
> - sa.sa_flags = SA_SIGINFO;
> - (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
> + {
> + struct sigaction sa;
> + sa.sa_sigaction = sigcancel_handler;
> + /* The signal handle should be non-interruptible to avoid the risk of
> + spurious EINTR caused by SIGCANCEL sent to process or if pthread_cancel
> + is called while cancellation is disabled in the target thread. */
> + sa.sa_flags = SA_SIGINFO | SA_RESTART;
> + sa.sa_mask = SIGALL_SET;
> + __libc_sigaction (SIGCANCEL, &sa, NULL);
> + }
> # endif
I'm a little bit concerned that adding SA_RESTART here adds more case
where we do not act on cancellation promptly. But that does not happen
because if we observe the cancellation conditions to apply in the
SIGCANCEL handler, we unwind straight from the signal handler, right?
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 070b3afa8d..cafd14f5af 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -296,20 +296,46 @@ extern void __nptl_unwind_freeres (void) attribute_hidden;
> #endif
>
>
> -/* Called when a thread reacts on a cancellation request. */
> static inline void
> __attribute ((noreturn, always_inline))
> -__do_cancel (void)
> +__do_cancel_with_result (void *result)
> {
> struct pthread *self = THREAD_SELF;
>
> - /* Make sure we get no more cancellations. */
> - THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
> + /* Make sure we get no more cancellations by clearing the cancel
> + state. */
> + int oldval = THREAD_GETMEM (self, cancelhandling);
> + while (1)
> + {
> + int newval = oldval | CANCELSTATE_BITMASK | EXITING_BITMASK;
> + if (oldval == newval)
> + break;
> +
> + oldval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> + oldval);
> + }
> +
> + THREAD_SETMEM (self, result, result);
>
> __pthread_unwind ((__pthread_unwind_buf_t *)
> THREAD_GETMEM (self, cleanup_jmp_buf));
> }
The function name should be more generic because it's for unwinding, not
cancellation.
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 64ac12e60a..b20254bdba 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -37,67 +37,24 @@ __pthread_cancel (pthread_t th)
> + /* Avoid signaling when thread attempts cancel itself (pthread_kill
> + is expensive). */
> + if (pd == THREAD_SELF
> + && (THREAD_GETMEM (pd, cancelhandling) & CANCELTYPE_BITMASK) == 0)
> + return 0;
I'm not sure if I understand this check. CANCELTYPE_BITMASK is a bit
misnamed (it's a single-bit masking). As far as I understand it, it's
set if async-cancel is enabled. Shouldn't we call __do_cancel directly
here in case of an async self-cancel? And otherwise defer cancellation?
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 130937c3c4..8cab7f970c 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -406,7 +406,7 @@ START_THREAD_DEFN
> /* If the parent was running cancellation handlers while creating
> the thread the new thread inherited the signal mask. Reset the
> cancellation signal mask. */
> - if (__glibc_unlikely (pd->parent_cancelhandling & CANCELING_BITMASK))
> + if (__glibc_unlikely (pd->parent_cancelhandling & CANCELED_BITMASK))
> {
> INTERNAL_SYSCALL_DECL (err);
> sigset_t mask;
This conflicts with the patch I posted for bug 25098. My change resets
the SIGCANCEL mask unconditionally, so the check is gone completely and
no longer needs adjustment.
> @@ -449,7 +449,8 @@ START_THREAD_DEFN
> have ownership (see CONCURRENCY NOTES above). */
> if (__glibc_unlikely (pd->stopped_start))
> {
> - int oldtype = CANCEL_ASYNC ();
> + int ct;
> + __pthread_setcanceltype (PTHREAD_CANCEL_ASYNCHRONOUS, &ct);
>
> /* Get the lock the parent locked to force synchronization. */
> lll_lock (pd->lock, LLL_PRIVATE);
> @@ -459,7 +460,7 @@ START_THREAD_DEFN
> /* And give it up right away. */
> lll_unlock (pd->lock, LLL_PRIVATE);
>
> - CANCEL_RESET (oldtype);
> + __pthread_setcanceltype (ct, NULL);
> }
I think htis is wrong. Either you need to use a cancellable lll_lock
here. Or maybe it is not even necessary to enable cancellation at all
because the new tread is eventually bound to make progress and will wake
up the futuex.
> diff --git a/sysdeps/nptl/librt-cancellation.c b/nptl/pthread_kill_internal.c
> similarity index 70%
> rename from sysdeps/nptl/librt-cancellation.c
> rename to nptl/pthread_kill_internal.c
> index 93ebe4aa71..b4f4f6dc78 100644
> --- a/sysdeps/nptl/librt-cancellation.c
> +++ b/nptl/pthread_kill_internal.c
> @@ -1,6 +1,6 @@
> -/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
> +/* Send a signal to a specific pthread. Internal version.
> + Copyright (C) 2002-2019 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
> - Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
>
> The GNU C Library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
> @@ -16,9 +16,11 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -#include <nptl/pthreadP.h>
> +#include <pthreadP.h>
>
> -
> -#define __pthread_enable_asynccancel __librt_enable_asynccancel
> -#define __pthread_disable_asynccancel __librt_disable_asynccancel
> -#include <nptl/cancellation.c>
> +int
> +__pthread_kill_internal (pthread_t threadid, int signo)
> +{
> + return ENOSYS;
> +}
> +hidden_def (__pthread_kill_internal)
I don't think we need this stub.
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index d771c31e46..84cc657aed 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -73,4 +73,4 @@ __pthread_setcanceltype (int type, int *oldtype)
>
> return 0;
> }
> -strong_alias (__pthread_setcanceltype, pthread_setcanceltype)
> +weak_alias (__pthread_setcanceltype, pthread_setcanceltype)
Why is this change necessary? pthread_setcanceltype is the only
function defined in this file, so there are no issues with static
linking. I'm concerned that this hides a problem elsewhere.
> diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> index 2e185dd748..75c0d53f3e 100644
> --- a/nptl/thrd_sleep.c
> +++ b/nptl/thrd_sleep.c
> @@ -24,13 +24,12 @@
> int
> thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> {
> - INTERNAL_SYSCALL_DECL (err);
> - int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> - if (INTERNAL_SYSCALL_ERROR_P (ret, err))
> + long int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, time_point, remaining);
The return type may need adjustment if the return type for the
cancellation wrapper changes.
> diff --git a/nptl/tst-cancel28.c b/nptl/tst-cancel28.c
> new file mode 100644
> index 0000000000..e27f5a0776
> --- /dev/null
> +++ b/nptl/tst-cancel28.c
> +static void *
> +writeopener (void *arg)
> +{
> + int fd;
> + for (;;)
> + {
> + fd = open (arg, O_WRONLY);
> + close (fd);
> + }
> + return NULL;
> +}
> +
> +static void *
> +leaker (void *arg)
> +{
> + int fd = open (arg, O_RDONLY);
> + TEST_VERIFY_EXIT (fd > 0);
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0);
> + close (fd);
> + return NULL;
> +}
I think these two functions *really* should check the return value of
close (or use xclose).
> +#define ITER_COUNT 1000
> +#define MAX_FILENO 1024
MAX_FILENO appears to be unused.
> +#define TIMEOUT 10
> +#include <support/test-driver.c>
TIMEOUT is smaller than the default test timeout.
> diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
> index e745dacac1..5628791de8 100644
> --- a/sysdeps/htl/pthreadP.h
> +++ b/sysdeps/htl/pthreadP.h
> @@ -25,6 +25,7 @@
>
> extern pthread_t __pthread_self (void);
> extern int __pthread_kill (pthread_t threadid, int signo);
> +extern int __pthread_kill_internal (pthread_t threadid, int signo);
> extern struct __pthread **__pthread_threads;
>
> extern int _pthread_mutex_init (pthread_mutex_t *mutex, const pthread_mutexattr_t *attr);
This change appears unnecessary because __pthread_kill_internal does not
seem to be used outside of nptl.
> diff --git a/sysdeps/nptl/cancellation-pc-check.h b/sysdeps/nptl/cancellation-pc-check.h
> new file mode 100644
> index 0000000000..8b26c4ec4e
> --- /dev/null
> +++ b/sysdeps/nptl/cancellation-pc-check.h
> +/* Check if the program counter (PC) from ucontext CTX is within the start and
> + then end boundary from the __syscall_cancel_arch bridge. Return TRUE if
> + the PC is within the boundary, meaning the syscall does not have any side
> + effects; or FALSE otherwise. */
> +static bool
> +ucontext_check_pc_boundary (void *ctx)
> +{
> + /* Both are defined in syscall_cancel.S. */
> + extern const char __syscall_cancel_arch_start[1];
> + extern const char __syscall_cancel_arch_end[1];
> +
> + uintptr_t pc = sigcontext_get_pc (ctx);
> + return pc >= (uintptr_t) __syscall_cancel_arch_start
> + && pc < (uintptr_t) __syscall_cancel_arch_end;
> +}
The name of this function should reflect that it is related to
cancellation.
Please add a comment explaining the kernel behavior regarding PC
(non-interrupted vs interrupted system calls), as found in the commit
message.
> +#endif
> diff --git a/sysdeps/nptl/cancellation-sigmask.h b/sysdeps/nptl/cancellation-sigmask.h
> new file mode 100644
> index 0000000000..77702c135e
> --- /dev/null
> +++ b/sysdeps/nptl/cancellation-sigmask.h
> +/* Add the SIGCANCEL signal on sigmask set at the ucontext CTX obtained from
> + the sigaction handler. */
> +static void
> +ucontext_add_cancel (void *ctx)
> +{
> + __sigaddset (&((ucontext_t*) ctx)->uc_sigmask, SIGCANCEL);
> +}
I find the name of the function rather unclear. Maybe
ucontext_block_sigcancel or something like that.
> diff --git a/sysdeps/unix/sysdep.h b/sysdeps/unix/sysdep.h
> index 10468c7129..04f30b8cbc 100644
> --- a/sysdeps/unix/sysdep.h
> +++ b/sysdeps/unix/sysdep.h
> +#define __SYSCALL_CANCEL0(name) \
> + (__syscall_cancel)(__NR_##name, 0, 0, 0, 0, 0, 0)
There are a bunch of formatting nits in the changes to this file.
“)(” should be “) (”. Likewise for “__SSC(”.
> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> index 1f240b8720..ddcdddcd5d 100644
> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c
> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> @@ -36,11 +36,9 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
>
> /* If the call is interrupted by a signal handler or encounters an error,
> it returns a positive value similar to errno. */
> - INTERNAL_SYSCALL_DECL (err);
> - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,
> + int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, clock_id, flags,
> req, rem);
> - return (INTERNAL_SYSCALL_ERROR_P (r, err)
> - ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> + return SYSCALL_CANCEL_ERROR (r) ? -r : 0;
> }
Shouldn't r change type here?
> diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> index b423673ed4..bd6d3b7cf7 100644
> --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> @@ -74,6 +74,12 @@
> ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \
> })
>
> +#define lll_futex_syscall_cp(...) \
> + ({ \
> + long int __ret = INTERNAL_SYSCALL_CANCEL (futex, __VA_ARGS__); \
> + __ret; \
> + })
> +
The statement expression appears unnecessary here.
> +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \
> + ({ \
> + long int __ret; \
> + int __op = FUTEX_WAIT; \
> + __ret = lll_futex_syscall_cp (futexp, \
> + __lll_private_flag (__op, private), \
> + val, timeout); \
> + __ret; \
> })
Is the statement expression needed? Should this be an inline function?
>
> -#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \
> - ({ \
> - int __oldtype = CANCEL_ASYNC (); \
> - long int __err = lll_futex_timed_wait (futexp, val, timeout, private); \
> - CANCEL_RESET (__oldtype); \
> - __err; \
> +#define lll_futex_clock_wait_bitset_cancel(futexp, val, clockid, \
> + timeout, private) \
> + ({ \
> + long int __ret; \
> + if (lll_futex_supported_clockid (clockid)) \
> + { \
> + const unsigned int clockbit = \
> + (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0; \
> + const int op = \
> + __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private); \
> + \
> + __ret = lll_futex_syscall_cp (futexp, op, val, \
> + timeout, NULL /* Unused. */, \
> + FUTEX_BITSET_MATCH_ANY); \
> + } \
> + else \
> + __ret = -EINVAL; \
> + __ret; \
> })
I do think this should be an inline function.
> --- a/sysdeps/unix/sysv/linux/pthread_kill.c
> +++ b/sysdeps/unix/sysv/linux/pthread_kill_internal.c
> @@ -16,24 +16,15 @@
> -strong_alias (__pthread_kill, pthread_kill)
> +hidden_def (__pthread_kill_internal)
Why hidden_def? I think you should use attribute_hidden on the
declaration and drop the libc_hidden_proto instead.
> diff --git a/sysdeps/unix/sysv/linux/syscall_cancel.c b/sysdeps/unix/sysv/linux/syscall_cancel.c
> new file mode 100644
> index 0000000000..79d66ec4f4
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/syscall_cancel.c
> +long int
> +__syscall_cancel_arch (volatile int *ch, __syscall_arg_t nr,
> + __syscall_arg_t a1, __syscall_arg_t a2,
> + __syscall_arg_t a3, __syscall_arg_t a4,
> + __syscall_arg_t a5, __syscall_arg_t a6)
> +{
> +#define ADD_LABEL(__label) \
> + asm volatile ( \
> + ".global " __label "\t\n" \
> + ".type " __label ",\%function\t\n" \
> + __label ":\n");
> +
> + ADD_LABEL ("__syscall_cancel_arch_start");
> + if (__glibc_unlikely (*ch & CANCELED_BITMASK))
> + __syscall_do_cancel();
> +
> + INTERNAL_SYSCALL_DECL(err);
> + long int result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6);
> + ADD_LABEL ("__syscall_cancel_arch_end");
> + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
> + return -INTERNAL_SYSCALL_ERRNO (result, err);
> + return result;
> +}
> +libc_hidden_def (__syscall_cancel_arch)
For bootstrapping, this is good enough, but I think we really, *really*
should remove this function, so that new ports do not use it by
accident.
> diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
> index fc9af51456..317bb7f973 100644
> --- a/sysdeps/unix/sysv/linux/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sysdep.h
> @@ -27,6 +27,26 @@
> -1l; \
> })
>
> +/* Check error from cancellable syscall and set errno accordingly.
> + Linux uses a negative return value to indicate syscall errors
> + and since version 2.1 the return value of a system call might be
> + negative even if the call succeeded (e.g., the `lseek' system call
> + might return a large offset).
> + Current contract is kernel make sure the no syscall returns a value
> + in -1 .. -4095 as a valid result so we can savely test with -4095. */
> +#define SYSCALL_CANCEL_ERROR(__ret) \
> + (__ret > -4096UL)
This doesn't look particularly type-safe. Does it have to be a macro?
If it is a macro, can we add type checks?
> +#define SYSCALL_CANCEL_RET(__ret) \
> + ({ \
> + if (SYSCALL_CANCEL_ERROR(__ret)) \
> + { \
> + __set_errno (-__ret); \
> + __ret = -1; \
> + } \
> + __ret; \
> + })
> +
> /* Provide a dummy argument that can be used to force register
> alignment for register pairs if required by the syscall ABI. */
> #ifdef __ASSUME_ALIGNED_REGISTER_PAIRS
I initially found this split from sysdeps/unix/sysdep.h a bit strange,
but I guess it does reflect the current layering.
Additional notes: {LIBC_,}CANCEL_{ASYNC,RESET} should be removed
everywhere as soon as possible. This may have a knockpm-effect where
__pthread_disable_asynccancel etc. can go as well.
Thanks,
Florian