Re: [Qemu-devel] [PATCH 18/34] linux-user: Fix race between multiple signals

[Peter Maydell]

On 6 September 2015 at 00:57, Timothy E Baldwin
<address@hidden> wrote:
> If multiple host signals are recieved in quick succession they would
> be queued in TaskState then delivered to the guest in spite of
> signals being blocked. Fixed by keeping host signals blocked until
> process_pending_signals() runs, this needs the guest signal state
> to be maintained by Qemu.
>
> Blocking host signals also ensures the correct behaviour with respect
> to multiple threads and the overrun count of timer related signals.
> Alas blocking and queuing in qemu is still needed because of virtual
> processor exceptions, SIGSEGV and SIGBUS.
>
> Blocking signals inside process_pending_signals() protects against
> concurrency problems with respect to signal handlers.
>
> Signed-off-by: Timothy Edward Baldwin <address@hidden>
>
> Conflicts:
>         linux-user/qemu.h

So, my primary problem with this patch is that it doesn't really
explain the underlying design.

We could really use a comment (say, at the top of signal.c) which describes
the overall design approach to signal handling, including when and how
a syscall implementation should block signals, when it needs to use
safe_syscall, when the host signal mask is the same as the target's
and when it is not, and so on. (Providing a comment like this would
make this patch much easier to review -- it is always harder work having
to reverse engineer the design from the code.)

> ---
>  linux-user/qemu.h    |   8 +++-
>  linux-user/signal.c  | 111 
> ++++++++++++++++++++++++++++++++++-----------------
>  linux-user/syscall.c |  49 +++++++++++++++--------
>  3 files changed, 113 insertions(+), 55 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 57c7e43..f2235eb3 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -127,14 +127,17 @@ typedef struct TaskState {
>  #endif
>      uint32_t stack_base;
>      int used; /* non zero if used */
> -    bool sigsegv_blocked; /* SIGSEGV blocked by guest */
>      struct image_info *info;
>      struct linux_binprm *bprm;
>
>      struct emulated_sigtable sigtab[TARGET_NSIG];
>      struct sigqueue sigqueue_table[MAX_SIGQUEUE_SIZE]; /* siginfo queue */
>      struct sigqueue *first_free; /* first free siginfo queue entry */
> -    int signal_pending; /* non zero if a signal may be pending */
> +    sigset_t signal_mask;
> +
> +    /* non zero if host signals blocked, bit 1 set if signal pending */
> +    volatile int signal_pending;

"volatile sig_atomic_t" would be preferable (if only because it clues
the reader into expect that there might be interactions between signal
handler and main code for this variable).

> +
>  } __attribute__((aligned(16))) TaskState;
>
>  extern char *exec_path;
> @@ -255,6 +258,7 @@ long do_sigreturn(CPUArchState *env);
>  long do_rt_sigreturn(CPUArchState *env);
>  abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong 
> sp);
>  int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
> +int block_signals(void); /* Returns non zero if signal pending */

This function needs a proper doc comment.

>
>  #ifdef TARGET_I386
>  /* vm86.c */
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 09551ba..3e272a5 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -197,53 +197,66 @@ void target_to_host_old_sigset(sigset_t *sigset,
>      target_to_host_sigset(sigset, &d);
>  }
>
> +int block_signals(void)
> +{
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +    sigset_t set;
> +    int pending;
> +
> +    sigfillset(&set);
> +    sigdelset(&set, SIGSEGV);
> +    sigdelset(&set, SIGBUS);

An explanation of why SIGSEGV and SIGBUS are special would be helpful.

> +    sigprocmask(SIG_SETMASK, &set, 0);

Should this be setting the signal mask for the thread only
(via pthread_sigmask) rather than the whole process?

> +
> +    pending = ts->signal_pending;
> +    pending |= 2;
> +    ts->signal_pending = pending;

Worth a comment that it's ok to do this non-atomically because we
have all signals blocked.

> +
> +#ifdef TARGET_USE_ERESTARTSYS
> +    return pending & 1;
> +#endif
> +    return 0;
> +}
> +
>  /* Wrapper for sigprocmask function
>   * Emulates a sigprocmask in a safe way for the guest. Note that set and 
> oldset
> - * are host signal set, not guest ones. This wraps the sigprocmask host calls
> - * that should be protected (calls originated from guest)
> + * are host signal set, not guest ones.
>   */

Why have you dropped the bit of the comment about wrapping the calls
originating from guests? AFAICT it's still true.

>  int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
>  {
> -    int ret;
> -    sigset_t val;
> -    sigset_t *temp = NULL;
> -    CPUState *cpu = thread_cpu;
> -    TaskState *ts = (TaskState *)cpu->opaque;
> -    bool segv_was_blocked = ts->sigsegv_blocked;
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +    if (oldset) {
> +        *oldset = ts->signal_mask;
> +    }
>
>      if (set) {
> -        bool has_sigsegv = sigismember(set, SIGSEGV);
> -        val = *set;
> -        temp = &val;
> +        int i;
>
> -        sigdelset(temp, SIGSEGV);
> +        if (block_signals()) {
> +            return -TARGET_ERESTARTSYS;
> +        }
>
>          switch (how) {
>          case SIG_BLOCK:
> -            if (has_sigsegv) {
> -                ts->sigsegv_blocked = true;
> -            }
> +            sigorset(&ts->signal_mask, &ts->signal_mask, set);
>              break;
>          case SIG_UNBLOCK:
> -            if (has_sigsegv) {
> -                ts->sigsegv_blocked = false;
> +            for (i = 0; i != NSIG; ++i) {
> +                if (sigismember(set, i)) {
> +                    sigdelset(&ts->signal_mask, i);
> +                }
>              }
>              break;
>          case SIG_SETMASK:
> -            ts->sigsegv_blocked = has_sigsegv;
> +            ts->signal_mask = *set;
>              break;
>          default:
>              g_assert_not_reached();
>          }
> -    }
> -
> -    ret = sigprocmask(how, temp, oldset);
>
> -    if (oldset && segv_was_blocked) {
> -        sigaddset(oldset, SIGSEGV);
>      }
> -
> -    return ret;
> +    return 0;
>  }
>
>  /* siginfo conversion */
> @@ -374,6 +387,7 @@ static int core_dump_signal(int sig)
>
>  void signal_init(void)
>  {
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
>      struct sigaction act;
>      struct sigaction oact;
>      int i, j;
> @@ -389,6 +403,9 @@ void signal_init(void)
>          target_to_host_signal_table[j] = i;
>      }
>
> +    /* Set the signal mask from the host mask. */
> +    sigprocmask(0, 0, &ts->signal_mask);
> +
>      /* set all host signal handlers. ALL signals are blocked during
>         the handlers to serialize them. */
>      memset(sigact_table, 0, sizeof(sigact_table));
> @@ -508,7 +525,7 @@ int queue_signal(CPUArchState *env, int sig, 
> target_siginfo_t *info)
>      queue = gdb_queuesig ();
>      handler = sigact_table[sig - 1]._sa_handler;
>
> -    if (ts->sigsegv_blocked && sig == TARGET_SIGSEGV) {
> +    if (sig == TARGET_SIGSEGV && sigismember(&ts->signal_mask, SIGSEGV)) {
>          /* Guest has blocked SIGSEGV but we got one anyway. Assume this
>           * is a forced SIGSEGV (ie one the kernel handles via force_sig_info
>           * because it got a real MMU fault). A blocked SIGSEGV in that
> @@ -605,6 +622,11 @@ static void host_signal_handler(int host_signum, 
> siginfo_t *info,
>
>      host_to_target_siginfo_noswap(&tinfo, info);
>      if (queue_signal(env, sig, &tinfo) == 1) {
> +        /* Block host signals until target signal handler entered */
> +        sigfillset(&uc->uc_sigmask);
> +        sigdelset(&uc->uc_sigmask, SIGSEGV);
> +        sigdelset(&uc->uc_sigmask, SIGBUS);
> +
>          /* interrupt the virtual CPU as soon as possible */
>          cpu_exit(thread_cpu);
>      }
> @@ -5628,26 +5650,40 @@ void process_pending_signals(CPUArchState *cpu_env)
>      CPUState *cpu = ENV_GET_CPU(cpu_env);
>      int sig;
>      abi_ulong handler;
> -    sigset_t set, old_set;
> +    sigset_t set;
>      target_sigset_t target_old_set;
>      struct emulated_sigtable *k;
>      struct target_sigaction *sa;
>      struct sigqueue *q;
>      TaskState *ts = cpu->opaque;
>
> -    if (!ts->signal_pending)
> +restart:
> +    if (!ts->signal_pending) {
>          return;
> +    }
>
>      /* FIXME: This is not threadsafe.  */
> +    sigfillset(&set);
> +    sigprocmask(SIG_SETMASK, &set, 0);
> +
> + next_signal:

I'm not a great fan of this use of goto for flow control.

>      k = ts->sigtab;
>      for(sig = 1; sig <= TARGET_NSIG; sig++) {
> -        if (k->pending)
> +        if (k->pending && (
> +                    !sigismember(&ts->signal_mask, 
> target_to_host_signal_table[sig])
> +                    || sig == TARGET_SIGSEGV)) {

It's not entirely clear why this is a special case for SIGSEGV only,
but we treat SIGSEGV and SIGBUS mostly the same elsewhere.

>              goto handle_signal;
> +        }
>          k++;
>      }
> -    /* if no signal is pending, just return */
> +
> +    /* if no signal is pending, unblock signals and restart */
>      ts->signal_pending = 0;
> -    return;
> +    set = ts->signal_mask;
> +    sigdelset(&set, SIGSEGV);
> +    sigdelset(&set, SIGBUS);
> +    sigprocmask(SIG_SETMASK, &set, 0);
> +    goto restart; /* Another signal? */
>
>   handle_signal:
>  #ifdef DEBUG_SIGNAL
> @@ -5668,7 +5704,7 @@ void process_pending_signals(CPUArchState *cpu_env)
>          handler = sa->_sa_handler;
>      }
>
> -    if (ts->sigsegv_blocked && sig == TARGET_SIGSEGV) {
> +    if (sig == TARGET_SIGSEGV && sigismember(&ts->signal_mask, SIGSEGV)) {
>          /* Guest has blocked SIGSEGV but we got one anyway. Assume this
>           * is a forced SIGSEGV (ie one the kernel handles via force_sig_info
>           * because it got a real MMU fault), and treat as if default handler.
> @@ -5698,11 +5734,12 @@ void process_pending_signals(CPUArchState *cpu_env)
>          if (!(sa->sa_flags & TARGET_SA_NODEFER))
>              sigaddset(&set, target_to_host_signal(sig));
>
> -        /* block signals in the handler using Linux */
> -        do_sigprocmask(SIG_BLOCK, &set, &old_set);
>          /* save the previous blocked signal state to restore it at the
>             end of the signal execution (see do_sigreturn) */
> -        host_to_target_sigset_internal(&target_old_set, &old_set);
> +        host_to_target_sigset_internal(&target_old_set, &ts->signal_mask);
> +
> +        /* block signals in the handler */
> +        sigorset(&ts->signal_mask, &ts->signal_mask, &set);
>
>          /* if the CPU is in VM86 mode, we restore the 32 bit values */
>  #if defined(TARGET_I386) && !defined(TARGET_X86_64)
> @@ -5722,9 +5759,11 @@ void process_pending_signals(CPUArchState *cpu_env)
>          else
>              setup_frame(sig, sa, &target_old_set, cpu_env);
>  #endif
> -       if (sa->sa_flags & TARGET_SA_RESETHAND)
> +        if (sa->sa_flags & TARGET_SA_RESETHAND)
>              sa->_sa_handler = TARGET_SIG_DFL;
>      }
>      if (q != &k->info)
>          free_sigqueue(cpu_env, q);
> +
> +    goto next_signal;
>  }
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 281fa2d..682090d 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4676,6 +4676,7 @@ static int do_fork(CPUArchState *env, unsigned int 
> flags, abi_ulong newsp,
>          new_cpu->opaque = ts;
>          ts->bprm = parent_ts->bprm;
>          ts->info = parent_ts->info;
> +        ts->signal_mask = parent_ts->signal_mask;
>          nptl_flags = flags;
>          flags &= ~CLONE_NPTL_FLAGS2;
>
> @@ -6492,9 +6493,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          {
>              sigset_t cur_set;
>              abi_ulong target_set;
> -            do_sigprocmask(0, NULL, &cur_set);
> -            host_to_target_old_sigset(&target_set, &cur_set);
> -            ret = target_set;
> +            ret = do_sigprocmask(0, NULL, &cur_set);
> +            if (!ret) {
> +                host_to_target_old_sigset(&target_set, &cur_set);
> +                ret = target_set;
> +            }
>          }
>          break;
>  #endif
> @@ -6503,12 +6506,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          {
>              sigset_t set, oset, cur_set;
>              abi_ulong target_set = arg1;
> -            do_sigprocmask(0, NULL, &cur_set);
> -            target_to_host_old_sigset(&set, &target_set);
> -            sigorset(&set, &set, &cur_set);
> -            do_sigprocmask(SIG_SETMASK, &set, &oset);
> -            host_to_target_old_sigset(&target_set, &oset);
> -            ret = target_set;
> +            ret = do_sigprocmask(0, NULL, &cur_set);
> +            if (!ret) {
> +                target_to_host_old_sigset(&set, &target_set);
> +                sigorset(&set, &set, &cur_set);
> +                do_sigprocmask(SIG_SETMASK, &set, &oset);
> +                host_to_target_old_sigset(&target_set, &oset);
> +                ret = target_set;
> +            }
>          }
>          break;
>  #endif
> @@ -6537,7 +6542,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>              mask = arg2;
>              target_to_host_old_sigset(&set, &mask);
>
> -            ret = get_errno(do_sigprocmask(how, &set, &oldset));
> +            ret = do_sigprocmask(how, &set, &oldset);
>              if (!is_error(ret)) {
>                  host_to_target_old_sigset(&mask, &oldset);
>                  ret = mask;
> @@ -6571,7 +6576,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  how = 0;
>                  set_ptr = NULL;
>              }
> -            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
> +            ret = do_sigprocmask(how, set_ptr, &oldset);
>              if (!is_error(ret) && arg3) {
>                  if (!(p = lock_user(VERIFY_WRITE, arg3, 
> sizeof(target_sigset_t), 0)))
>                      goto efault;
> @@ -6611,7 +6616,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  how = 0;
>                  set_ptr = NULL;
>              }
> -            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
> +            ret = do_sigprocmask(how, set_ptr, &oldset);
>              if (!is_error(ret) && arg3) {
>                  if (!(p = lock_user(VERIFY_WRITE, arg3, 
> sizeof(target_sigset_t), 0)))
>                      goto efault;
> @@ -6716,11 +6721,19 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          break;
>  #ifdef TARGET_NR_sigreturn
>      case TARGET_NR_sigreturn:
> -        ret = do_sigreturn(cpu_env);
> +        if (block_signals()) {
> +            ret = -TARGET_ERESTARTSYS;
> +        } else {
> +            ret = do_sigreturn(cpu_env);
> +        }
>          break;
>  #endif
>      case TARGET_NR_rt_sigreturn:
> -        ret = do_rt_sigreturn(cpu_env);
> +        if (block_signals()) {
> +            ret = -TARGET_ERESTARTSYS;
> +        } else {
> +            ret = do_rt_sigreturn(cpu_env);
> +        }
>          break;
>      case TARGET_NR_sethostname:
>          if (!(p = lock_user_string(arg1)))
> @@ -8744,9 +8757,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>              }
>              mask = arg2;
>              target_to_host_old_sigset(&set, &mask);
> -            do_sigprocmask(how, &set, &oldset);
> -            host_to_target_old_sigset(&mask, &oldset);
> -            ret = mask;
> +            ret = do_sigprocmask(how, &set, &oldset);
> +            if (!ret) {
> +                host_to_target_old_sigset(&mask, &oldset);
> +                ret = mask;
> +            }
>          }
>          break;
>  #endif
> --
> 2.1.4

thanks
-- PMM