Re: [PATCH v2 36/40] bsd-user/signal.c: implement do_sigaction

[Warner Losh]

> On Jan 24, 2022, at 6:29 PM, 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>
> 
> Pending Comments from Peter Maydell <peter.maydell@linaro.org>
> 
> (1) in block_signals, sigprocmast
> 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()?

FreeBSD changes this on a per-thread basis for both
sigprocmask and pthread_sigmask(). pthread_sigmask() just
does some extra stuff with SIGCANCEL for pthread_cancel
support which qemu doesn’t use. They are the same. I’m inclined
to leave it as sigprocmask() since I’m unsure what the implications
of doing funky things for SIGCANCEL would be.

> (2) do_sigaction, first if validating which signals can be sent
> 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.)

That fix is relevant, so I’ll bring that in. Thanks!

> (3) Noting confusion in do_sigaction between host and target
> errnos (they are identical on BSD, but we should still return
> the right sort in case it ever does matter).

Will fix before V3 of patches.

Warner

> ---
> bsd-user/signal-common.h | 22 ++++++++++++
> bsd-user/signal.c        | 76 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 98 insertions(+)
> 
> diff --git a/bsd-user/signal-common.h b/bsd-user/signal-common.h
> index 786ec592d18..7ff8e8f2e40 100644
> --- a/bsd-user/signal-common.h
> +++ b/bsd-user/signal-common.h
> @@ -9,7 +9,29 @@
> #ifndef SIGNAL_COMMON_H
> #define SIGNAL_COMMON_H
> 
> +/**
> + * 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 */
> +
> long do_rt_sigreturn(CPUArchState *env);
> +int do_sigaction(int sig, const struct target_sigaction *act,
> +                struct target_sigaction *oact);
> abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong 
> sp);
> long do_sigreturn(CPUArchState *env, abi_ulong addr);
> void force_sig_fault(int sig, int code, abi_ulong addr);
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index 05caf812ccb..42021556d65 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -309,6 +309,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);
> +
> +    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)
> {
> @@ -554,6 +570,66 @@ static void host_signal_handler(int host_sig, siginfo_t 
> *info, void *puc)
>     cpu_exit(thread_cpu);
> }
> 
> +/* 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 || sig == TARGET_SIGKILL ||
> +        sig == TARGET_SIGSTOP) {
> +        return -EINVAL;
> +    }
> +
> +    if (block_signals()) {
> +        return -TARGET_ERESTART;
> +    }
> +
> +    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;
> +}
> +
> static inline abi_ulong get_sigframe(struct target_sigaction *ka,
>         CPUArchState *env, size_t frame_size)
> {
> -- 
> 2.33.1
>