Re: [PATCH] Updating gdbstub to allow safe multithreading in usermode emulation

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 28 May 2022 at 21:11, Ben Cohen <ben@thefarbeyond.com> wrote:
>
> I was testing some multi-threaded code in qemu's usermode and ran into
> issues with the gdbstub because the user mode qemu emulation spawns new
> threads when the process tries to make a new thread but the gdbstub does
> not handle the threads well. The current gdbstub has a single global
> struct which contains all the state data, and multiple threads can write
> to this struct simultaneously, causing gdb packets to be corrupted. The
> different threads can also try to read off the gdb socket at the same
> time causing the packet to be devided between threads. This patch is
> designed to add a single separate thread for the usermode gdbstub which
> will handle all the gdb comms and avoid the multithreading issues.
>
> To demonstrate that the mutlithreading was not working properly before
> and that it hopefully works properlly now, I wrote a small test program
> with some gdb scripts that can be found here:
> http://url6163.thefarbeyond.com/ls/click?upn=dUoerjbXT3NK9PiRMHEMD5Fm1RmJf0eUWTDkIDLODGZSRXu94ynuiGwQ-2FCFcimw5rndUfJbozecGKoOE5zbdfRVgadbVSeCrgP5IlB2UqPU-3DUBT-_E8SO-2FEypfU855L0ybQoiQY4Xaj8Z6NYzBoBK89OH-2BiIJOE8-2BoeErelzsKKZyRONZN5M7Gzw0Zs4K0tnG4gxJSOWW89OLEjRW7ISHPWO2lT6fJJUM88-2BLL6sh4BfexcL-2FKt7KhnEpzqyb9bd5UZ-2FR2iPVIjp7zfshwPtjJEHAHaIGeNZbI4nFw81hhs0N1tt9sAcC1ALVazbgnC5E-2F5ZChA-3D-3D

Thanks for this patch. I don't have time to do a full review of it,
but it sounds like you've definitely identified a bug. However
I'm not sure this is the right way to fix it.

Is signal handling really the only place where we need to be more careful
about the gdbstub handling for a multithreaded guest program?
How about other bits of gdbstub activity ?

Some more specific comments:

> +        /*
> +         * This mutex is first locked here to ensure that it is in a locked
> +         * state before the gdb_signal_handler_loop handles the next signal
> +         * and unlocks it.
> +         */
> +        qemu_mutex_lock(&signal_done_mutex);
> +        waiting_signal = &signal;
> +        /*
> +         * The thread locks this mutex again to wait until the
> +         * gdb_signal_handler_loop is finished handling the signal and has
> +         * unlocked the mutex.
> +         */
> +        qemu_mutex_lock(&signal_done_mutex);

This code is locking the same mutex twice in a row. That's not
guaranteed to do anything sensible, so it's not a valid thing to do.

> +        /*
> +         * Finally, unlock this mutex in preparation for the next call to
> +         * this function
> +         */
> +        qemu_mutex_unlock(&signal_done_mutex);
> +        sig = signal_result;
> +        if (!cpu->singlestep_enabled) {
> +            /*
> +             * If this thread is not stepping and is handling its signal
> +             * then it can always safely unlock this mutex.
> +             */
> +            qemu_mutex_unlock(&another_thread_is_stepping);
> +        } else {
> +            /*
> +             * If this thread was already stepping it will already be holding
> +             * this mutex so here try to lock instead of waiting on a lock.
> +             * This lock will prevent other non-stepping threads from handling
> +             * a signal until stepping is done.
> +             */
> +            qemu_mutex_trylock(&another_thread_is_stepping);
> +        }
> +    }
> +    /*
> +     * Unlock here to because we are done handling the signal and
> +     * another thread can now start handling a pending signal.
> +     */
> +    qemu_mutex_unlock(&signal_wait_mutex);
> +    return sig;
> +}

I have not analysed the code in detail but I get the impression that
maybe you're trying to use some of these mutexes to do jobs that
would be better done with a semaphore ?

> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index c35d7334b4..15bfb76cca 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -47,6 +47,19 @@ void gdb_do_syscallv(gdb_syscall_complete_cb cb, const char *fmt, va_list va);
>  int use_gdb_syscalls(void);
>
>  #ifdef CONFIG_USER_ONLY
> +/**
> + * gdb_thread_handle_signal
> + * @cpu_env: The guest thread's cpu env
> + * @sig: The signal being handled for the guest thread
> + *
> + * This function is a layer in between the gdb_handlesig function and the
> + * guest cpu threads. Instead of directly handling signals in the guest
> + * threads, this function passes off a signal to a handler loop thread running
> + * in the gdbstub that will handle each thread's signal atomically to avoid
> + * having races between threads to read and send data on the gdb socket. The
> + * function returns the signal value from gdb_handlesig
> + */
> +int gdb_thread_handle_signal(CPUState *cpu_env, int sig);
>  /**
>   * gdb_handlesig: yield control to gdb
>   * @cpu: CPU
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8d29bfaa6b..a252791217 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1068,7 +1068,11 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig,
>      /* dequeue signal */
>      k->pending = 0;
>
> +#ifdef CONFIG_USER_ONLY
> +    sig = gdb_thread_handle_signal(cpu, sig);
> +#else
>      sig = gdb_handlesig(cpu, sig);
> +#endif

You might as well have left the function name alone and changed
the implementation of it, which would avoid changing this file.
In particular, for linux-user/ code, as the name suggests,
CONFIG_USER_ONLY will always be set, so the #else here is
unnecessary. Plus you would need to change bsd-user/ if you
change the function name.

thanks
-- PMM