Re: [PATCH 29/30] bsd-user/signal.c: implement do_sigaction

[Peter Maydell]

On Sun, 9 Jan 2022 at 16:32, Warner Losh <imp@bsdimp.com> wrote:
>
> Implement the meat of the sigaction(2) system call with do_sigaction and
> helper routiner block_signals (which is also used to implemement signal
> masking so it's global).
>
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Kyle Evans <kevans@freebsd.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  bsd-user/qemu.h   | 21 +++++++++++++
>  bsd-user/signal.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index b8c64ca0e5b..c643d6ba246 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -226,8 +226,29 @@ int host_to_target_signal(int sig);
>  void host_to_target_sigset(target_sigset_t *d, const sigset_t *s);
>  void target_to_host_sigset(sigset_t *d, const target_sigset_t *s);
>  long do_sigreturn(CPUArchState *regs, abi_ulong addr);
> +int do_sigaction(int sig, const struct target_sigaction *act,
> +                struct target_sigaction *oact);
>  void QEMU_NORETURN force_sig(int target_sig);
>  int qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t 
> *right);
> +/**
> + * block_signals: block all signals while handling this guest syscall
> + *
> + * Block all signals, and arrange that the signal mask is returned to
> + * its correct value for the guest before we resume execution of guest code.
> + * If this function returns non-zero, then the caller should immediately
> + * return -TARGET_ERESTARTSYS to the main loop, which will take the pending
> + * signal and restart execution of the syscall.
> + * If block_signals() returns zero, then the caller can continue with
> + * emulation of the system call knowing that no signals can be taken
> + * (and therefore that no race conditions will result).
> + * This should only be called once, because if it is called a second time
> + * it will always return non-zero. (Think of it like a mutex that can't
> + * be recursively locked.)
> + * Signals will be unblocked again by process_pending_signals().
> + *
> + * Return value: non-zero if there was a pending signal, zero if not.
> + */
> +int block_signals(void); /* Returns non zero if signal pending */
>
>  /* mmap.c */
>  int target_mprotect(abi_ulong start, abi_ulong len, int prot);
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index d11f5eddd7e..f055d1db407 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -231,6 +231,22 @@ static void tswap_siginfo(target_siginfo_t *tinfo, const 
> target_siginfo_t *info)
>      }
>  }
>
> +int block_signals(void)
> +{
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +    sigset_t set;
> +
> +    /*
> +     * It's OK to block everything including SIGSEGV, because we won't run 
> any
> +     * further guest code before unblocking signals in
> +     * process_pending_signals().
> +     */
> +    sigfillset(&set);
> +    sigprocmask(SIG_SETMASK, &set, 0);

For linux-user we rely on sigprocmask() in a multithreaded
program setting the signal mask for only the calling thread,
which isn't POSIX-mandated. (Arguably we should use
pthread_sigmask() instead, but we don't for basically
historical reasons since linux-user is host-OS-specific anyway.)
Does BSD have the same "this changes this thread's signal mask"
semantics for sigprocmask()?

> +
> +    return qatomic_xchg(&ts->signal_pending, 1);
> +}
> +
>  /* Returns 1 if given signal should dump core if not handled. */
>  static int core_dump_signal(int sig)
>  {
> @@ -534,6 +550,66 @@ static int fatal_signal(int sig)
>      }
>  }
>
> +/* do_sigaction() return host values and errnos */
> +int do_sigaction(int sig, const struct target_sigaction *act,
> +        struct target_sigaction *oact)
> +{
> +    struct target_sigaction *k;
> +    struct sigaction act1;
> +    int host_sig;
> +    int ret = 0;
> +
> +    if (sig < 1 || sig > TARGET_NSIG || TARGET_SIGKILL == sig ||
> +            TARGET_SIGSTOP == sig) {

Kernel seems to allow SIGKILL and SIGSTOP unless act is
non-NULL and act->sa_handler is SIG_DFL ?
https://github.com/freebsd/freebsd-src/blob/main/sys/kern/kern_sig.c#L747
(Compare linux-user commit ee3500d33a7431, a recent bugfix.)

> +        return -EINVAL;
> +    }
> +
> +    if (block_signals()) {
> +        return -TARGET_ERESTART;

Are we returning host errnos, or target errnos ?
(The linux-user version of this function has been a bit
confused about this in the past; I suspect you've picked up
fragments of it from different points in time.)

> +    }
> +
> +    k = &sigact_table[sig - 1];
> +    if (oact) {
> +        oact->_sa_handler = tswapal(k->_sa_handler);
> +        oact->sa_flags = tswap32(k->sa_flags);
> +        oact->sa_mask = k->sa_mask;
> +    }
> +    if (act) {
> +        /* XXX: this is most likely not threadsafe. */
> +        k->_sa_handler = tswapal(act->_sa_handler);
> +        k->sa_flags = tswap32(act->sa_flags);
> +        k->sa_mask = act->sa_mask;
> +
> +        /* Update the host signal state. */
> +        host_sig = target_to_host_signal(sig);
> +        if (host_sig != SIGSEGV && host_sig != SIGBUS) {
> +            memset(&act1, 0, sizeof(struct sigaction));
> +            sigfillset(&act1.sa_mask);
> +            act1.sa_flags = SA_SIGINFO;
> +            if (k->sa_flags & TARGET_SA_RESTART) {
> +                act1.sa_flags |= SA_RESTART;
> +            }
> +            /*
> +             *  Note: It is important to update the host kernel signal mask 
> to
> +             *  avoid getting unexpected interrupted system calls.
> +             */
> +            if (k->_sa_handler == TARGET_SIG_IGN) {
> +                act1.sa_sigaction = (void *)SIG_IGN;
> +            } else if (k->_sa_handler == TARGET_SIG_DFL) {
> +                if (fatal_signal(sig)) {
> +                    act1.sa_sigaction = host_signal_handler;
> +                } else {
> +                    act1.sa_sigaction = (void *)SIG_DFL;
> +                }
> +            } else {
> +                act1.sa_sigaction = host_signal_handler;
> +            }
> +            ret = sigaction(host_sig, &act1, NULL);
> +        }
> +    }
> +    return ret;
> +}

-- PMM