From c6c05052961e6066d36f5c7c6e32d36ea9f17dff Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Fri, 22 Jan 2021 11:20:41 +0100 Subject: [PATCH] coroutine-sigaltstack: overhaul SIGUSR2 treatment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (1) Disposition (action) for any given signal is global for the process. When two threads run coroutine-sigaltstack's qemu_coroutine_new() concurrently, they may interfere with each other: one of them may revert the SIGUSR2 handler to SIG_DFL, between the other thread (a) setting up coroutine_trampoline() as the handler and (b) raising SIGUSR2. That SIGUSR2 will then terminate the QEMU process abnormally. Outside of coroutine-sigaltstack, qemu does not use SIGUSR2 [*]. So move the pthread_sigmask() and sigaction() calls from qemu_coroutine_new() to coroutine_init(). This will keep the handler installed all the time, while also ensuring that all threads block SIGUSR2 all the time. [*] In user-mode emulation, the guest can register signal handlers for any signal but SIGSEGV and SIGBUS, so if it registers a SIGUSR2 handler, that will interfere with coroutine-sigaltstack. However, we do not use coroutines for user-mode emulation, so that is fine. (2) The temporary unblocking of SIGUSR2 in qemu_coroutine_new() with sigsuspend(), which implements the synchronous delivery of SIGUSR2 to the thread, is needlessly complicated. Remove the "tr_called"-based loop around sigsuspend(), as by the time we reach sigsuspend(), SIGUSR2 is certainly pending. (3) Relatedly, the top of the signal handler can only be entered via the sigsuspend() in qemu_coroutine_new(). Express this fact in the signal handler by abort()ing on (tr_handler==NULL). First, even if another process sends a SIGUSR2 to the QEMU process asynchronously, SIGUSR2 will only be unblocked by sigsuspend() in the next qemu_coroutine_new() execution, and by that time, the thread in question will have raised SIGUSR2 anyway. Second, there is no reason for sigsuspend() *not* to be both a compiler barrier and a memory barrier. (4) Finally, the "tr_handler" field should be more strongly typed; it only ever points to a CoroutineSigAltStack object. Based on Max's original patch. Cc: "Daniel P. Berrangé" Cc: Eric Blake Cc: Kevin Wolf Cc: Markus Armbruster Cc: Max Reitz Cc: Paolo Bonzini Cc: Peter Maydell Cc: Stefan Hajnoczi Signed-off-by: Max Reitz Signed-off-by: Laszlo Ersek --- util/coroutine-sigaltstack.c | 89 +++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 37 deletions(-) diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c index aade82afb8c0..a59513367532 100644 --- a/util/coroutine-sigaltstack.c +++ b/util/coroutine-sigaltstack.c @@ -44,21 +44,22 @@ typedef struct { /** * Per-thread coroutine bookkeeping */ typedef struct { /** Currently executing coroutine */ Coroutine *current; /** The default coroutine */ CoroutineSigAltStack leader; /** Information for the signal handler (trampoline) */ sigjmp_buf tr_reenter; - volatile sig_atomic_t tr_called; - void *tr_handler; + CoroutineSigAltStack *tr_handler; } CoroutineThreadState; static pthread_key_t thread_state_key; +static void coroutine_trampoline(int signal); + static CoroutineThreadState *coroutine_get_thread_state(void) { CoroutineThreadState *s = pthread_getspecific(thread_state_key); @@ -81,16 +82,51 @@ static void qemu_coroutine_thread_cleanup(void *opaque) static void __attribute__((constructor)) coroutine_init(void) { int ret; + sigset_t sigs; + struct sigaction sa; ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup); if (ret != 0) { fprintf(stderr, "unable to create leader key: %s\n", strerror(errno)); abort(); } + + /* + * This constructor function is running in the main thread. Masking SIGUSR2 + * here means that both the main thread, and directly or indirectly + * descendant threads thereof, will block SIGUSR2. (The signal mask is + * thread-specific, and inherited through pthread_create().) + */ + sigemptyset(&sigs); + sigaddset(&sigs, SIGUSR2); + pthread_sigmask(SIG_BLOCK, &sigs, NULL); + + /* + * Establish the SIGUSR2 signal handler. This is a process-wide operation, + * and so will apply to all threads from here on. + * + * We'll only unblock the delivery of SIGUSR2 in a narrow window, in + * qemu_coroutine_new(). + * + * While the handler is running, SIGUSR2 itself will be blocked due to + * setting none of the SA_NODEFER and SA_RESETHAND flags below. All other + * signals will *remain* blocked, from where we unblock SIGUSR2 in + * qemu_coroutine_new(). Still, we need to set "sa_mask" below *somehow*, + * and then it's simplest to make it block all signals, even though it + * makes no difference to the signal mask that's already going to be in + * effect when the handler is entered. + */ + sa.sa_handler = coroutine_trampoline; + sigfillset(&sa.sa_mask); + sa.sa_flags = SA_ONSTACK; + if (sigaction(SIGUSR2, &sa, NULL) != 0) { + perror("Unable to install SIGUSR2 handler"); + abort(); + } } /* "boot" function * This is what starts the coroutine, is called from the trampoline * (from the signal handler when it is not signal handling, read ahead * for more information). */ @@ -110,38 +146,47 @@ static void coroutine_bootstrap(CoroutineSigAltStack *self, Coroutine *co) /* * This is used as the signal handler. This is called with the brand new stack * (thanks to sigaltstack). We have to return, given that this is a signal * handler and the sigmask and some other things are changed. */ static void coroutine_trampoline(int signal) { CoroutineSigAltStack *self; Coroutine *co; CoroutineThreadState *coTS; /* Get the thread specific information */ coTS = coroutine_get_thread_state(); self = coTS->tr_handler; - coTS->tr_called = 1; + + if (!self) { + /* + * Never reached -- the top of coroutine_trampoline() can only be + * entered from the sigsuspend() call in qemu_coroutine_new(). + */ + abort(); + } + + coTS->tr_handler = NULL; co = &self->base; /* * Here we have to do a bit of a ping pong between the caller, given that * this is a signal handler and we have to do a return "soon". Then the * caller can reestablish everything and do a siglongjmp here again. */ if (!sigsetjmp(coTS->tr_reenter, 0)) { return; } /* * Ok, the caller has siglongjmp'ed back to us, so now prepare * us for the real machine state switching. We have to jump * into another function here to get a new stack context for * the auto variables (which have to be auto-variables * because the start of the thread happens later). Else with * PIC (i.e. Position Independent Code which is used when PTH * is built as a shared library) most platforms would * horrible core dump as experience showed. */ coroutine_bootstrap(self, co); } @@ -149,107 +194,77 @@ static void coroutine_trampoline(int signal) Coroutine *qemu_coroutine_new(void) { CoroutineSigAltStack *co; CoroutineThreadState *coTS; - struct sigaction sa; - struct sigaction osa; stack_t ss; stack_t oss; sigset_t sigs; - sigset_t osigs; sigjmp_buf old_env; /* The way to manipulate stack is with the sigaltstack function. We * prepare a stack, with it delivering a signal to ourselves and then * put sigsetjmp/siglongjmp where needed. * This has been done keeping coroutine-ucontext as a model and with the * pth ideas (GNU Portable Threads). See coroutine-ucontext for the basics * of the coroutines and see pth_mctx.c (from the pth project) for the * sigaltstack way of manipulating stacks. */ co = g_malloc0(sizeof(*co)); co->stack_size = COROUTINE_STACK_SIZE; co->stack = qemu_alloc_stack(&co->stack_size); co->base.entry_arg = &old_env; /* stash away our jmp_buf */ - coTS = coroutine_get_thread_state(); - coTS->tr_handler = co; - - /* - * Preserve the SIGUSR2 signal state, block SIGUSR2, - * and establish our signal handler. The signal will - * later transfer control onto the signal stack. - */ - sigemptyset(&sigs); - sigaddset(&sigs, SIGUSR2); - pthread_sigmask(SIG_BLOCK, &sigs, &osigs); - sa.sa_handler = coroutine_trampoline; - sigfillset(&sa.sa_mask); - sa.sa_flags = SA_ONSTACK; - if (sigaction(SIGUSR2, &sa, &osa) != 0) { - abort(); - } - /* * Set the new stack. */ ss.ss_sp = co->stack; ss.ss_size = co->stack_size; ss.ss_flags = 0; if (sigaltstack(&ss, &oss) < 0) { abort(); } /* * Now transfer control onto the signal stack and set it up. * It will return immediately via "return" after the sigsetjmp() - * was performed. Be careful here with race conditions. The - * signal can be delivered the first time sigsuspend() is - * called. + * was performed. */ - coTS->tr_called = 0; + coTS = coroutine_get_thread_state(); + coTS->tr_handler = co; pthread_kill(pthread_self(), SIGUSR2); sigfillset(&sigs); sigdelset(&sigs, SIGUSR2); - while (!coTS->tr_called) { - sigsuspend(&sigs); - } + sigsuspend(&sigs); /* * Inform the system that we are back off the signal stack by * removing the alternative signal stack. Be careful here: It * first has to be disabled, before it can be removed. */ sigaltstack(NULL, &ss); ss.ss_flags = SS_DISABLE; if (sigaltstack(&ss, NULL) < 0) { abort(); } sigaltstack(NULL, &ss); if (!(oss.ss_flags & SS_DISABLE)) { sigaltstack(&oss, NULL); } - /* - * Restore the old SIGUSR2 signal handler and mask - */ - sigaction(SIGUSR2, &osa, NULL); - pthread_sigmask(SIG_SETMASK, &osigs, NULL); - /* * Now enter the trampoline again, but this time not as a signal * handler. Instead we jump into it directly. The functionally * redundant ping-pong pointer arithmetic is necessary to avoid * type-conversion warnings related to the `volatile' qualifier and * the fact that `jmp_buf' usually is an array type. */ if (!sigsetjmp(old_env, 0)) { siglongjmp(coTS->tr_reenter, 1); } /* * Ok, we returned again, so now we're finished */ return &co->base; } -- 2.19.1.3.g30247aa5d201