Re: [Qemu-devel] [PATCH for-4.1] linux-user: Make sigaltstack stacks per-thread

[Laurent Vivier]

Le 25/07/2019 à 15:16, Peter Maydell a écrit :
> The alternate signal stack set up by the sigaltstack syscall is
> supposed to be per-thread.  We were incorrectly implementing it as
> process-wide.  This causes problems for guest binaries that rely on
> this.  Notably the Go runtime does, and so we were seeing crashes
> caused by races where two guest threads might incorrectly both
> execute on the same stack simultaneously.
> 
> Replace the global target_sigaltstack_used with a field
> sigaltstack_used in the TaskState, and make all the references to the
> old global instead get a pointer to the TaskState and use the field.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1696773
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> I've marked this as "for-4.1" but it is quite late in the release
> cycle and I think this could use more testing than I have given it...

It looks good. I'm going to test it with LTP on all the targets I have.
I will test the go program too.

Thanks,
Laurent

> 
> Thanks are due to:
>  * the original bug reporter, for providing a nice simple test case
>  * rr, for allowing me to capture and forensically examine a single
>    example of the failure
>  * the Go project for having a good clear HACKING.md that explained
>    their stack usage and mentioned specifically that signal stacks
>    are per-thread (per-M, in their terms)
>  * a colleague, for prodding me into actually spending the necessary
>    two days grovelling through gdb sessions and logs to figure out
>    what was actually going wrong
> ---
>  linux-user/qemu.h          |  2 ++
>  linux-user/signal-common.h |  1 -
>  linux-user/hppa/signal.c   |  3 ++-
>  linux-user/main.c          |  5 +++++
>  linux-user/signal.c        | 35 +++++++++++++++++++----------------
>  5 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 4258e4162d2..aac03346270 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -151,6 +151,8 @@ typedef struct TaskState {
>       */
>      int signal_pending;
>  
> +    /* This thread's sigaltstack, if it has one */
> +    struct target_sigaltstack sigaltstack_used;
>  } __attribute__((aligned(16))) TaskState;
>  
>  extern char *exec_path;
> diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
> index 51030a93069..1df1068552f 100644
> --- a/linux-user/signal-common.h
> +++ b/linux-user/signal-common.h
> @@ -19,7 +19,6 @@
>  
>  #ifndef SIGNAL_COMMON_H
>  #define SIGNAL_COMMON_H
> -extern struct target_sigaltstack target_sigaltstack_used;
>  
>  int on_sig_stack(unsigned long sp);
>  int sas_ss_flags(unsigned long sp);
> diff --git a/linux-user/hppa/signal.c b/linux-user/hppa/signal.c
> index b6927ee6735..d1a58feeb36 100644
> --- a/linux-user/hppa/signal.c
> +++ b/linux-user/hppa/signal.c
> @@ -111,10 +111,11 @@ void setup_rt_frame(int sig, struct target_sigaction 
> *ka,
>      abi_ulong frame_addr, sp, haddr;
>      struct target_rt_sigframe *frame;
>      int i;
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
>  
>      sp = get_sp_from_cpustate(env);
>      if ((ka->sa_flags & TARGET_SA_ONSTACK) && !sas_ss_flags(sp)) {
> -        sp = (target_sigaltstack_used.ss_sp + 0x7f) & ~0x3f;
> +        sp = (ts->sigaltstack_used.ss_sp + 0x7f) & ~0x3f;
>      }
>      frame_addr = QEMU_ALIGN_UP(sp, 64);
>      sp = frame_addr + PARISC_RT_SIGFRAME_SIZE32;
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a59ae9439de..8ffc5251955 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -180,6 +180,11 @@ void stop_all_tasks(void)
>  void init_task_state(TaskState *ts)
>  {
>      ts->used = 1;
> +    ts->sigaltstack_used = (struct target_sigaltstack) {
> +        .ss_sp = 0,
> +        .ss_size = 0,
> +        .ss_flags = TARGET_SS_DISABLE,
> +    };
>  }
>  
>  CPUArchState *cpu_copy(CPUArchState *env)
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5cd237834d9..5ca6d62b15d 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -25,12 +25,6 @@
>  #include "trace.h"
>  #include "signal-common.h"
>  
> -struct target_sigaltstack target_sigaltstack_used = {
> -    .ss_sp = 0,
> -    .ss_size = 0,
> -    .ss_flags = TARGET_SS_DISABLE,
> -};
> -
>  static struct target_sigaction sigact_table[TARGET_NSIG];
>  
>  static void host_signal_handler(int host_signum, siginfo_t *info,
> @@ -251,13 +245,17 @@ void set_sigmask(const sigset_t *set)
>  
>  int on_sig_stack(unsigned long sp)
>  {
> -    return (sp - target_sigaltstack_used.ss_sp
> -            < target_sigaltstack_used.ss_size);
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +    return (sp - ts->sigaltstack_used.ss_sp
> +            < ts->sigaltstack_used.ss_size);
>  }
>  
>  int sas_ss_flags(unsigned long sp)
>  {
> -    return (target_sigaltstack_used.ss_size == 0 ? SS_DISABLE
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +    return (ts->sigaltstack_used.ss_size == 0 ? SS_DISABLE
>              : on_sig_stack(sp) ? SS_ONSTACK : 0);
>  }
>  
> @@ -266,17 +264,21 @@ abi_ulong target_sigsp(abi_ulong sp, struct 
> target_sigaction *ka)
>      /*
>       * This is the X/Open sanctioned signal stack switching.
>       */
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
>      if ((ka->sa_flags & TARGET_SA_ONSTACK) && !sas_ss_flags(sp)) {
> -        return target_sigaltstack_used.ss_sp + 
> target_sigaltstack_used.ss_size;
> +        return ts->sigaltstack_used.ss_sp + ts->sigaltstack_used.ss_size;
>      }
>      return sp;
>  }
>  
>  void target_save_altstack(target_stack_t *uss, CPUArchState *env)
>  {
> -    __put_user(target_sigaltstack_used.ss_sp, &uss->ss_sp);
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +    __put_user(ts->sigaltstack_used.ss_sp, &uss->ss_sp);
>      __put_user(sas_ss_flags(get_sp_from_cpustate(env)), &uss->ss_flags);
> -    __put_user(target_sigaltstack_used.ss_size, &uss->ss_size);
> +    __put_user(ts->sigaltstack_used.ss_size, &uss->ss_size);
>  }
>  
>  /* siginfo conversion */
> @@ -708,12 +710,13 @@ abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong 
> uoss_addr, abi_ulong sp)
>  {
>      int ret;
>      struct target_sigaltstack oss;
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
>  
>      /* XXX: test errors */
>      if(uoss_addr)
>      {
> -        __put_user(target_sigaltstack_used.ss_sp, &oss.ss_sp);
> -        __put_user(target_sigaltstack_used.ss_size, &oss.ss_size);
> +        __put_user(ts->sigaltstack_used.ss_sp, &oss.ss_sp);
> +        __put_user(ts->sigaltstack_used.ss_size, &oss.ss_size);
>          __put_user(sas_ss_flags(sp), &oss.ss_flags);
>      }
>  
> @@ -760,8 +763,8 @@ abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong 
> uoss_addr, abi_ulong sp)
>              }
>          }
>  
> -        target_sigaltstack_used.ss_sp = ss.ss_sp;
> -        target_sigaltstack_used.ss_size = ss.ss_size;
> +        ts->sigaltstack_used.ss_sp = ss.ss_sp;
> +        ts->sigaltstack_used.ss_size = ss.ss_size;
>      }
>  
>      if (uoss_addr) {
>