Re: Thread safety of coroutine-sigaltstack

[Eric Blake]

On 1/20/21 10:26 AM, Max Reitz wrote:
> Hi,
> 
> I’ve run into trouble with Vladimir’s async backup series on MacOS,
> namely that iotest 256 fails with qemu exiting because of a SIGUSR2.
> 
> Turns out this is because MacOS (-xcode) uses coroutine-sigaltstack,
> when I use this on Linux, I get the same error.
> 
> (You can find the series applied on my block branch e.g. here:
> 
> https://github.com/XanClic/qemu.git block
> )
> 
> Some debugging later I found that the problem seems to be two threads
> simultaneously creating a coroutine.  It makes sense that this case
> would appear with Vladimir’s series and iotest 256, because 256 runs two
> backup jobs in two different threads in a transaction, i.e. they’re
> launched simultaneously.  The async backup series makes backup use many
> concurrent coroutines and so by default launches 64+x coroutines when
> the backup is started.  Thus, the case of two coroutines created
> concurrently in two threads is very likely to occur.
> 
> I think the problem is in coroutine-sigaltstack’s qemu_coroutine_new().
> It sets up a SIGUSR2 handler, then changes the signal handling stack,
> then raises SIGUSR2, then reverts the signal handling stack and the
> SIGUSR2 handler.  As far as I’m aware, setting up signal handlers and
> changing the signal handling stack are both process-global operations,
> and so if two threads do so concurrently, they will interfere with each
> other.

Yes, that is absolutely correct - messing with the signal handlers is
process-wide.  I guess we've been lucky that we haven't been trying to
create coroutines in separate threads in the past.

>  What usually happens is that one thread sets up everything,
> while the other is already in the process of reverting its changes: So
> the second thread reverts the SIGUSR2 handler to the default, and then
> the first thread raises SIGUSR2, thus making qemu exit.
> 
> (Could be worse though.  Both threads could set up the sigaltstack, then
> both raise SIGUSR2, and then we get one coroutine_trampoline()
> invocation in each thread, but both would use the same stack.  But I
> don’t think I’ve ever seen that happen, presumably because the race time
> window is much shorter.)
> 
> Now, this all seems obvious to me, but I’m wondering...  If
> coroutine-sigaltstack really couldn’t create coroutines concurrently,
> why wouldn’t we have noticed before?  I mean, this new backup case is
> kind of a stress test, yes, but surely we would have seen the problem
> already, right?  That’s why I’m not sure whether my analysis is correct.

I'm not sure if there is anything else going wrong, but you have
definitely uncovered a latent problem, and I agree that a mutex is the
right way to fix it.

> 
> Anyway, I’ve attached a patch that wraps the whole SIGUSR2 handling
> section in a mutex, and that makes 256 pass reliably with Vladimir’s
> async backup series.  Besides being unsure whether the problem is really
> in coroutine-sigaltstack, I also don’t know whether getting out the big
> guns and wrapping everything in the mutex is the best solution.  So,
> it’s an RFC, I guess.
> 
> Max


>>From 08d4bb6a98fa731025683f20afe1381291d26031 Mon Sep 17 00:00:00 2001
> From: Max Reitz <mreitz@redhat.com>
> Date: Wed, 20 Jan 2021 16:59:40 +0100
> Subject: [RFC] coroutine-sigaltstack: Add SIGUSR2 mutex
> 
> Modifying signal handlers or the signal handling stack is a
> process-global operation.  When two threads run coroutine-sigaltstack's
> qemu_coroutine_new() concurrently, thay may interfere with each other,

they

> e.g.:
> 
> - One of the threads may revert the SIGUSR2 handler back to the default
>   between the other thread setting up coroutine_trampoline() as the
>   handler and raising SIGUSR2.  That SIGUSR2 will then lead to the
>   process exiting.
> 
> - Both threads may set up their coroutine stack with sigaltstack()
>   simultaneously, so that only one of them sticks.  Both then raise
>   SIGUSR2, which goes to each of the threads separately, but both signal
>   handler invocations will then use the same stack, which cannot work.
> 
> We have to ensure that only one thread at a time can modify the
> process-global SIGUSR2 handler and the signal handling stack.  To do so,
> wrap the whole section where that is done in a mutex.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  util/coroutine-sigaltstack.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org