On 6 September 2015 at 00:57, Timothy E Baldwin
<t.e.baldwi...@members.leeds.ac.uk> 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 <t.e.baldwi...@members.leeds.ac.uk>
>
> 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
