[Qemu-devel] [PATCH v2 00/19] linux-user: fix various signal race conditions

[Qemu-devel] [PATCH v2 00/19] linux-user: fix various signal race conditions

[Peter Maydell]

This patchset overhauls the linux-user signal handling code to
fix a number of race conditions. It is essentially a v2 of
Timothy Baldwin's original patchset, though I have addressed
code review issues, refactored it a little, fixed the occasional
minor bug and added some patches of my own for other issues I
spotted along the way.

The meat of the patchset is splitting out the guest thread's idea
of its signal mask from the host thread's actual signal mask. This
allows us to temporarily block all host signals as a method for
fixing some races:

 * block signals in host signal handler until we have processed
   the signal queue to deliver the guest signal (fixes a race
   where a second host signal could arrive and we would deliver
   it even if the guest handler's signal mask should prevent it)
 * block signals while we are manipulating QEMU data structures which
   the host signal handler reads (eg in sigaction syscall emulation)
 * block signals to fix races between signals and noninterruptible
   syscalls like pause, which we could in theory do with safe_syscall
   but which would be a pain to do that way because of variations
   in whether syscalls exist on different host architectures
 * block signals to fix races for complicated syscalls like fork
   which would be too painful to handle by trying to roll back
   if something was interrupted partway through

We also:
 * remove a lot of code that is made redundant by processing
   default signal actions in one place rather than two
 * make sure that synchronous signals correctly take priority
   over asynchronous signals
 * use safe_syscall for sigsuspend
 * use safe_syscall for kill/tkill/tgkill
 * make a better guess at which bits of the union in siginfo_t
   need to be converted by looking at si_code as well as si_signo
 * use __get_user and __put_user for siginfo conversion to avoid
   potential problems with misaligned guest addresses

Changes since Timothy's v1 patchset:
 * some patches at the front to factor out handle_pending_signal()
   to avoid using goto for flow control logic
 * new function set_sigmask() for setting signal mask when we have
   already blocked signals -- this allows us to define calling block_signals()
   twice to be illegal, which then means we can have signal_pending be a
   simple flag rather than a word with two flag bits in it
 * use the qemu atomics.h functions rather than raw volatile variable
   (it makes it clearer that the variable has to be handled with care IMHO)
 * bunch of extra commentary
 * add code to stop sigprocmask being able to mark SIGKILL, SIGSTOP as blocked
 * fixed handling of ssetmask
 * new patches to better handle conversion of siginfo_t structures
   (these fix problems with LTP tests like kill10 which try to kill
   processes by sending them an asynchronous SIGSEGV and expect to
   see the si_pid field in the resulting siginfo in the recipient.)

With all of these fixes plus the safe_syscall patches now in
master, the following LTP test cases which used to hang now do not:

 mq_timedreceive01 mq_timedsend01 kill10 kill11 msgrcv03
 nanosleep04 splice02 waitpid02 inotify06 pselect02 pselect02_64

(Not all of these were signal related issues, and a few might have
been fixed some time back.)

Next on my todo list after this is to expand the safe_syscall
support to more host architectures. There are also a few more
bugs lurking I suspect.

thanks
-- PMM


Peter Maydell (11):
  linux-user: Factor out handle_signal code from
    process_pending_signals()
  linux-user: Move handle_pending_signal() to avoid need for declaration
  linux-user: Fix stray tab-indent
  linux-user: Factor out uses of do_sigprocmask() from sigreturn code
  linux-user: Define macro for size of host kernel sigset_t
  linux-user: Use safe_syscall for sigsuspend syscalls
  linux-user: Fix race between multiple signals
  linux-user: Use safe_syscall for kill, tkill and tgkill syscalls
  linux-user: Use both si_code and si_signo when converting siginfo_t
  linux-user: Avoid possible misalignment in host_to_target_siginfo()
  linux-user: Avoid possible misalignment in target_to_host_siginfo()

Timothy E Baldwin (8):
  linux-user: Remove redundant default action check in queue_signal()
  linux-user: Remove redundant gdb_queuesig()
  linux-user: Remove real-time signal queuing
  linux-user: Queue synchronous signals separately
  linux-user: Block signals during sigaction() handling
  linux-user: pause() should not pause if signal pending
  linux-user: Restart exit() if signal pending
  linux-user: Restart fork() if signals pending

 gdbstub.c                 |  13 --
 include/exec/gdbstub.h    |   1 -
 linux-user/main.c         |   7 -
 linux-user/qemu.h         |  62 ++++-
 linux-user/signal.c       | 572 ++++++++++++++++++++++++++--------------------
 linux-user/syscall.c      | 124 ++++++----
 linux-user/syscall_defs.h |  15 ++
 7 files changed, 476 insertions(+), 318 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 01/19] linux-user: Factor out handle_signal code from process_pending_signals()

[Peter Maydell]

Factor out the code to handle a single signal from the
process_pending_signals() function. The use of goto for flow control
is OK currently, but would get significantly uglier if extended to
allow running the handle_signal code multiple times.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 8090b4d..a9ac491 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5765,33 +5765,40 @@ long do_rt_sigreturn(CPUArchState *env)
 
 #endif
 
+static void handle_pending_signal(CPUArchState *cpu_env, int sig);
+
 void process_pending_signals(CPUArchState *cpu_env)
 {
     CPUState *cpu = ENV_GET_CPU(cpu_env);
     int sig;
-    abi_ulong handler;
-    sigset_t set, old_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)
         return;
 
     /* FIXME: This is not threadsafe.  */
-    k = ts->sigtab;
     for(sig = 1; sig <= TARGET_NSIG; sig++) {
-        if (k->pending)
-            goto handle_signal;
-        k++;
+        if (ts->sigtab[sig - 1].pending) {
+            handle_pending_signal(cpu_env, sig);
+            return;
+        }
     }
     /* if no signal is pending, just return */
     ts->signal_pending = 0;
     return;
+}
+
+static void handle_pending_signal(CPUArchState *cpu_env, int sig)
+{
+    CPUState *cpu = ENV_GET_CPU(cpu_env);
+    abi_ulong handler;
+    sigset_t set, old_set;
+    target_sigset_t target_old_set;
+    struct target_sigaction *sa;
+    struct sigqueue *q;
+    TaskState *ts = cpu->opaque;
+    struct emulated_sigtable *k = &ts->sigtab[sig - 1];
 
- handle_signal:
     trace_user_handle_signal(cpu_env, sig);
     /* dequeue signal */
     q = k->first;
-- 
1.9.1

[Qemu-devel] [PATCH v2 02/19] linux-user: Move handle_pending_signal() to avoid need for declaration

[Peter Maydell]

Move the handle_pending_signal() function above process_pending_signals()
to avoid the need for a forward declaration. (Whitespace only change.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index a9ac491..5f98c71 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5765,29 +5765,6 @@ long do_rt_sigreturn(CPUArchState *env)
 
 #endif
 
-static void handle_pending_signal(CPUArchState *cpu_env, int sig);
-
-void process_pending_signals(CPUArchState *cpu_env)
-{
-    CPUState *cpu = ENV_GET_CPU(cpu_env);
-    int sig;
-    TaskState *ts = cpu->opaque;
-
-    if (!ts->signal_pending)
-        return;
-
-    /* FIXME: This is not threadsafe.  */
-    for(sig = 1; sig <= TARGET_NSIG; sig++) {
-        if (ts->sigtab[sig - 1].pending) {
-            handle_pending_signal(cpu_env, sig);
-            return;
-        }
-    }
-    /* if no signal is pending, just return */
-    ts->signal_pending = 0;
-    return;
-}
-
 static void handle_pending_signal(CPUArchState *cpu_env, int sig)
 {
     CPUState *cpu = ENV_GET_CPU(cpu_env);
@@ -5876,3 +5853,24 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
     if (q != &k->info)
         free_sigqueue(cpu_env, q);
 }
+
+void process_pending_signals(CPUArchState *cpu_env)
+{
+    CPUState *cpu = ENV_GET_CPU(cpu_env);
+    int sig;
+    TaskState *ts = cpu->opaque;
+
+    if (!ts->signal_pending)
+        return;
+
+    /* FIXME: This is not threadsafe.  */
+    for(sig = 1; sig <= TARGET_NSIG; sig++) {
+        if (ts->sigtab[sig - 1].pending) {
+            handle_pending_signal(cpu_env, sig);
+            return;
+        }
+    }
+    /* if no signal is pending, just return */
+    ts->signal_pending = 0;
+    return;
+}
-- 
1.9.1

[Qemu-devel] [PATCH v2 03/19] linux-user: Fix stray tab-indent

[Peter Maydell]

Fix a stray tab-indented linux in linux-user/signal.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5f98c71..5069c3f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5847,8 +5847,9 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
         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);
-- 
1.9.1

[Qemu-devel] [PATCH v2 04/19] linux-user: Factor out uses of do_sigprocmask() from sigreturn code

[Peter Maydell]

All the architecture specific handlers for sigreturn include calls
to do_sigprocmask(SIGSETMASK, &set, NULL) to set the signal mask
from the uc_sigmask in the context being restored. Factor these
out into calls to a set_sigmask() function. The next patch will
want to add code which is not run when setting the signal mask
via do_sigreturn, and this change allows us to separate the two
cases.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 55 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5069c3f..1b86a85 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -239,6 +239,15 @@ int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
     return ret;
 }
 
+#if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) && \
+    !defined(TARGET_X86_64)
+/* Just set the guest's signal mask to the specified value */
+static void set_sigmask(const sigset_t *set)
+{
+    do_sigprocmask(SIG_SETMASK, set, NULL);
+}
+#endif
+
 /* siginfo conversion */
 
 static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
@@ -1093,7 +1102,7 @@ long do_sigreturn(CPUX86State *env)
     }
 
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     /* restore registers */
     if (restore_sigcontext(env, &frame->sc))
@@ -1118,7 +1127,7 @@ long do_rt_sigreturn(CPUX86State *env)
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
         goto badframe;
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     if (restore_sigcontext(env, &frame->uc.tuc_mcontext)) {
         goto badframe;
@@ -1258,7 +1267,7 @@ static int target_restore_sigframe(CPUARMState *env,
     uint64_t pstate;
 
     target_to_host_sigset(&set, &sf->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     for (i = 0; i < 31; i++) {
         __get_user(env->xregs[i], &sf->uc.tuc_mcontext.regs[i]);
@@ -1900,7 +1909,7 @@ static long do_sigreturn_v1(CPUARMState *env)
     }
 
     target_to_host_sigset_internal(&host_set, &set);
-    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
+    set_sigmask(&host_set);
 
     if (restore_sigcontext(env, &frame->sc)) {
         goto badframe;
@@ -1981,7 +1990,7 @@ static int do_sigframe_return_v2(CPUARMState *env, target_ulong frame_addr,
     abi_ulong *regspace;
 
     target_to_host_sigset(&host_set, &uc->tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
+    set_sigmask(&host_set);
 
     if (restore_sigcontext(env, &uc->tuc_mcontext))
         return 1;
@@ -2077,7 +2086,7 @@ static long do_rt_sigreturn_v1(CPUARMState *env)
     }
 
     target_to_host_sigset(&host_set, &frame->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
+    set_sigmask(&host_set);
 
     if (restore_sigcontext(env, &frame->uc.tuc_mcontext)) {
         goto badframe;
@@ -2453,7 +2462,7 @@ long do_sigreturn(CPUSPARCState *env)
     }
 
     target_to_host_sigset_internal(&host_set, &set);
-    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
+    set_sigmask(&host_set);
 
     if (err) {
         goto segv_and_exit;
@@ -2576,7 +2585,7 @@ void sparc64_set_context(CPUSPARCState *env)
             }
         }
         target_to_host_sigset_internal(&set, &target_set);
-        do_sigprocmask(SIG_SETMASK, &set, NULL);
+        set_sigmask(&set);
     }
     env->pc = pc;
     env->npc = npc;
@@ -2993,7 +3002,7 @@ long do_sigreturn(CPUMIPSState *regs)
     }
 
     target_to_host_sigset_internal(&blocked, &target_set);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
 
     restore_sigcontext(regs, &frame->sf_sc);
 
@@ -3097,7 +3106,7 @@ long do_rt_sigreturn(CPUMIPSState *env)
     }
 
     target_to_host_sigset(&blocked, &frame->rs_uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
 
     restore_sigcontext(env, &frame->rs_uc.tuc_mcontext);
 
@@ -3371,7 +3380,7 @@ long do_sigreturn(CPUSH4State *regs)
         goto badframe;
 
     target_to_host_sigset_internal(&blocked, &target_set);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
 
     restore_sigcontext(regs, &frame->sc);
 
@@ -3397,7 +3406,7 @@ long do_rt_sigreturn(CPUSH4State *regs)
     }
 
     target_to_host_sigset(&blocked, &frame->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
 
     restore_sigcontext(regs, &frame->uc.tuc_mcontext);
 
@@ -3621,7 +3630,7 @@ long do_sigreturn(CPUMBState *env)
         __get_user(target_set.sig[i], &frame->extramask[i - 1]);
     }
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     restore_sigcontext(&frame->uc.tuc_mcontext, env);
     /* We got here through a sigreturn syscall, our path back is via an
@@ -3792,7 +3801,7 @@ long do_sigreturn(CPUCRISState *env)
         __get_user(target_set.sig[i], &frame->extramask[i - 1]);
     }
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     restore_sigcontext(&frame->sc, env);
     unlock_user_struct(frame, frame_addr, 0);
@@ -4284,7 +4293,7 @@ long do_sigreturn(CPUS390XState *env)
     __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
 
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL); /* ~_BLOCKABLE? */
+    set_sigmask(&set); /* ~_BLOCKABLE? */
 
     if (restore_sigregs(env, &frame->sregs)) {
         goto badframe;
@@ -4310,7 +4319,7 @@ long do_rt_sigreturn(CPUS390XState *env)
     }
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
 
-    do_sigprocmask(SIG_SETMASK, &set, NULL); /* ~_BLOCKABLE? */
+    set_sigmask(&set); /* ~_BLOCKABLE? */
 
     if (restore_sigregs(env, &frame->uc.tuc_mcontext)) {
         goto badframe;
@@ -4872,7 +4881,7 @@ long do_sigreturn(CPUPPCState *env)
     __get_user(set.sig[1], &sc->_unused[3]);
 #endif
     target_to_host_sigset_internal(&blocked, &set);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
 
     __get_user(sr_addr, &sc->regs);
     if (!lock_user_struct(VERIFY_READ, sr, sr_addr, 1))
@@ -4913,7 +4922,7 @@ static int do_setcontext(struct target_ucontext *ucp, CPUPPCState *env, int sig)
         return 1;
 
     target_to_host_sigset_internal(&blocked, &set);
-    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
+    set_sigmask(&blocked);
     restore_user_regs(env, mcp, sig);
 
     unlock_user_struct(mcp, mcp_addr, 1);
@@ -5261,7 +5270,7 @@ long do_sigreturn(CPUM68KState *env)
     }
 
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     /* restore registers */
 
@@ -5287,7 +5296,7 @@ long do_rt_sigreturn(CPUM68KState *env)
         goto badframe;
 
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     /* restore registers */
 
@@ -5530,7 +5539,7 @@ long do_sigreturn(CPUAlphaState *env)
     __get_user(target_set.sig[0], &sc->sc_mask);
 
     target_to_host_sigset_internal(&set, &target_set);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     restore_sigcontext(env, sc);
     unlock_user_struct(sc, sc_addr, 0);
@@ -5551,7 +5560,7 @@ long do_rt_sigreturn(CPUAlphaState *env)
         goto badframe;
     }
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     restore_sigcontext(env, &frame->uc.tuc_mcontext);
     if (do_sigaltstack(frame_addr + offsetof(struct target_rt_sigframe,
@@ -5718,7 +5727,7 @@ long do_rt_sigreturn(CPUTLGState *env)
         goto badframe;
     }
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
-    do_sigprocmask(SIG_SETMASK, &set, NULL);
+    set_sigmask(&set);
 
     restore_sigcontext(env, &frame->uc.tuc_mcontext);
     if (do_sigaltstack(frame_addr + offsetof(struct target_rt_sigframe,
-- 
1.9.1

[Qemu-devel] [PATCH v2 05/19] linux-user: Define macro for size of host kernel sigset_t

[Peter Maydell]

Some host syscalls take an argument specifying the size of a
host kernel's sigset_t (which isn't necessarily the same as
that of the host libc's type of that name). Instead of hardcoding
_NSIG / 8 where we do this, define and use a SIGSET_T_SIZE macro.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index df70255..e4b7404 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -119,6 +119,10 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #define	VFAT_IOCTL_READDIR_BOTH		_IOR('r', 1, struct linux_dirent [2])
 #define	VFAT_IOCTL_READDIR_SHORT	_IOR('r', 2, struct linux_dirent [2])
 
+/* This is the size of the host kernel's sigset_t, needed where we make
+ * direct system calls that take a sigset_t pointer and a size.
+ */
+#define SIGSET_T_SIZE (_NSIG / 8)
 
 #undef _syscall0
 #undef _syscall1
@@ -7221,7 +7225,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             /* Extract the two packed args for the sigset */
             if (arg6) {
                 sig_ptr = &sig;
-                sig.size = _NSIG / 8;
+                sig.size = SIGSET_T_SIZE;
 
                 arg7 = lock_user(VERIFY_READ, arg6, sizeof(*arg7) * 2, 1);
                 if (!arg7) {
@@ -8275,7 +8279,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                     set = NULL;
                 }
 
-                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts, set, _NSIG/8));
+                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts,
+                                          set, SIGSET_T_SIZE));
 
                 if (!is_error(ret) && arg3) {
                     host_to_target_timespec(arg3, timeout_ts);
-- 
1.9.1

[Qemu-devel] [PATCH v2 06/19] linux-user: Use safe_syscall for sigsuspend syscalls

[Peter Maydell]

Use the safe_syscall wrapper for sigsuspend syscalls. This
means that we will definitely deliver a signal that arrives
before we do the sigsuspend call, rather than blocking first
and delivering afterwards.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e4b7404..083f26f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -703,6 +703,7 @@ safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
               fd_set *, exceptfds, struct timespec *, timeout, void *, sig)
 safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
               const struct timespec *,timeout,int *,uaddr2,int,val3)
+safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
 
 static inline int host_to_target_sock_type(int host_type)
 {
@@ -7007,7 +7008,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             target_to_host_old_sigset(&set, p);
             unlock_user(p, arg1, 0);
 #endif
-            ret = get_errno(sigsuspend(&set));
+            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
         }
         break;
 #endif
@@ -7018,7 +7019,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 goto efault;
             target_to_host_sigset(&set, p);
             unlock_user(p, arg1, 0);
-            ret = get_errno(sigsuspend(&set));
+            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
         }
         break;
     case TARGET_NR_rt_sigtimedwait:
-- 
1.9.1

[Qemu-devel] [PATCH v2 07/19] linux-user: Fix race between multiple signals

[Peter Maydell]

If multiple host signals are received in quick succession they would
be queued in TaskState then delivered to the guest in spite of
signals being supposed to be blocked by the guest signal handler's
sa_mask. Fix this by decoupling the guest signal mask from the
host signal mask, so we can have protected sections where all
host signals are blocked. In particular we block signals from
when host_signal_handler() queues a signal from the guest until
process_pending_signals() has unqueued it. We also block signals
while we are manipulating the guest signal mask in emulation of
sigprocmask and similar syscalls.

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 that would otherwise happen if host_signal_handler()
ran and accessed the signal data structures while process_pending_signals()
was manipulating them.

Since we now track the guest signal mask separately from that
of the host, the sigsuspend system calls must track the signal
mask passed to them, because when we process signals as we leave
the sigsuspend the guest signal mask in force is that passed to
sigsuspend.

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-19-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
[PMM: make signal_pending a simple flag rather than a word with two flag bits;
 ensure we don't call block_signals() twice in sigreturn codepaths;
 document and assert() the guarantee that using do_sigprocmask() to
 get the current mask never fails;  use the qemu atomics.h functions
 rather than raw volatile variable access; add extra commentary and
 documentation; block SIGSEGV/SIGBUS in block_signals() and in
 process_pending_signals() because they can't occur synchronously here;
 check the right do_sigprocmask() call for errors in ssetmask syscall;
 expand commit message; fixed sigsuspend() hanging]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/qemu.h    |  50 +++++++++++++++-
 linux-user/signal.c  | 163 +++++++++++++++++++++++++++++++++++----------------
 linux-user/syscall.c |  73 ++++++++++++++++-------
 3 files changed, 213 insertions(+), 73 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index f09b750..5138289 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -123,14 +123,33 @@ 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 */
+    /* This thread's signal mask, as requested by the guest program.
+     * The actual signal mask of this thread may differ:
+     *  + we don't let SIGSEGV and SIGBUS be blocked while running guest code
+     *  + sometimes we block all signals to avoid races
+     */
+    sigset_t signal_mask;
+    /* The signal mask imposed by a guest sigsuspend syscall, if we are
+     * currently in the middle of such a syscall
+     */
+    sigset_t sigsuspend_mask;
+    /* Nonzero if we're leaving a sigsuspend and sigsuspend_mask is valid. */
+    int in_sigsuspend;
+
+    /* Nonzero if process_pending_signals() needs to do something (either
+     * handle a pending signal or unblock signals).
+     * This flag is written from a signal handler so should be accessed via
+     * the atomic_read() and atomic_write() functions. (It is not accessed
+     * from multiple threads.)
+     */
+    int signal_pending;
+
 } __attribute__((aligned(16))) TaskState;
 
 extern char *exec_path;
@@ -235,6 +254,12 @@ unsigned long init_guest_space(unsigned long host_start,
  * It's also OK to implement these with safe_syscall, though it will be
  * a little less efficient if a signal is delivered at the 'wrong' moment.
  *
+ * Some non-interruptible syscalls need to be handled using block_signals()
+ * to block signals for the duration of the syscall. This mainly applies
+ * to code which needs to modify the data structures used by the
+ * host_signal_handler() function and the functions it calls, including
+ * all syscalls which change the thread's signal mask.
+ *
  * (2) Interruptible syscalls
  *
  * These are guest syscalls that can be interrupted by signals and
@@ -266,6 +291,8 @@ unsigned long init_guest_space(unsigned long host_start,
  * you make in the implementation returns either -TARGET_ERESTARTSYS or
  * EINTR though.)
  *
+ * block_signals() cannot be used for interruptible syscalls.
+ *
  *
  * How and why the safe_syscall implementation works:
  *
@@ -352,6 +379,25 @@ 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);
+/**
+ * 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 */
 
 #ifdef TARGET_I386
 /* vm86.c */
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 1b86a85..a89853d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -190,61 +190,81 @@ 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;
+
+    /* 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);
+
+    pending = atomic_xchg(&ts->signal_pending, 1);
+
+    return pending;
+}
+
 /* 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. Returns -TARGET_ERESTARTSYS if
+ * a signal was already pending and the syscall must be restarted, or
+ * 0 on success.
+ * If set is NULL, this is guaranteed not to fail.
  */
 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 = 1; 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);
+        /* Silently ignore attempts to change blocking status of KILL or STOP */
+        sigdelset(&ts->signal_mask, SIGKILL);
+        sigdelset(&ts->signal_mask, SIGSTOP);
     }
-
-    return ret;
+    return 0;
 }
 
 #if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) && \
     !defined(TARGET_X86_64)
-/* Just set the guest's signal mask to the specified value */
+/* Just set the guest's signal mask to the specified value; the
+ * caller is assumed to have called block_signals() already.
+ */
 static void set_sigmask(const sigset_t *set)
 {
-    do_sigprocmask(SIG_SETMASK, set, NULL);
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+    ts->signal_mask = *set;
 }
 #endif
 
@@ -376,6 +396,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;
@@ -391,6 +412,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));
@@ -509,7 +533,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
@@ -565,7 +589,7 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
         q->next = NULL;
         k->pending = 1;
         /* signal that a new signal is pending */
-        ts->signal_pending = 1;
+        atomic_set(&ts->signal_pending, 1);
         return 1; /* indicates that the signal was queued */
     }
 }
@@ -583,6 +607,7 @@ static void host_signal_handler(int host_signum, siginfo_t *info,
     CPUArchState *env = thread_cpu->env_ptr;
     int sig;
     target_siginfo_t tinfo;
+    ucontext_t *uc = puc;
 
     /* the CPU emulator uses some host signals to detect exceptions,
        we forward to it some signals */
@@ -602,6 +627,16 @@ 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. We
+         * can't block SIGSEGV or SIGBUS while we're executing guest
+         * code in case the guest code provokes one in the window between
+         * now and it getting out to the main loop. Signals will be
+         * unblocked again in process_pending_signals().
+         */
+        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);
     }
@@ -2673,9 +2708,13 @@ void sparc64_get_context(CPUSPARCState *env)
     env->pc = env->npc;
     env->npc += 4;
 
-    err = 0;
-
-    do_sigprocmask(0, NULL, &set);
+    /* If we're only reading the signal mask then do_sigprocmask()
+     * is guaranteed not to fail, which is important because we don't
+     * have any way to signal a failure or restart this operation since
+     * this is not a normal syscall.
+     */
+    err = do_sigprocmask(0, NULL, &set);
+    assert(err == 0);
     host_to_target_sigset_internal(&target_set, &set);
     if (TARGET_NSIG_WORDS == 1) {
         __put_user(target_set.sig[0],
@@ -5778,7 +5817,7 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
 {
     CPUState *cpu = ENV_GET_CPU(cpu_env);
     abi_ulong handler;
-    sigset_t set, old_set;
+    sigset_t set;
     target_sigset_t target_old_set;
     struct target_sigaction *sa;
     struct sigqueue *q;
@@ -5801,7 +5840,7 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
         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.
@@ -5825,17 +5864,23 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
         force_sig(sig);
     } else {
         /* compute the blocked signals during the handler execution */
+        sigset_t *blocked_set;
+
         target_to_host_sigset(&set, &sa->sa_mask);
         /* SA_NODEFER indicates that the current signal should not be
            blocked during the handler */
         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 */
+        blocked_set = ts->in_sigsuspend ?
+            &ts->sigsuspend_mask : &ts->signal_mask;
+        sigorset(&ts->signal_mask, blocked_set, &set);
+        ts->in_sigsuspend = 0;
 
         /* if the CPU is in VM86 mode, we restore the 32 bit values */
 #if defined(TARGET_I386) && !defined(TARGET_X86_64)
@@ -5869,18 +5914,38 @@ void process_pending_signals(CPUArchState *cpu_env)
     CPUState *cpu = ENV_GET_CPU(cpu_env);
     int sig;
     TaskState *ts = cpu->opaque;
+    sigset_t set;
+    sigset_t *blocked_set;
 
-    if (!ts->signal_pending)
-        return;
-
-    /* FIXME: This is not threadsafe.  */
-    for(sig = 1; sig <= TARGET_NSIG; sig++) {
-        if (ts->sigtab[sig - 1].pending) {
-            handle_pending_signal(cpu_env, sig);
-            return;
+    while (atomic_read(&ts->signal_pending)) {
+        /* FIXME: This is not threadsafe.  */
+        sigfillset(&set);
+        sigprocmask(SIG_SETMASK, &set, 0);
+
+        for (sig = 1; sig <= TARGET_NSIG; sig++) {
+            blocked_set = ts->in_sigsuspend ?
+                &ts->sigsuspend_mask : &ts->signal_mask;
+
+            if (ts->sigtab[sig - 1].pending &&
+                (!sigismember(blocked_set,
+                              target_to_host_signal_table[sig])
+                 || sig == TARGET_SIGSEGV)) {
+                handle_pending_signal(cpu_env, sig);
+                /* Restart scan from the beginning */
+                sig = 1;
+            }
         }
-    }
-    /* if no signal is pending, just return */
-    ts->signal_pending = 0;
-    return;
+
+        /* if no signal is pending, unblock signals and recheck (the act
+         * of unblocking might cause us to take another host signal which
+         * will set signal_pending again).
+         */
+        atomic_set(&ts->signal_pending, 0);
+        ts->in_sigsuspend = 0;
+        set = ts->signal_mask;
+        sigdelset(&set, SIGSEGV);
+        sigdelset(&set, SIGBUS);
+        sigprocmask(SIG_SETMASK, &set, 0);
+    }
+    ts->in_sigsuspend = 0;
 }
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 083f26f..5a34642 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4746,6 +4746,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;
 
@@ -6841,9 +6842,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
@@ -6852,12 +6855,20 @@ 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);
+            /* We only have one word of the new mask so we must read
+             * the rest of it with do_sigprocmask() and OR in this word.
+             * We are guaranteed that a do_sigprocmask() that only queries
+             * the signal mask will not fail.
+             */
+            ret = do_sigprocmask(0, NULL, &cur_set);
+            assert(!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;
+            ret = do_sigprocmask(SIG_SETMASK, &set, &oset);
+            if (!ret) {
+                host_to_target_old_sigset(&target_set, &oset);
+                ret = target_set;
+            }
         }
         break;
 #endif
@@ -6886,7 +6897,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;
@@ -6920,7 +6931,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;
@@ -6960,7 +6971,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;
@@ -6998,28 +7009,36 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef TARGET_NR_sigsuspend
     case TARGET_NR_sigsuspend:
         {
-            sigset_t set;
+            TaskState *ts = cpu->opaque;
 #if defined(TARGET_ALPHA)
             abi_ulong mask = arg1;
-            target_to_host_old_sigset(&set, &mask);
+            target_to_host_old_sigset(&ts->sigsuspend_mask, &mask);
 #else
             if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
                 goto efault;
-            target_to_host_old_sigset(&set, p);
+            target_to_host_old_sigset(&ts->sigsuspend_mask, p);
             unlock_user(p, arg1, 0);
 #endif
-            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
+            ret = get_errno(safe_rt_sigsuspend(&ts->sigsuspend_mask,
+                                               SIGSET_T_SIZE));
+            if (ret != -TARGET_ERESTARTSYS) {
+                ts->in_sigsuspend = 1;
+            }
         }
         break;
 #endif
     case TARGET_NR_rt_sigsuspend:
         {
-            sigset_t set;
+            TaskState *ts = cpu->opaque;
             if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
                 goto efault;
-            target_to_host_sigset(&set, p);
+            target_to_host_sigset(&ts->sigsuspend_mask, p);
             unlock_user(p, arg1, 0);
-            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
+            ret = get_errno(safe_rt_sigsuspend(&ts->sigsuspend_mask,
+                                               SIGSET_T_SIZE));
+            if (ret != -TARGET_ERESTARTSYS) {
+                ts->in_sigsuspend = 1;
+            }
         }
         break;
     case TARGET_NR_rt_sigtimedwait:
@@ -7065,11 +7084,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)))
@@ -9123,9 +9150,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
-- 
1.9.1

[Qemu-devel] [PATCH v2 08/19] linux-user: Remove redundant default action check in queue_signal()

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

Both queue_signal() and process_pending_signals() did check for default
actions of signals, this is redundant and also causes fatal and stopping
signals to incorrectly cause guest system calls to be interrupted.

The code in queue_signal() is removed.

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-21-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index a89853d..2c6790d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -525,46 +525,10 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
     TaskState *ts = cpu->opaque;
     struct emulated_sigtable *k;
     struct sigqueue *q, **pq;
-    abi_ulong handler;
-    int queue;
 
     trace_user_queue_signal(env, sig);
     k = &ts->sigtab[sig - 1];
-    queue = gdb_queuesig ();
-    handler = sigact_table[sig - 1]._sa_handler;
 
-    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
-         * situation is treated as if using the default handler. This is
-         * not correct if some other process has randomly sent us a SIGSEGV
-         * via kill(), but that is not easy to distinguish at this point,
-         * so we assume it doesn't happen.
-         */
-        handler = TARGET_SIG_DFL;
-    }
-
-    if (!queue && handler == TARGET_SIG_DFL) {
-        if (sig == TARGET_SIGTSTP || sig == TARGET_SIGTTIN || sig == TARGET_SIGTTOU) {
-            kill(getpid(),SIGSTOP);
-            return 0;
-        } else
-        /* default handler : ignore some signal. The other are fatal */
-        if (sig != TARGET_SIGCHLD &&
-            sig != TARGET_SIGURG &&
-            sig != TARGET_SIGWINCH &&
-            sig != TARGET_SIGCONT) {
-            force_sig(sig);
-        } else {
-            return 0; /* indicate ignored */
-        }
-    } else if (!queue && handler == TARGET_SIG_IGN) {
-        /* ignore signal */
-        return 0;
-    } else if (!queue && handler == TARGET_SIG_ERR) {
-        force_sig(sig);
-    } else {
         pq = &k->first;
         if (sig < TARGET_SIGRTMIN) {
             /* if non real time signal, we queue exactly one signal */
@@ -591,7 +555,6 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
         /* signal that a new signal is pending */
         atomic_set(&ts->signal_pending, 1);
         return 1; /* indicates that the signal was queued */
-    }
 }
 
 #ifndef HAVE_SAFE_SYSCALL
-- 
1.9.1

[Qemu-devel] [PATCH v2 09/19] linux-user: Remove redundant gdb_queuesig()

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-22-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 gdbstub.c              | 13 -------------
 include/exec/gdbstub.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index b9e3710..ae83028 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1494,19 +1494,6 @@ void gdb_exit(CPUArchState *env, int code)
 
 #ifdef CONFIG_USER_ONLY
 int
-gdb_queuesig (void)
-{
-    GDBState *s;
-
-    s = gdbserver_state;
-
-    if (gdbserver_fd < 0 || s->fd < 0)
-        return 0;
-    else
-        return 1;
-}
-
-int
 gdb_handlesig(CPUState *cpu, int sig)
 {
     GDBState *s;
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 8e3f8d8..f9708bb 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -48,7 +48,6 @@ int use_gdb_syscalls(void);
 void gdb_set_stop_cpu(CPUState *cpu);
 void gdb_exit(CPUArchState *, int);
 #ifdef CONFIG_USER_ONLY
-int gdb_queuesig (void);
 int gdb_handlesig(CPUState *, int);
 void gdb_signalled(CPUArchState *, int);
 void gdbserver_fork(CPUState *);
-- 
1.9.1

[Qemu-devel] [PATCH v2 10/19] linux-user: Remove real-time signal queuing

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

As host signals are now blocked whenever guest signals are blocked, the
queue of realtime signals is now in Linux. The QEMU queue is now
redundant and can be removed. (We already did not queue non-RT signals, and
none of the calls to queue_signal() except the one in host_signal_handler()
pass an RT signal number.)

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-23-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: minor commit message tweak]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c   |  7 ------
 linux-user/qemu.h   | 11 +--------
 linux-user/signal.c | 70 ++++++++++-------------------------------------------
 3 files changed, 14 insertions(+), 74 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index b2bc6ab..b6da0ba 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3794,14 +3794,7 @@ void stop_all_tasks(void)
 /* Assumes contents are already zeroed.  */
 void init_task_state(TaskState *ts)
 {
-    int i;
- 
     ts->used = 1;
-    ts->first_free = ts->sigqueue_table;
-    for (i = 0; i < MAX_SIGQUEUE_SIZE - 1; i++) {
-        ts->sigqueue_table[i].next = &ts->sigqueue_table[i + 1];
-    }
-    ts->sigqueue_table[i].next = NULL;
 }
 
 CPUArchState *cpu_copy(CPUArchState *env)
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 5138289..b201f90 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -78,16 +78,9 @@ struct vm86_saved_state {
 
 #define MAX_SIGQUEUE_SIZE 1024
 
-struct sigqueue {
-    struct sigqueue *next;
-    target_siginfo_t info;
-};
-
 struct emulated_sigtable {
     int pending; /* true if signal is pending */
-    struct sigqueue *first;
-    struct sigqueue info; /* in order to always have memory for the
-                             first signal, we put it here */
+    target_siginfo_t info;
 };
 
 /* NOTE: we force a big alignment so that the stack stored after is
@@ -127,8 +120,6 @@ typedef struct TaskState {
     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 */
     /* This thread's signal mask, as requested by the guest program.
      * The actual signal mask of this thread may differ:
      *  + we don't let SIGSEGV and SIGBUS be blocked while running guest code
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 2c6790d..5db1c0b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -441,27 +441,6 @@ void signal_init(void)
     }
 }
 
-/* signal queue handling */
-
-static inline struct sigqueue *alloc_sigqueue(CPUArchState *env)
-{
-    CPUState *cpu = ENV_GET_CPU(env);
-    TaskState *ts = cpu->opaque;
-    struct sigqueue *q = ts->first_free;
-    if (!q)
-        return NULL;
-    ts->first_free = q->next;
-    return q;
-}
-
-static inline void free_sigqueue(CPUArchState *env, struct sigqueue *q)
-{
-    CPUState *cpu = ENV_GET_CPU(env);
-    TaskState *ts = cpu->opaque;
-
-    q->next = ts->first_free;
-    ts->first_free = q;
-}
 
 /* abort execution with signal */
 static void QEMU_NORETURN force_sig(int target_sig)
@@ -524,37 +503,20 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
     CPUState *cpu = ENV_GET_CPU(env);
     TaskState *ts = cpu->opaque;
     struct emulated_sigtable *k;
-    struct sigqueue *q, **pq;
 
     trace_user_queue_signal(env, sig);
     k = &ts->sigtab[sig - 1];
 
-        pq = &k->first;
-        if (sig < TARGET_SIGRTMIN) {
-            /* if non real time signal, we queue exactly one signal */
-            if (!k->pending)
-                q = &k->info;
-            else
-                return 0;
-        } else {
-            if (!k->pending) {
-                /* first signal */
-                q = &k->info;
-            } else {
-                q = alloc_sigqueue(env);
-                if (!q)
-                    return -EAGAIN;
-                while (*pq != NULL)
-                    pq = &(*pq)->next;
-            }
-        }
-        *pq = q;
-        q->info = *info;
-        q->next = NULL;
-        k->pending = 1;
-        /* signal that a new signal is pending */
-        atomic_set(&ts->signal_pending, 1);
-        return 1; /* indicates that the signal was queued */
+    /* we queue exactly one signal */
+    if (k->pending) {
+        return 0;
+    }
+
+    k->info = *info;
+    k->pending = 1;
+    /* signal that a new signal is pending */
+    atomic_set(&ts->signal_pending, 1);
+    return 1; /* indicates that the signal was queued */
 }
 
 #ifndef HAVE_SAFE_SYSCALL
@@ -5783,16 +5745,12 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
     sigset_t set;
     target_sigset_t target_old_set;
     struct target_sigaction *sa;
-    struct sigqueue *q;
     TaskState *ts = cpu->opaque;
     struct emulated_sigtable *k = &ts->sigtab[sig - 1];
 
     trace_user_handle_signal(cpu_env, sig);
     /* dequeue signal */
-    q = k->first;
-    k->first = q->next;
-    if (!k->first)
-        k->pending = 0;
+    k->pending = 0;
 
     sig = gdb_handlesig(cpu, sig);
     if (!sig) {
@@ -5857,10 +5815,10 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
 #if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64) \
     || defined(TARGET_OPENRISC) || defined(TARGET_TILEGX)
         /* These targets do not have traditional signals.  */
-        setup_rt_frame(sig, sa, &q->info, &target_old_set, cpu_env);
+        setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env);
 #else
         if (sa->sa_flags & TARGET_SA_SIGINFO)
-            setup_rt_frame(sig, sa, &q->info, &target_old_set, cpu_env);
+            setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env);
         else
             setup_frame(sig, sa, &target_old_set, cpu_env);
 #endif
@@ -5868,8 +5826,6 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
             sa->_sa_handler = TARGET_SIG_DFL;
         }
     }
-    if (q != &k->info)
-        free_sigqueue(cpu_env, q);
 }
 
 void process_pending_signals(CPUArchState *cpu_env)
-- 
1.9.1

[Qemu-devel] [PATCH v2 11/19] linux-user: Queue synchronous signals separately

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

If a synchronous signal and an asynchronous signal arrive near simultaneously,
and the signal number of the asynchronous signal is lower than that of the
synchronous signal the the handler for the asynchronous would be called first,
and then the handler for the synchronous signal would be called within or
after the first handler with an incorrect context.

This is fixed by queuing synchronous signals separately. Note that this does
risk delaying a asynchronous signal until the synchronous signal handler
returns rather than handling the signal on another thread, but this seems
unlikely to cause problems for real guest programs and is unavoidable unless
we could guarantee to roll back and reexecute whatever guest instruction
caused the synchronous signal (which would be a bit odd if we've already
logged its execution, for instance, and would require careful analysis of
all guest CPUs to check it was possible in all cases).

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-24-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
[PMM: added a comment]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/qemu.h   |  1 +
 linux-user/signal.c | 74 ++++++++++++++++++++++++++++++-----------------------
 2 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index b201f90..6bd7b32 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -119,6 +119,7 @@ typedef struct TaskState {
     struct image_info *info;
     struct linux_binprm *bprm;
 
+    struct emulated_sigtable sync_signal;
     struct emulated_sigtable sigtab[TARGET_NSIG];
     /* This thread's signal mask, as requested by the guest program.
      * The actual signal mask of this thread may differ:
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5db1c0b..f489028 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -502,18 +502,11 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     TaskState *ts = cpu->opaque;
-    struct emulated_sigtable *k;
 
     trace_user_queue_signal(env, sig);
-    k = &ts->sigtab[sig - 1];
-
-    /* we queue exactly one signal */
-    if (k->pending) {
-        return 0;
-    }
 
-    k->info = *info;
-    k->pending = 1;
+    ts->sync_signal.info = *info;
+    ts->sync_signal.pending = sig;
     /* signal that a new signal is pending */
     atomic_set(&ts->signal_pending, 1);
     return 1; /* indicates that the signal was queued */
@@ -530,9 +523,13 @@ static void host_signal_handler(int host_signum, siginfo_t *info,
                                 void *puc)
 {
     CPUArchState *env = thread_cpu->env_ptr;
+    CPUState *cpu = ENV_GET_CPU(env);
+    TaskState *ts = cpu->opaque;
+
     int sig;
     target_siginfo_t tinfo;
     ucontext_t *uc = puc;
+    struct emulated_sigtable *k;
 
     /* the CPU emulator uses some host signals to detect exceptions,
        we forward to it some signals */
@@ -551,20 +548,23 @@ static void host_signal_handler(int host_signum, siginfo_t *info,
     rewind_if_in_safe_syscall(puc);
 
     host_to_target_siginfo_noswap(&tinfo, info);
-    if (queue_signal(env, sig, &tinfo) == 1) {
-        /* Block host signals until target signal handler entered. We
-         * can't block SIGSEGV or SIGBUS while we're executing guest
-         * code in case the guest code provokes one in the window between
-         * now and it getting out to the main loop. Signals will be
-         * unblocked again in process_pending_signals().
-         */
-        sigfillset(&uc->uc_sigmask);
-        sigdelset(&uc->uc_sigmask, SIGSEGV);
-        sigdelset(&uc->uc_sigmask, SIGBUS);
+    k = &ts->sigtab[sig - 1];
+    k->info = tinfo;
+    k->pending = sig;
+    ts->signal_pending = 1;
+
+    /* Block host signals until target signal handler entered. We
+     * can't block SIGSEGV or SIGBUS while we're executing guest
+     * code in case the guest code provokes one in the window between
+     * now and it getting out to the main loop. Signals will be
+     * unblocked again in process_pending_signals().
+     */
+    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);
-    }
+    /* interrupt the virtual CPU as soon as possible */
+    cpu_exit(thread_cpu);
 }
 
 /* do_sigaltstack() returns target values and errnos. */
@@ -5761,14 +5761,6 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
         handler = sa->_sa_handler;
     }
 
-    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.
-         */
-        handler = TARGET_SIG_DFL;
-    }
-
     if (handler == TARGET_SIG_DFL) {
         /* default handler : ignore some signal. The other are job control or fatal */
         if (sig == TARGET_SIGTSTP || sig == TARGET_SIGTTIN || sig == TARGET_SIGTTOU) {
@@ -5841,14 +5833,32 @@ void process_pending_signals(CPUArchState *cpu_env)
         sigfillset(&set);
         sigprocmask(SIG_SETMASK, &set, 0);
 
+        sig = ts->sync_signal.pending;
+        if (sig) {
+            /* Synchronous signals are forced,
+             * see force_sig_info() and callers in Linux
+             * Note that not all of our queue_signal() calls in QEMU correspond
+             * to force_sig_info() calls in Linux (some are send_sig_info()).
+             * However it seems like a kernel bug to me to allow the process
+             * to block a synchronous signal since it could then just end up
+             * looping round and round indefinitely.
+             */
+            if (sigismember(&ts->signal_mask, target_to_host_signal_table[sig])
+                || sigact_table[sig - 1]._sa_handler == TARGET_SIG_IGN) {
+                sigdelset(&ts->signal_mask, target_to_host_signal_table[sig]);
+                sigact_table[sig - 1]._sa_handler = TARGET_SIG_DFL;
+            }
+
+            handle_pending_signal(cpu_env, sig);
+        }
+
         for (sig = 1; sig <= TARGET_NSIG; sig++) {
             blocked_set = ts->in_sigsuspend ?
                 &ts->sigsuspend_mask : &ts->signal_mask;
 
             if (ts->sigtab[sig - 1].pending &&
                 (!sigismember(blocked_set,
-                              target_to_host_signal_table[sig])
-                 || sig == TARGET_SIGSEGV)) {
+                              target_to_host_signal_table[sig]))) {
                 handle_pending_signal(cpu_env, sig);
                 /* Restart scan from the beginning */
                 sig = 1;
-- 
1.9.1

[Qemu-devel] [PATCH v2 12/19] linux-user: Block signals during sigaction() handling

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

Block signals while emulating sigaction. This is a non-interruptible
syscall, and using block_signals() avoids races where the host
signal handler is invoked and tries to examine the signal handler
data structures while we are updating them.

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-29-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
[PMM: expanded commit message]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index f489028..b21d6bf 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -640,7 +640,7 @@ out:
     return ret;
 }
 
-/* do_sigaction() return host values and errnos */
+/* do_sigaction() return target values and host errnos */
 int do_sigaction(int sig, const struct target_sigaction *act,
                  struct target_sigaction *oact)
 {
@@ -649,8 +649,14 @@ int do_sigaction(int sig, const struct target_sigaction *act,
     int host_sig;
     int ret = 0;
 
-    if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP)
-        return -EINVAL;
+    if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP) {
+        return -TARGET_EINVAL;
+    }
+
+    if (block_signals()) {
+        return -TARGET_ERESTARTSYS;
+    }
+
     k = &sigact_table[sig - 1];
     if (oact) {
         __put_user(k->_sa_handler, &oact->_sa_handler);
-- 
1.9.1

[Qemu-devel] [PATCH v2 13/19] linux-user: pause() should not pause if signal pending

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

Fix races between signal handling and the pause syscall by
reimplementing it using block_signals() and sigsuspend().
(Using safe_syscall(pause) would also work, except that the
pause syscall doesn't exist on all architectures.)

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-28-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
[PMM: tweaked commit message]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5a34642..3fc9c8a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6418,7 +6418,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_NR_pause /* not on alpha */
     case TARGET_NR_pause:
-        ret = get_errno(pause());
+        if (!block_signals()) {
+            sigsuspend(&((TaskState *)cpu->opaque)->signal_mask);
+        }
+        ret = -TARGET_EINTR;
         break;
 #endif
 #ifdef TARGET_NR_utime
-- 
1.9.1

[Qemu-devel] [PATCH v2 14/19] linux-user: Restart exit() if signal pending

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

Without this a signal could vanish on thread exit.

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-26-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3fc9c8a..cb5d519 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5999,8 +5999,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
            However in threaded applictions it is used for thread termination,
            and _exit_group is used for application termination.
            Do thread termination if we have more then one thread.  */
-        /* FIXME: This probably breaks if a signal arrives.  We should probably
-           be disabling signals.  */
+
+        if (block_signals()) {
+            ret = -TARGET_ERESTARTSYS;
+            break;
+        }
+
         if (CPU_NEXT(first_cpu)) {
             TaskState *ts;
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 15/19] linux-user: Use safe_syscall for kill, tkill and tgkill syscalls

[Peter Maydell]

Use the safe_syscall wrapper for the kill, tkill and tgkill syscalls.
Without this, if a thread sent a SIGKILL to itself it could kill the
thread before we had a chance to process a signal that arrived just
before the SIGKILL, and that signal would get lost.

We drop all the ifdeffery for tkill and tgkill, because every guest
architecture we support implements them, and they've been in Linux
since 2003 so we can assume the host headers define the __NR_tkill
and __NR_tgkill constants.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Timothy's patchset used block_signals() for this, but I
think safe_syscall is nicer when it can be used, since
it's lower overhead and less intrusive. Also extended to
cover all the thread-killing syscalls rather than just kill.
---
 linux-user/syscall.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index cb5d519..549f571 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -186,8 +186,6 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 arg4,type5 arg5,	\
 #define __NR_sys_getpriority __NR_getpriority
 #define __NR_sys_rt_sigqueueinfo __NR_rt_sigqueueinfo
 #define __NR_sys_syslog __NR_syslog
-#define __NR_sys_tgkill __NR_tgkill
-#define __NR_sys_tkill __NR_tkill
 #define __NR_sys_futex __NR_futex
 #define __NR_sys_inotify_init __NR_inotify_init
 #define __NR_sys_inotify_add_watch __NR_inotify_add_watch
@@ -225,12 +223,6 @@ _syscall5(int, _llseek,  uint,  fd, ulong, hi, ulong, lo,
 #endif
 _syscall3(int,sys_rt_sigqueueinfo,int,pid,int,sig,siginfo_t *,uinfo)
 _syscall3(int,sys_syslog,int,type,char*,bufp,int,len)
-#if defined(TARGET_NR_tgkill) && defined(__NR_tgkill)
-_syscall3(int,sys_tgkill,int,tgid,int,pid,int,sig)
-#endif
-#if defined(TARGET_NR_tkill) && defined(__NR_tkill)
-_syscall2(int,sys_tkill,int,tid,int,sig)
-#endif
 #ifdef __NR_exit_group
 _syscall1(int,exit_group,int,error_code)
 #endif
@@ -704,6 +696,9 @@ safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
 safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
               const struct timespec *,timeout,int *,uaddr2,int,val3)
 safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
+safe_syscall2(int, kill, pid_t, pid, int, sig)
+safe_syscall2(int, tkill, int, tid, int, sig)
+safe_syscall3(int, tgkill, int, tgid, int, pid, int, sig)
 
 static inline int host_to_target_sock_type(int host_type)
 {
@@ -6528,7 +6523,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = 0;
         break;
     case TARGET_NR_kill:
-        ret = get_errno(kill(arg1, target_to_host_signal(arg2)));
+        ret = get_errno(safe_kill(arg1, target_to_host_signal(arg2)));
         break;
 #ifdef TARGET_NR_rename
     case TARGET_NR_rename:
@@ -9764,18 +9759,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
 
-#if defined(TARGET_NR_tkill) && defined(__NR_tkill)
     case TARGET_NR_tkill:
-        ret = get_errno(sys_tkill((int)arg1, target_to_host_signal(arg2)));
+        ret = get_errno(safe_tkill((int)arg1, target_to_host_signal(arg2)));
         break;
-#endif
 
-#if defined(TARGET_NR_tgkill) && defined(__NR_tgkill)
     case TARGET_NR_tgkill:
-	ret = get_errno(sys_tgkill((int)arg1, (int)arg2,
+        ret = get_errno(safe_tgkill((int)arg1, (int)arg2,
                         target_to_host_signal(arg3)));
-	break;
-#endif
+        break;
 
 #ifdef TARGET_NR_set_robust_list
     case TARGET_NR_set_robust_list:
-- 
1.9.1

[Qemu-devel] [PATCH v2 16/19] linux-user: Restart fork() if signals pending

[Peter Maydell]

From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

If there is a signal pending during fork() the signal handler will
erroneously be called in both the parent and child, so handle any
pending signals first.

Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Message-id: 1441497448-32489-20-git-send-email-T.E.Baldwin99@members.leeds.ac.uk
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 549f571..7d5f123 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4796,6 +4796,11 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0) {
             return -TARGET_EINVAL;
         }
+
+        if (block_signals()) {
+            return -TARGET_ERESTARTSYS;
+        }
+
         fork_start();
         ret = fork();
         if (ret == 0) {
-- 
1.9.1

[Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Peter Maydell]

The siginfo_t struct includes a union. The correct way to identify
which fields of the union are relevant is complicated, because we
have to use a combination of the si_code and si_signo to figure out
which of the union's members are valid.  (Within the host kernel it
is always possible to tell, but the kernel carefully avoids giving
userspace the high 16 bits of si_code, so we don't have the
information to do this the easy way...) We therefore make our best
guess, bearing in mind that a guest can spoof most of the si_codes
via rt_sigqueueinfo() if it likes.  Once we have made our guess, we
record it in the top 16 bits of the si_code, so that tswap_siginfo()
later can use it.  tswap_siginfo() then strips these top bits out
before writing si_code to the guest (sign-extending the lower bits).

This fixes a bug where fields were sometimes wrong; in particular
the LTP kill10 test went into an infinite loop because its signal
handler got a si_pid value of 0 rather than the pid of the sending
process.

As part of this change, we switch to using __put_user() in the
tswap_siginfo code which writes out the byteswapped values to
the target memory, in case the target memory pointer is not
sufficiently aligned for the host CPU's requirements.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c       | 165 ++++++++++++++++++++++++++++++++--------------
 linux-user/syscall_defs.h |  15 +++++
 2 files changed, 131 insertions(+), 49 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index b21d6bf..8ea0cbf 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -17,6 +17,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/bitops.h"
 #include <sys/ucontext.h>
 #include <sys/resource.h>
 
@@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
                                                  const siginfo_t *info)
 {
     int sig = host_to_target_signal(info->si_signo);
+    int si_code = info->si_code;
+    int si_type;
     tinfo->si_signo = sig;
     tinfo->si_errno = 0;
     tinfo->si_code = info->si_code;
 
-    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
-            || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
-        /* Should never come here, but who knows. The information for
-           the target is irrelevant.  */
-        tinfo->_sifields._sigfault._addr = 0;
-    } else if (sig == TARGET_SIGIO) {
-        tinfo->_sifields._sigpoll._band = info->si_band;
-        tinfo->_sifields._sigpoll._fd = info->si_fd;
-    } else if (sig == TARGET_SIGCHLD) {
-        tinfo->_sifields._sigchld._pid = info->si_pid;
-        tinfo->_sifields._sigchld._uid = info->si_uid;
-        tinfo->_sifields._sigchld._status
+    /* This is awkward, because we have to use a combination of
+     * the si_code and si_signo to figure out which of the union's
+     * members are valid. (Within the host kernel it is always possible
+     * to tell, but the kernel carefully avoids giving userspace the
+     * high 16 bits of si_code, so we don't have the information to
+     * do this the easy way...) We therefore make our best guess,
+     * bearing in mind that a guest can spoof most of the si_codes
+     * via rt_sigqueueinfo() if it likes.
+     *
+     * Once we have made our guess, we record it in the top 16 bits of
+     * the si_code, so that tswap_siginfo() later can use it.
+     * tswap_siginfo() will strip these top bits out before writing
+     * si_code to the guest (sign-extending the lower bits).
+     */
+
+    switch (si_code) {
+    case SI_USER:
+    case SI_TKILL:
+    case SI_KERNEL:
+        /* Sent via kill(), tkill() or tgkill(), or direct from the kernel.
+         * These are the only unspoofable si_code values.
+         */
+        tinfo->_sifields._kill._pid = info->si_pid;
+        tinfo->_sifields._kill._uid = info->si_uid;
+        si_type = QEMU_SI_KILL;
+        break;
+    default:
+        /* Everything else is spoofable. Make best guess based on signal */
+        switch (sig) {
+        case TARGET_SIGCHLD:
+            tinfo->_sifields._sigchld._pid = info->si_pid;
+            tinfo->_sifields._sigchld._uid = info->si_uid;
+            tinfo->_sifields._sigchld._status
                 = host_to_target_waitstatus(info->si_status);
-        tinfo->_sifields._sigchld._utime = info->si_utime;
-        tinfo->_sifields._sigchld._stime = info->si_stime;
-    } else if (sig >= TARGET_SIGRTMIN) {
-        tinfo->_sifields._rt._pid = info->si_pid;
-        tinfo->_sifields._rt._uid = info->si_uid;
-        /* XXX: potential problem if 64 bit */
-        tinfo->_sifields._rt._sigval.sival_ptr
+            tinfo->_sifields._sigchld._utime = info->si_utime;
+            tinfo->_sifields._sigchld._stime = info->si_stime;
+            si_type = QEMU_SI_CHLD;
+            break;
+        case TARGET_SIGIO:
+            tinfo->_sifields._sigpoll._band = info->si_band;
+            tinfo->_sifields._sigpoll._fd = info->si_fd;
+            si_type = QEMU_SI_POLL;
+            break;
+        default:
+            /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source. */
+            tinfo->_sifields._rt._pid = info->si_pid;
+            tinfo->_sifields._rt._uid = info->si_uid;
+            /* XXX: potential problem if 64 bit */
+            tinfo->_sifields._rt._sigval.sival_ptr
                 = (abi_ulong)(unsigned long)info->si_value.sival_ptr;
+            si_type = QEMU_SI_RT;
+            break;
+        }
+        break;
     }
+
+    tinfo->si_code = deposit32(si_code, 16, 16, si_type);
 }
 
 static void tswap_siginfo(target_siginfo_t *tinfo,
                           const target_siginfo_t *info)
 {
-    int sig = info->si_signo;
-    tinfo->si_signo = tswap32(sig);
-    tinfo->si_errno = tswap32(info->si_errno);
-    tinfo->si_code = tswap32(info->si_code);
-
-    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
-        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
-        tinfo->_sifields._sigfault._addr
-            = tswapal(info->_sifields._sigfault._addr);
-    } else if (sig == TARGET_SIGIO) {
-        tinfo->_sifields._sigpoll._band
-            = tswap32(info->_sifields._sigpoll._band);
-        tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
-    } else if (sig == TARGET_SIGCHLD) {
-        tinfo->_sifields._sigchld._pid
-            = tswap32(info->_sifields._sigchld._pid);
-        tinfo->_sifields._sigchld._uid
-            = tswap32(info->_sifields._sigchld._uid);
-        tinfo->_sifields._sigchld._status
-            = tswap32(info->_sifields._sigchld._status);
-        tinfo->_sifields._sigchld._utime
-            = tswapal(info->_sifields._sigchld._utime);
-        tinfo->_sifields._sigchld._stime
-            = tswapal(info->_sifields._sigchld._stime);
-    } else if (sig >= TARGET_SIGRTMIN) {
-        tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
-        tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
-        tinfo->_sifields._rt._sigval.sival_ptr
-            = tswapal(info->_sifields._rt._sigval.sival_ptr);
+    int si_type = extract32(info->si_code, 16, 16);
+    int si_code = sextract32(info->si_code, 0, 16);
+
+    __put_user(info->si_signo, &tinfo->si_signo);
+    __put_user(info->si_errno, &tinfo->si_errno);
+    __put_user(si_code, &tinfo->si_code);
+
+    /* We can use our internal marker of which fields in the structure
+     * are valid, rather than duplicating the guesswork of
+     * host_to_target_siginfo_noswap() here.
+     */
+    switch (si_type) {
+    case QEMU_SI_KILL:
+        __put_user(info->_sifields._kill._pid, &tinfo->_sifields._kill._pid);
+        __put_user(info->_sifields._kill._uid, &tinfo->_sifields._kill._uid);
+        break;
+    case QEMU_SI_TIMER:
+        __put_user(info->_sifields._timer._timer1,
+                   &tinfo->_sifields._timer._timer1);
+        __put_user(info->_sifields._timer._timer2,
+                   &tinfo->_sifields._timer._timer2);
+        break;
+    case QEMU_SI_POLL:
+        __put_user(info->_sifields._sigpoll._band,
+                   &tinfo->_sifields._sigpoll._band);
+        __put_user(info->_sifields._sigpoll._fd,
+                   &tinfo->_sifields._sigpoll._fd);
+        break;
+    case QEMU_SI_FAULT:
+        __put_user(info->_sifields._sigfault._addr,
+                   &tinfo->_sifields._sigfault._addr);
+        break;
+    case QEMU_SI_CHLD:
+        __put_user(info->_sifields._sigchld._pid,
+                   &tinfo->_sifields._sigchld._pid);
+        __put_user(info->_sifields._sigchld._uid,
+                   &tinfo->_sifields._sigchld._uid);
+        __put_user(info->_sifields._sigchld._status,
+                   &tinfo->_sifields._sigchld._status);
+        __put_user(info->_sifields._sigchld._utime,
+                   &tinfo->_sifields._sigchld._utime);
+        __put_user(info->_sifields._sigchld._stime,
+                   &tinfo->_sifields._sigchld._stime);
+        break;
+    case QEMU_SI_RT:
+        __put_user(info->_sifields._rt._pid, &tinfo->_sifields._rt._pid);
+        __put_user(info->_sifields._rt._uid, &tinfo->_sifields._rt._uid);
+        __put_user(info->_sifields._rt._sigval.sival_ptr,
+                   &tinfo->_sifields._rt._sigval.sival_ptr);
+        break;
+    default:
+        g_assert_not_reached();
     }
 }
 
-
 void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
 {
     host_to_target_siginfo_noswap(tinfo, info);
@@ -505,6 +565,13 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
 
     trace_user_queue_signal(env, sig);
 
+    /* Currently all callers define siginfo structures which
+     * use the _sifields._sigfault union member, so we can
+     * set the type here. If that changes we should push this
+     * out so the si_type is passed in by callers.
+     */
+    info->si_code = deposit32(info->si_code, 16, 16, QEMU_SI_FAULT);
+
     ts->sync_signal.info = *info;
     ts->sync_signal.pending = sig;
     /* signal that a new signal is pending */
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 34af15a..124754f 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -673,6 +673,21 @@ typedef struct {
 
 #define TARGET_SI_PAD_SIZE ((TARGET_SI_MAX_SIZE - TARGET_SI_PREAMBLE_SIZE) / sizeof(int))
 
+/* Within QEMU the top 16 bits of si_code indicate which of the parts of
+ * the union in target_siginfo is valid. This only applies between
+ * host_to_target_siginfo_noswap() and tswap_siginfo(); it does not
+ * appear either within host siginfo_t or in target_siginfo structures
+ * which we get from the guest userspace program. (The Linux kernel
+ * does a similar thing with using the top bits for its own internal
+ * purposes but not letting them be visible to userspace.)
+ */
+#define QEMU_SI_KILL 0
+#define QEMU_SI_TIMER 1
+#define QEMU_SI_POLL 2
+#define QEMU_SI_FAULT 3
+#define QEMU_SI_CHLD 4
+#define QEMU_SI_RT 5
+
 typedef struct target_siginfo {
 #ifdef TARGET_MIPS
 	int si_signo;
-- 
1.9.1

[Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

[Peter Maydell]

host_to_target_siginfo() is implemented by a combination of
host_to_target_siginfo_noswap() followed by tswap_siginfo().
The first of these two functions assumes that the target_siginfo_t
it is writing to is correctly aligned, but the pointer passed
into host_to_target_siginfo() is directly from the guest and
might be misaligned. Use a local variable to avoid this problem.
(tswap_siginfo() does now correctly handle a misaligned destination.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 8ea0cbf..7e2a80f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -400,8 +400,9 @@ static void tswap_siginfo(target_siginfo_t *tinfo,
 
 void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
 {
-    host_to_target_siginfo_noswap(tinfo, info);
-    tswap_siginfo(tinfo, tinfo);
+    target_siginfo_t tgt_tmp;
+    host_to_target_siginfo_noswap(&tgt_tmp, info);
+    tswap_siginfo(tinfo, &tgt_tmp);
 }
 
 /* XXX: we support only POSIX RT signals are used. */
-- 
1.9.1

[Qemu-devel] [PATCH v2 19/19] linux-user: Avoid possible misalignment in target_to_host_siginfo()

[Peter Maydell]

Reimplement target_to_host_siginfo() to use __get_user(), which
handles possibly misaligned source guest structures correctly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7e2a80f..8417da7 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -409,13 +409,18 @@ void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
 /* XXX: find a solution for 64 bit (additional malloced data is needed) */
 void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo)
 {
-    info->si_signo = tswap32(tinfo->si_signo);
-    info->si_errno = tswap32(tinfo->si_errno);
-    info->si_code = tswap32(tinfo->si_code);
-    info->si_pid = tswap32(tinfo->_sifields._rt._pid);
-    info->si_uid = tswap32(tinfo->_sifields._rt._uid);
-    info->si_value.sival_ptr =
-            (void *)(long)tswapal(tinfo->_sifields._rt._sigval.sival_ptr);
+    /* This conversion is used only for the rt_sigqueueinfo syscall,
+     * and so we know that the _rt fields are the valid ones.
+     */
+    abi_ulong sival_ptr;
+
+    __get_user(info->si_signo, &tinfo->si_signo);
+    __get_user(info->si_errno, &tinfo->si_errno);
+    __get_user(info->si_code, &tinfo->si_code);
+    __get_user(info->si_pid, &tinfo->_sifields._rt._pid);
+    __get_user(info->si_uid, &tinfo->_sifields._rt._uid);
+    __get_user(sival_ptr, &tinfo->_sifields._rt._sigval.sival_ptr);
+    info->si_value.sival_ptr = (void *)(long)sival_ptr;
 }
 
 static int fatal_signal (int sig)
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 00/19] linux-user: fix various signal race conditions

[Peter Maydell]

Ping?

thanks
-- PMM

On 27 May 2016 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset overhauls the linux-user signal handling code to
> fix a number of race conditions. It is essentially a v2 of
> Timothy Baldwin's original patchset, though I have addressed
> code review issues, refactored it a little, fixed the occasional
> minor bug and added some patches of my own for other issues I
> spotted along the way.
>
> The meat of the patchset is splitting out the guest thread's idea
> of its signal mask from the host thread's actual signal mask. This
> allows us to temporarily block all host signals as a method for
> fixing some races:
>
>  * block signals in host signal handler until we have processed
>    the signal queue to deliver the guest signal (fixes a race
>    where a second host signal could arrive and we would deliver
>    it even if the guest handler's signal mask should prevent it)
>  * block signals while we are manipulating QEMU data structures which
>    the host signal handler reads (eg in sigaction syscall emulation)
>  * block signals to fix races between signals and noninterruptible
>    syscalls like pause, which we could in theory do with safe_syscall
>    but which would be a pain to do that way because of variations
>    in whether syscalls exist on different host architectures
>  * block signals to fix races for complicated syscalls like fork
>    which would be too painful to handle by trying to roll back
>    if something was interrupted partway through
>
> We also:
>  * remove a lot of code that is made redundant by processing
>    default signal actions in one place rather than two
>  * make sure that synchronous signals correctly take priority
>    over asynchronous signals
>  * use safe_syscall for sigsuspend
>  * use safe_syscall for kill/tkill/tgkill
>  * make a better guess at which bits of the union in siginfo_t
>    need to be converted by looking at si_code as well as si_signo
>  * use __get_user and __put_user for siginfo conversion to avoid
>    potential problems with misaligned guest addresses
>
> Changes since Timothy's v1 patchset:
>  * some patches at the front to factor out handle_pending_signal()
>    to avoid using goto for flow control logic
>  * new function set_sigmask() for setting signal mask when we have
>    already blocked signals -- this allows us to define calling block_signals()
>    twice to be illegal, which then means we can have signal_pending be a
>    simple flag rather than a word with two flag bits in it
>  * use the qemu atomics.h functions rather than raw volatile variable
>    (it makes it clearer that the variable has to be handled with care IMHO)
>  * bunch of extra commentary
>  * add code to stop sigprocmask being able to mark SIGKILL, SIGSTOP as blocked
>  * fixed handling of ssetmask
>  * new patches to better handle conversion of siginfo_t structures
>    (these fix problems with LTP tests like kill10 which try to kill
>    processes by sending them an asynchronous SIGSEGV and expect to
>    see the si_pid field in the resulting siginfo in the recipient.)
>
> With all of these fixes plus the safe_syscall patches now in
> master, the following LTP test cases which used to hang now do not:
>
>  mq_timedreceive01 mq_timedsend01 kill10 kill11 msgrcv03
>  nanosleep04 splice02 waitpid02 inotify06 pselect02 pselect02_64
>
> (Not all of these were signal related issues, and a few might have
> been fixed some time back.)
>
> Next on my todo list after this is to expand the safe_syscall
> support to more host architectures. There are also a few more
> bugs lurking I suspect.
>
> thanks
> -- PMM
>
>
> Peter Maydell (11):
>   linux-user: Factor out handle_signal code from
>     process_pending_signals()
>   linux-user: Move handle_pending_signal() to avoid need for declaration
>   linux-user: Fix stray tab-indent
>   linux-user: Factor out uses of do_sigprocmask() from sigreturn code
>   linux-user: Define macro for size of host kernel sigset_t
>   linux-user: Use safe_syscall for sigsuspend syscalls
>   linux-user: Fix race between multiple signals
>   linux-user: Use safe_syscall for kill, tkill and tgkill syscalls
>   linux-user: Use both si_code and si_signo when converting siginfo_t
>   linux-user: Avoid possible misalignment in host_to_target_siginfo()
>   linux-user: Avoid possible misalignment in target_to_host_siginfo()
>
> Timothy E Baldwin (8):
>   linux-user: Remove redundant default action check in queue_signal()
>   linux-user: Remove redundant gdb_queuesig()
>   linux-user: Remove real-time signal queuing
>   linux-user: Queue synchronous signals separately
>   linux-user: Block signals during sigaction() handling
>   linux-user: pause() should not pause if signal pending
>   linux-user: Restart exit() if signal pending
>   linux-user: Restart fork() if signals pending
>
>  gdbstub.c                 |  13 --
>  include/exec/gdbstub.h    |   1 -
>  linux-user/main.c         |   7 -
>  linux-user/qemu.h         |  62 ++++-
>  linux-user/signal.c       | 572 ++++++++++++++++++++++++++--------------------
>  linux-user/syscall.c      | 124 ++++++----
>  linux-user/syscall_defs.h |  15 ++
>  7 files changed, 476 insertions(+), 318 deletions(-)
>
> --
> 1.9.1

Re: [Qemu-devel] [PATCH v2 01/19] linux-user: Factor out handle_signal code from process_pending_signals()

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Factor out the code to handle a single signal from the
> process_pending_signals() function. The use of goto for flow control
> is OK currently, but would get significantly uglier if extended to
> allow running the handle_signal code multiple times.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/signal.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8090b4d..a9ac491 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5765,33 +5765,40 @@ long do_rt_sigreturn(CPUArchState *env)
>  
>  #endif
>  
> +static void handle_pending_signal(CPUArchState *cpu_env, int sig);
> +
>  void process_pending_signals(CPUArchState *cpu_env)
>  {
>      CPUState *cpu = ENV_GET_CPU(cpu_env);
>      int sig;
> -    abi_ulong handler;
> -    sigset_t set, old_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)
>          return;
>  
>      /* FIXME: This is not threadsafe.  */
> -    k = ts->sigtab;
>      for(sig = 1; sig <= TARGET_NSIG; sig++) {
> -        if (k->pending)
> -            goto handle_signal;
> -        k++;
> +        if (ts->sigtab[sig - 1].pending) {
> +            handle_pending_signal(cpu_env, sig);
> +            return;
> +        }
>      }
>      /* if no signal is pending, just return */
>      ts->signal_pending = 0;
>      return;
> +}
> +
> +static void handle_pending_signal(CPUArchState *cpu_env, int sig)
> +{
> +    CPUState *cpu = ENV_GET_CPU(cpu_env);
> +    abi_ulong handler;
> +    sigset_t set, old_set;
> +    target_sigset_t target_old_set;
> +    struct target_sigaction *sa;
> +    struct sigqueue *q;
> +    TaskState *ts = cpu->opaque;
> +    struct emulated_sigtable *k = &ts->sigtab[sig - 1];
>  
> - handle_signal:
>      trace_user_handle_signal(cpu_env, sig);
>      /* dequeue signal */
>      q = k->first;
> 

Re: [Qemu-devel] [PATCH v2 02/19] linux-user: Move handle_pending_signal() to avoid need for declaration

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Move the handle_pending_signal() function above process_pending_signals()
> to avoid the need for a forward declaration. (Whitespace only change.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/signal.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index a9ac491..5f98c71 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5765,29 +5765,6 @@ long do_rt_sigreturn(CPUArchState *env)
>  
>  #endif
>  
> -static void handle_pending_signal(CPUArchState *cpu_env, int sig);
> -
> -void process_pending_signals(CPUArchState *cpu_env)
> -{
> -    CPUState *cpu = ENV_GET_CPU(cpu_env);
> -    int sig;
> -    TaskState *ts = cpu->opaque;
> -
> -    if (!ts->signal_pending)
> -        return;
> -
> -    /* FIXME: This is not threadsafe.  */
> -    for(sig = 1; sig <= TARGET_NSIG; sig++) {
> -        if (ts->sigtab[sig - 1].pending) {
> -            handle_pending_signal(cpu_env, sig);
> -            return;
> -        }
> -    }
> -    /* if no signal is pending, just return */
> -    ts->signal_pending = 0;
> -    return;
> -}
> -
>  static void handle_pending_signal(CPUArchState *cpu_env, int sig)
>  {
>      CPUState *cpu = ENV_GET_CPU(cpu_env);
> @@ -5876,3 +5853,24 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
>      if (q != &k->info)
>          free_sigqueue(cpu_env, q);
>  }
> +
> +void process_pending_signals(CPUArchState *cpu_env)
> +{
> +    CPUState *cpu = ENV_GET_CPU(cpu_env);
> +    int sig;
> +    TaskState *ts = cpu->opaque;
> +
> +    if (!ts->signal_pending)
> +        return;
> +
> +    /* FIXME: This is not threadsafe.  */
> +    for(sig = 1; sig <= TARGET_NSIG; sig++) {
> +        if (ts->sigtab[sig - 1].pending) {
> +            handle_pending_signal(cpu_env, sig);
> +            return;
> +        }
> +    }
> +    /* if no signal is pending, just return */
> +    ts->signal_pending = 0;
> +    return;
> +}
> 

Re: [Qemu-devel] [PATCH v2 03/19] linux-user: Fix stray tab-indent

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Fix a stray tab-indented linux in linux-user/signal.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/signal.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5f98c71..5069c3f 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5847,8 +5847,9 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig)
>          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);
> 

Re: [Qemu-devel] [PATCH v2 04/19] linux-user: Factor out uses of do_sigprocmask() from sigreturn code

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> All the architecture specific handlers for sigreturn include calls
> to do_sigprocmask(SIGSETMASK, &set, NULL) to set the signal mask
> from the uc_sigmask in the context being restored. Factor these
> out into calls to a set_sigmask() function. The next patch will
> want to add code which is not run when setting the signal mask
> via do_sigreturn, and this change allows us to separate the two
> cases.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/signal.c | 55 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5069c3f..1b86a85 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -239,6 +239,15 @@ int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
>      return ret;
>  }
>  
> +#if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) && \
> +    !defined(TARGET_X86_64)
> +/* Just set the guest's signal mask to the specified value */
> +static void set_sigmask(const sigset_t *set)
> +{
> +    do_sigprocmask(SIG_SETMASK, set, NULL);
> +}
> +#endif
> +
>  /* siginfo conversion */
>  
>  static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> @@ -1093,7 +1102,7 @@ long do_sigreturn(CPUX86State *env)
>      }
>  
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      /* restore registers */
>      if (restore_sigcontext(env, &frame->sc))
> @@ -1118,7 +1127,7 @@ long do_rt_sigreturn(CPUX86State *env)
>      if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
>          goto badframe;
>      target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      if (restore_sigcontext(env, &frame->uc.tuc_mcontext)) {
>          goto badframe;
> @@ -1258,7 +1267,7 @@ static int target_restore_sigframe(CPUARMState *env,
>      uint64_t pstate;
>  
>      target_to_host_sigset(&set, &sf->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      for (i = 0; i < 31; i++) {
>          __get_user(env->xregs[i], &sf->uc.tuc_mcontext.regs[i]);
> @@ -1900,7 +1909,7 @@ static long do_sigreturn_v1(CPUARMState *env)
>      }
>  
>      target_to_host_sigset_internal(&host_set, &set);
> -    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
> +    set_sigmask(&host_set);
>  
>      if (restore_sigcontext(env, &frame->sc)) {
>          goto badframe;
> @@ -1981,7 +1990,7 @@ static int do_sigframe_return_v2(CPUARMState *env, target_ulong frame_addr,
>      abi_ulong *regspace;
>  
>      target_to_host_sigset(&host_set, &uc->tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
> +    set_sigmask(&host_set);
>  
>      if (restore_sigcontext(env, &uc->tuc_mcontext))
>          return 1;
> @@ -2077,7 +2086,7 @@ static long do_rt_sigreturn_v1(CPUARMState *env)
>      }
>  
>      target_to_host_sigset(&host_set, &frame->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
> +    set_sigmask(&host_set);
>  
>      if (restore_sigcontext(env, &frame->uc.tuc_mcontext)) {
>          goto badframe;
> @@ -2453,7 +2462,7 @@ long do_sigreturn(CPUSPARCState *env)
>      }
>  
>      target_to_host_sigset_internal(&host_set, &set);
> -    do_sigprocmask(SIG_SETMASK, &host_set, NULL);
> +    set_sigmask(&host_set);
>  
>      if (err) {
>          goto segv_and_exit;
> @@ -2576,7 +2585,7 @@ void sparc64_set_context(CPUSPARCState *env)
>              }
>          }
>          target_to_host_sigset_internal(&set, &target_set);
> -        do_sigprocmask(SIG_SETMASK, &set, NULL);
> +        set_sigmask(&set);
>      }
>      env->pc = pc;
>      env->npc = npc;
> @@ -2993,7 +3002,7 @@ long do_sigreturn(CPUMIPSState *regs)
>      }
>  
>      target_to_host_sigset_internal(&blocked, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>  
>      restore_sigcontext(regs, &frame->sf_sc);
>  
> @@ -3097,7 +3106,7 @@ long do_rt_sigreturn(CPUMIPSState *env)
>      }
>  
>      target_to_host_sigset(&blocked, &frame->rs_uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>  
>      restore_sigcontext(env, &frame->rs_uc.tuc_mcontext);
>  
> @@ -3371,7 +3380,7 @@ long do_sigreturn(CPUSH4State *regs)
>          goto badframe;
>  
>      target_to_host_sigset_internal(&blocked, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>  
>      restore_sigcontext(regs, &frame->sc);
>  
> @@ -3397,7 +3406,7 @@ long do_rt_sigreturn(CPUSH4State *regs)
>      }
>  
>      target_to_host_sigset(&blocked, &frame->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>  
>      restore_sigcontext(regs, &frame->uc.tuc_mcontext);
>  
> @@ -3621,7 +3630,7 @@ long do_sigreturn(CPUMBState *env)
>          __get_user(target_set.sig[i], &frame->extramask[i - 1]);
>      }
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      restore_sigcontext(&frame->uc.tuc_mcontext, env);
>      /* We got here through a sigreturn syscall, our path back is via an
> @@ -3792,7 +3801,7 @@ long do_sigreturn(CPUCRISState *env)
>          __get_user(target_set.sig[i], &frame->extramask[i - 1]);
>      }
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      restore_sigcontext(&frame->sc, env);
>      unlock_user_struct(frame, frame_addr, 0);
> @@ -4284,7 +4293,7 @@ long do_sigreturn(CPUS390XState *env)
>      __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
>  
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL); /* ~_BLOCKABLE? */
> +    set_sigmask(&set); /* ~_BLOCKABLE? */
>  
>      if (restore_sigregs(env, &frame->sregs)) {
>          goto badframe;
> @@ -4310,7 +4319,7 @@ long do_rt_sigreturn(CPUS390XState *env)
>      }
>      target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
>  
> -    do_sigprocmask(SIG_SETMASK, &set, NULL); /* ~_BLOCKABLE? */
> +    set_sigmask(&set); /* ~_BLOCKABLE? */
>  
>      if (restore_sigregs(env, &frame->uc.tuc_mcontext)) {
>          goto badframe;
> @@ -4872,7 +4881,7 @@ long do_sigreturn(CPUPPCState *env)
>      __get_user(set.sig[1], &sc->_unused[3]);
>  #endif
>      target_to_host_sigset_internal(&blocked, &set);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>  
>      __get_user(sr_addr, &sc->regs);
>      if (!lock_user_struct(VERIFY_READ, sr, sr_addr, 1))
> @@ -4913,7 +4922,7 @@ static int do_setcontext(struct target_ucontext *ucp, CPUPPCState *env, int sig)
>          return 1;
>  
>      target_to_host_sigset_internal(&blocked, &set);
> -    do_sigprocmask(SIG_SETMASK, &blocked, NULL);
> +    set_sigmask(&blocked);
>      restore_user_regs(env, mcp, sig);
>  
>      unlock_user_struct(mcp, mcp_addr, 1);
> @@ -5261,7 +5270,7 @@ long do_sigreturn(CPUM68KState *env)
>      }
>  
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      /* restore registers */
>  
> @@ -5287,7 +5296,7 @@ long do_rt_sigreturn(CPUM68KState *env)
>          goto badframe;
>  
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      /* restore registers */
>  
> @@ -5530,7 +5539,7 @@ long do_sigreturn(CPUAlphaState *env)
>      __get_user(target_set.sig[0], &sc->sc_mask);
>  
>      target_to_host_sigset_internal(&set, &target_set);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      restore_sigcontext(env, sc);
>      unlock_user_struct(sc, sc_addr, 0);
> @@ -5551,7 +5560,7 @@ long do_rt_sigreturn(CPUAlphaState *env)
>          goto badframe;
>      }
>      target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      restore_sigcontext(env, &frame->uc.tuc_mcontext);
>      if (do_sigaltstack(frame_addr + offsetof(struct target_rt_sigframe,
> @@ -5718,7 +5727,7 @@ long do_rt_sigreturn(CPUTLGState *env)
>          goto badframe;
>      }
>      target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
> -    do_sigprocmask(SIG_SETMASK, &set, NULL);
> +    set_sigmask(&set);
>  
>      restore_sigcontext(env, &frame->uc.tuc_mcontext);
>      if (do_sigaltstack(frame_addr + offsetof(struct target_rt_sigframe,
> 

Re: [Qemu-devel] [PATCH v2 05/19] linux-user: Define macro for size of host kernel sigset_t

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Some host syscalls take an argument specifying the size of a
> host kernel's sigset_t (which isn't necessarily the same as
> that of the host libc's type of that name). Instead of hardcoding
> _NSIG / 8 where we do this, define and use a SIGSET_T_SIZE macro.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/syscall.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index df70255..e4b7404 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -119,6 +119,10 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #define	VFAT_IOCTL_READDIR_BOTH		_IOR('r', 1, struct linux_dirent [2])
>  #define	VFAT_IOCTL_READDIR_SHORT	_IOR('r', 2, struct linux_dirent [2])
>  
> +/* This is the size of the host kernel's sigset_t, needed where we make
> + * direct system calls that take a sigset_t pointer and a size.
> + */
> +#define SIGSET_T_SIZE (_NSIG / 8)
>  
>  #undef _syscall0
>  #undef _syscall1
> @@ -7221,7 +7225,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              /* Extract the two packed args for the sigset */
>              if (arg6) {
>                  sig_ptr = &sig;
> -                sig.size = _NSIG / 8;
> +                sig.size = SIGSET_T_SIZE;
>  
>                  arg7 = lock_user(VERIFY_READ, arg6, sizeof(*arg7) * 2, 1);
>                  if (!arg7) {
> @@ -8275,7 +8279,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      set = NULL;
>                  }
>  
> -                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts, set, _NSIG/8));
> +                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts,
> +                                          set, SIGSET_T_SIZE));
>  
>                  if (!is_error(ret) && arg3) {
>                      host_to_target_timespec(arg3, timeout_ts);
> 

Re: [Qemu-devel] [PATCH v2 06/19] linux-user: Use safe_syscall for sigsuspend syscalls

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Use the safe_syscall wrapper for sigsuspend syscalls. This
> means that we will definitely deliver a signal that arrives
> before we do the sigsuspend call, rather than blocking first
> and delivering afterwards.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/syscall.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e4b7404..083f26f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -703,6 +703,7 @@ safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
>                fd_set *, exceptfds, struct timespec *, timeout, void *, sig)
>  safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
>                const struct timespec *,timeout,int *,uaddr2,int,val3)
> +safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
>  
>  static inline int host_to_target_sock_type(int host_type)
>  {
> @@ -7007,7 +7008,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              target_to_host_old_sigset(&set, p);
>              unlock_user(p, arg1, 0);
>  #endif
> -            ret = get_errno(sigsuspend(&set));
> +            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
>          }
>          break;
>  #endif
> @@ -7018,7 +7019,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  goto efault;
>              target_to_host_sigset(&set, p);
>              unlock_user(p, arg1, 0);
> -            ret = get_errno(sigsuspend(&set));
> +            ret = get_errno(safe_rt_sigsuspend(&set, SIGSET_T_SIZE));
>          }
>          break;
>      case TARGET_NR_rt_sigtimedwait:
> 

Re: [Qemu-devel] [PATCH v2 15/19] linux-user: Use safe_syscall for kill, tkill and tgkill syscalls

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> Use the safe_syscall wrapper for the kill, tkill and tgkill syscalls.
> Without this, if a thread sent a SIGKILL to itself it could kill the
> thread before we had a chance to process a signal that arrived just
> before the SIGKILL, and that signal would get lost.
> 
> We drop all the ifdeffery for tkill and tgkill, because every guest
> architecture we support implements them, and they've been in Linux
> since 2003 so we can assume the host headers define the __NR_tkill
> and __NR_tgkill constants.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
> Timothy's patchset used block_signals() for this, but I
> think safe_syscall is nicer when it can be used, since
> it's lower overhead and less intrusive. Also extended to
> cover all the thread-killing syscalls rather than just kill.
> ---
>  linux-user/syscall.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index cb5d519..549f571 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -186,8 +186,6 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 arg4,type5 arg5,	\
>  #define __NR_sys_getpriority __NR_getpriority
>  #define __NR_sys_rt_sigqueueinfo __NR_rt_sigqueueinfo
>  #define __NR_sys_syslog __NR_syslog
> -#define __NR_sys_tgkill __NR_tgkill
> -#define __NR_sys_tkill __NR_tkill
>  #define __NR_sys_futex __NR_futex
>  #define __NR_sys_inotify_init __NR_inotify_init
>  #define __NR_sys_inotify_add_watch __NR_inotify_add_watch
> @@ -225,12 +223,6 @@ _syscall5(int, _llseek,  uint,  fd, ulong, hi, ulong, lo,
>  #endif
>  _syscall3(int,sys_rt_sigqueueinfo,int,pid,int,sig,siginfo_t *,uinfo)
>  _syscall3(int,sys_syslog,int,type,char*,bufp,int,len)
> -#if defined(TARGET_NR_tgkill) && defined(__NR_tgkill)
> -_syscall3(int,sys_tgkill,int,tgid,int,pid,int,sig)
> -#endif
> -#if defined(TARGET_NR_tkill) && defined(__NR_tkill)
> -_syscall2(int,sys_tkill,int,tid,int,sig)
> -#endif
>  #ifdef __NR_exit_group
>  _syscall1(int,exit_group,int,error_code)
>  #endif
> @@ -704,6 +696,9 @@ safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
>  safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
>                const struct timespec *,timeout,int *,uaddr2,int,val3)
>  safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
> +safe_syscall2(int, kill, pid_t, pid, int, sig)
> +safe_syscall2(int, tkill, int, tid, int, sig)
> +safe_syscall3(int, tgkill, int, tgid, int, pid, int, sig)
>  
>  static inline int host_to_target_sock_type(int host_type)
>  {
> @@ -6528,7 +6523,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          ret = 0;
>          break;
>      case TARGET_NR_kill:
> -        ret = get_errno(kill(arg1, target_to_host_signal(arg2)));
> +        ret = get_errno(safe_kill(arg1, target_to_host_signal(arg2)));
>          break;
>  #ifdef TARGET_NR_rename
>      case TARGET_NR_rename:
> @@ -9764,18 +9759,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          break;
>  #endif
>  
> -#if defined(TARGET_NR_tkill) && defined(__NR_tkill)
>      case TARGET_NR_tkill:
> -        ret = get_errno(sys_tkill((int)arg1, target_to_host_signal(arg2)));
> +        ret = get_errno(safe_tkill((int)arg1, target_to_host_signal(arg2)));
>          break;
> -#endif
>  
> -#if defined(TARGET_NR_tgkill) && defined(__NR_tgkill)
>      case TARGET_NR_tgkill:
> -	ret = get_errno(sys_tgkill((int)arg1, (int)arg2,
> +        ret = get_errno(safe_tgkill((int)arg1, (int)arg2,
>                          target_to_host_signal(arg3)));
> -	break;
> -#endif
> +        break;
>  
>  #ifdef TARGET_NR_set_robust_list
>      case TARGET_NR_set_robust_list:
> 

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> The siginfo_t struct includes a union. The correct way to identify
> which fields of the union are relevant is complicated, because we
> have to use a combination of the si_code and si_signo to figure out
> which of the union's members are valid.  (Within the host kernel it
> is always possible to tell, but the kernel carefully avoids giving
> userspace the high 16 bits of si_code, so we don't have the
> information to do this the easy way...) We therefore make our best
> guess, bearing in mind that a guest can spoof most of the si_codes
> via rt_sigqueueinfo() if it likes.  Once we have made our guess, we
> record it in the top 16 bits of the si_code, so that tswap_siginfo()
> later can use it.  tswap_siginfo() then strips these top bits out
> before writing si_code to the guest (sign-extending the lower bits).
> 
> This fixes a bug where fields were sometimes wrong; in particular
> the LTP kill10 test went into an infinite loop because its signal
> handler got a si_pid value of 0 rather than the pid of the sending
> process.
> 
> As part of this change, we switch to using __put_user() in the
> tswap_siginfo code which writes out the byteswapped values to
> the target memory, in case the target memory pointer is not
> sufficiently aligned for the host CPU's requirements.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c       | 165 ++++++++++++++++++++++++++++++++--------------
>  linux-user/syscall_defs.h |  15 +++++
>  2 files changed, 131 insertions(+), 49 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index b21d6bf..8ea0cbf 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
>  #include <sys/ucontext.h>
>  #include <sys/resource.h>
>  
> @@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>                                                   const siginfo_t *info)
>  {
>      int sig = host_to_target_signal(info->si_signo);
> +    int si_code = info->si_code;
> +    int si_type;
>      tinfo->si_signo = sig;
>      tinfo->si_errno = 0;
>      tinfo->si_code = info->si_code;
>  
> -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> -            || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> -        /* Should never come here, but who knows. The information for
> -           the target is irrelevant.  */
> -        tinfo->_sifields._sigfault._addr = 0;
> -    } else if (sig == TARGET_SIGIO) {
> -        tinfo->_sifields._sigpoll._band = info->si_band;
> -        tinfo->_sifields._sigpoll._fd = info->si_fd;
> -    } else if (sig == TARGET_SIGCHLD) {
> -        tinfo->_sifields._sigchld._pid = info->si_pid;
> -        tinfo->_sifields._sigchld._uid = info->si_uid;
> -        tinfo->_sifields._sigchld._status
> +    /* This is awkward, because we have to use a combination of
> +     * the si_code and si_signo to figure out which of the union's
> +     * members are valid. (Within the host kernel it is always possible
> +     * to tell, but the kernel carefully avoids giving userspace the
> +     * high 16 bits of si_code, so we don't have the information to
> +     * do this the easy way...) We therefore make our best guess,
> +     * bearing in mind that a guest can spoof most of the si_codes
> +     * via rt_sigqueueinfo() if it likes.
> +     *
> +     * Once we have made our guess, we record it in the top 16 bits of
> +     * the si_code, so that tswap_siginfo() later can use it.
> +     * tswap_siginfo() will strip these top bits out before writing
> +     * si_code to the guest (sign-extending the lower bits).
> +     */
> +
> +    switch (si_code) {
> +    case SI_USER:
> +    case SI_TKILL:
> +    case SI_KERNEL:
> +        /* Sent via kill(), tkill() or tgkill(), or direct from the kernel.
> +         * These are the only unspoofable si_code values.
> +         */
> +        tinfo->_sifields._kill._pid = info->si_pid;
> +        tinfo->_sifields._kill._uid = info->si_uid;
> +        si_type = QEMU_SI_KILL;
> +        break;
> +    default:
> +        /* Everything else is spoofable. Make best guess based on signal */
> +        switch (sig) {
> +        case TARGET_SIGCHLD:
> +            tinfo->_sifields._sigchld._pid = info->si_pid;
> +            tinfo->_sifields._sigchld._uid = info->si_uid;
> +            tinfo->_sifields._sigchld._status
>                  = host_to_target_waitstatus(info->si_status);
> -        tinfo->_sifields._sigchld._utime = info->si_utime;
> -        tinfo->_sifields._sigchld._stime = info->si_stime;
> -    } else if (sig >= TARGET_SIGRTMIN) {
> -        tinfo->_sifields._rt._pid = info->si_pid;
> -        tinfo->_sifields._rt._uid = info->si_uid;
> -        /* XXX: potential problem if 64 bit */
> -        tinfo->_sifields._rt._sigval.sival_ptr
> +            tinfo->_sifields._sigchld._utime = info->si_utime;
> +            tinfo->_sifields._sigchld._stime = info->si_stime;
> +            si_type = QEMU_SI_CHLD;
> +            break;
> +        case TARGET_SIGIO:
> +            tinfo->_sifields._sigpoll._band = info->si_band;
> +            tinfo->_sifields._sigpoll._fd = info->si_fd;
> +            si_type = QEMU_SI_POLL;
> +            break;
> +        default:
> +            /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source. */
> +            tinfo->_sifields._rt._pid = info->si_pid;
> +            tinfo->_sifields._rt._uid = info->si_uid;
> +            /* XXX: potential problem if 64 bit */
> +            tinfo->_sifields._rt._sigval.sival_ptr
>                  = (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> +            si_type = QEMU_SI_RT;
> +            break;
> +        }
> +        break;
>      }
> +
> +    tinfo->si_code = deposit32(si_code, 16, 16, si_type);
>  }
>  
>  static void tswap_siginfo(target_siginfo_t *tinfo,
>                            const target_siginfo_t *info)
>  {
> -    int sig = info->si_signo;
> -    tinfo->si_signo = tswap32(sig);
> -    tinfo->si_errno = tswap32(info->si_errno);
> -    tinfo->si_code = tswap32(info->si_code);
> -
> -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> -        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> -        tinfo->_sifields._sigfault._addr
> -            = tswapal(info->_sifields._sigfault._addr);
> -    } else if (sig == TARGET_SIGIO) {
> -        tinfo->_sifields._sigpoll._band
> -            = tswap32(info->_sifields._sigpoll._band);
> -        tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
> -    } else if (sig == TARGET_SIGCHLD) {
> -        tinfo->_sifields._sigchld._pid
> -            = tswap32(info->_sifields._sigchld._pid);
> -        tinfo->_sifields._sigchld._uid
> -            = tswap32(info->_sifields._sigchld._uid);
> -        tinfo->_sifields._sigchld._status
> -            = tswap32(info->_sifields._sigchld._status);
> -        tinfo->_sifields._sigchld._utime
> -            = tswapal(info->_sifields._sigchld._utime);
> -        tinfo->_sifields._sigchld._stime
> -            = tswapal(info->_sifields._sigchld._stime);
> -    } else if (sig >= TARGET_SIGRTMIN) {
> -        tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
> -        tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
> -        tinfo->_sifields._rt._sigval.sival_ptr
> -            = tswapal(info->_sifields._rt._sigval.sival_ptr);
> +    int si_type = extract32(info->si_code, 16, 16);
> +    int si_code = sextract32(info->si_code, 0, 16);
> +
> +    __put_user(info->si_signo, &tinfo->si_signo);
> +    __put_user(info->si_errno, &tinfo->si_errno);
> +    __put_user(si_code, &tinfo->si_code);
> +
> +    /* We can use our internal marker of which fields in the structure
> +     * are valid, rather than duplicating the guesswork of
> +     * host_to_target_siginfo_noswap() here.
> +     */
> +    switch (si_type) {
> +    case QEMU_SI_KILL:
> +        __put_user(info->_sifields._kill._pid, &tinfo->_sifields._kill._pid);
> +        __put_user(info->_sifields._kill._uid, &tinfo->_sifields._kill._uid);
> +        break;
> +    case QEMU_SI_TIMER:

Where is coming from QEMU_SI_TIMER?
It is not used elsewhere.

Laurent

Re: [Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

[Laurent Vivier]

Le 27/05/2016 à 16:52, Peter Maydell a écrit :
> host_to_target_siginfo() is implemented by a combination of
> host_to_target_siginfo_noswap() followed by tswap_siginfo().
> The first of these two functions assumes that the target_siginfo_t
> it is writing to is correctly aligned, but the pointer passed
> into host_to_target_siginfo() is directly from the guest and
> might be misaligned. Use a local variable to avoid this problem.
> (tswap_siginfo() does now correctly handle a misaligned destination.)

You mean the pointer from the guest can not be correctly aligned for the
guest?

Laurent
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8ea0cbf..7e2a80f 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -400,8 +400,9 @@ static void tswap_siginfo(target_siginfo_t *tinfo,
>  
>  void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
>  {
> -    host_to_target_siginfo_noswap(tinfo, info);
> -    tswap_siginfo(tinfo, tinfo);
> +    target_siginfo_t tgt_tmp;
> +    host_to_target_siginfo_noswap(&tgt_tmp, info);
> +    tswap_siginfo(tinfo, &tgt_tmp);
>  }
>  
>  /* XXX: we support only POSIX RT signals are used. */
> 

Re: [Qemu-devel] [PATCH v2 19/19] linux-user: Avoid possible misalignment in target_to_host_siginfo()

[Laurent Vivier]

Le 27/05/2016 à 16:52, Peter Maydell a écrit :
> Reimplement target_to_host_siginfo() to use __get_user(), which
> handles possibly misaligned source guest structures correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/signal.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 7e2a80f..8417da7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -409,13 +409,18 @@ void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
>  /* XXX: find a solution for 64 bit (additional malloced data is needed) */
>  void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo)
>  {
> -    info->si_signo = tswap32(tinfo->si_signo);
> -    info->si_errno = tswap32(tinfo->si_errno);
> -    info->si_code = tswap32(tinfo->si_code);
> -    info->si_pid = tswap32(tinfo->_sifields._rt._pid);
> -    info->si_uid = tswap32(tinfo->_sifields._rt._uid);
> -    info->si_value.sival_ptr =
> -            (void *)(long)tswapal(tinfo->_sifields._rt._sigval.sival_ptr);
> +    /* This conversion is used only for the rt_sigqueueinfo syscall,
> +     * and so we know that the _rt fields are the valid ones.
> +     */
> +    abi_ulong sival_ptr;
> +
> +    __get_user(info->si_signo, &tinfo->si_signo);
> +    __get_user(info->si_errno, &tinfo->si_errno);
> +    __get_user(info->si_code, &tinfo->si_code);
> +    __get_user(info->si_pid, &tinfo->_sifields._rt._pid);
> +    __get_user(info->si_uid, &tinfo->_sifields._rt._uid);
> +    __get_user(sival_ptr, &tinfo->_sifields._rt._sigval.sival_ptr);
> +    info->si_value.sival_ptr = (void *)(long)sival_ptr;
>  }
>  
>  static int fatal_signal (int sig)
> 

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Peter Maydell]

On 7 June 2016 at 20:22, Laurent Vivier <laurent@vivier.eu> wrote:
> Where is coming from QEMU_SI_TIMER?
> It is not used elsewhere.

It's the enum constant that goes with "we use the
.sifields.timer fields of the union". At the moment we
don't have any cases which cause us to think we should
use those (and we didn't before this patch either), so
the case in this case statement is purely for completeness.
(I suspect the _timer fields are wrong anyway, since they're
pretty much dead code.)

The awkward thing about SI_TIMER is that because glibc
can call rt_sigqueueinfo() with a si_code of SI_TIMER[*] we
have no way to tell "this is a SI_TIMER signal from
the kernel with valid .timer fields" from "this is a
SI_TIMER from rt_sigqueueinfo with valid .rt fields".
So we assume it's always the latter.

[*] for instance, see thread_expire_timer() in
http://osxr.org:8080/glibc/source/nptl/sysdeps/pthread/timer_routines.c

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

[Peter Maydell]

On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>> host_to_target_siginfo() is implemented by a combination of
>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>> The first of these two functions assumes that the target_siginfo_t
>> it is writing to is correctly aligned, but the pointer passed
>> into host_to_target_siginfo() is directly from the guest and
>> might be misaligned. Use a local variable to avoid this problem.
>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>
> You mean the pointer from the guest can not be correctly aligned for the
> guest?

Might not be correctly aligned for the host (for that matter
it might not be correctly aligned for the guest,
if the guest is being malicious or buggy, but it's the
host alignment we care about.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Riku Voipio]

On Fri, May 27, 2016 at 03:51:59PM +0100, Peter Maydell wrote:
> The siginfo_t struct includes a union. The correct way to identify
> which fields of the union are relevant is complicated, because we
> have to use a combination of the si_code and si_signo to figure out
> which of the union's members are valid.  (Within the host kernel it
> is always possible to tell, but the kernel carefully avoids giving
> userspace the high 16 bits of si_code, so we don't have the
> information to do this the easy way...) We therefore make our best
> guess, bearing in mind that a guest can spoof most of the si_codes
> via rt_sigqueueinfo() if it likes.  Once we have made our guess, we
> record it in the top 16 bits of the si_code, so that tswap_siginfo()
> later can use it.  tswap_siginfo() then strips these top bits out
> before writing si_code to the guest (sign-extending the lower bits).
> 
> This fixes a bug where fields were sometimes wrong; in particular
> the LTP kill10 test went into an infinite loop because its signal
> handler got a si_pid value of 0 rather than the pid of the sending
> process.
> 
> As part of this change, we switch to using __put_user() in the
> tswap_siginfo code which writes out the byteswapped values to
> the target memory, in case the target memory pointer is not
> sufficiently aligned for the host CPU's requirements.

At least on Debian jessie, this blows up a selection of architectures:

  CC    microblazeel-linux-user/linux-user/signal.o
  CC    cris-linux-user/linux-user/signal.o
  CC    microblaze-linux-user/linux-user/signal.o
  CC    sparc-linux-user/linux-user/signal.o
  CC    sparc64-linux-user/linux-user/signal.o
  CC    x86_64-linux-user/linux-user/signal.o
  CC    unicore32-linux-user/linux-user/signal.o
  CC    sparc32plus-linux-user/linux-user/signal.o
/home/voipio/linaro/qemu/linux-user/signal.c: In function ‘host_to_target_siginfo’:
/home/voipio/linaro/qemu/linux-user/signal.c:387:10: error: ‘tgt_tmp._sifields._sigchld._stime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         __put_user(info->_sifields._sigchld._stime,
          ^
/home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._stime’ was declared here
     target_siginfo_t tgt_tmp;
                      ^
/home/voipio/linaro/qemu/linux-user/signal.c:385:10: error: ‘tgt_tmp._sifields._sigchld._utime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         __put_user(info->_sifields._sigchld._utime,
          ^
/home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._utime’ was declared here
     target_siginfo_t tgt_tmp;
                      ^
/home/voipio/linaro/qemu/linux-user/signal.c:383:10: error: ‘tgt_tmp._sifields._sigchld._status’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         __put_user(info->_sifields._sigchld._status,
          ^
/home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._status’ was declared here
     target_siginfo_t tgt_tmp;
                      ^
cc1: all warnings being treated as errors

These appear to be the architectures where setup_rt_frame isn't implemented.


> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c       | 165 ++++++++++++++++++++++++++++++++--------------
>  linux-user/syscall_defs.h |  15 +++++
>  2 files changed, 131 insertions(+), 49 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index b21d6bf..8ea0cbf 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
>  #include <sys/ucontext.h>
>  #include <sys/resource.h>
>  
> @@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>                                                   const siginfo_t *info)
>  {
>      int sig = host_to_target_signal(info->si_signo);
> +    int si_code = info->si_code;
> +    int si_type;
>      tinfo->si_signo = sig;
>      tinfo->si_errno = 0;
>      tinfo->si_code = info->si_code;
>  
> -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> -            || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> -        /* Should never come here, but who knows. The information for
> -           the target is irrelevant.  */
> -        tinfo->_sifields._sigfault._addr = 0;
> -    } else if (sig == TARGET_SIGIO) {
> -        tinfo->_sifields._sigpoll._band = info->si_band;
> -        tinfo->_sifields._sigpoll._fd = info->si_fd;
> -    } else if (sig == TARGET_SIGCHLD) {
> -        tinfo->_sifields._sigchld._pid = info->si_pid;
> -        tinfo->_sifields._sigchld._uid = info->si_uid;
> -        tinfo->_sifields._sigchld._status
> +    /* This is awkward, because we have to use a combination of
> +     * the si_code and si_signo to figure out which of the union's
> +     * members are valid. (Within the host kernel it is always possible
> +     * to tell, but the kernel carefully avoids giving userspace the
> +     * high 16 bits of si_code, so we don't have the information to
> +     * do this the easy way...) We therefore make our best guess,
> +     * bearing in mind that a guest can spoof most of the si_codes
> +     * via rt_sigqueueinfo() if it likes.
> +     *
> +     * Once we have made our guess, we record it in the top 16 bits of
> +     * the si_code, so that tswap_siginfo() later can use it.
> +     * tswap_siginfo() will strip these top bits out before writing
> +     * si_code to the guest (sign-extending the lower bits).
> +     */
> +
> +    switch (si_code) {
> +    case SI_USER:
> +    case SI_TKILL:
> +    case SI_KERNEL:
> +        /* Sent via kill(), tkill() or tgkill(), or direct from the kernel.
> +         * These are the only unspoofable si_code values.
> +         */
> +        tinfo->_sifields._kill._pid = info->si_pid;
> +        tinfo->_sifields._kill._uid = info->si_uid;
> +        si_type = QEMU_SI_KILL;
> +        break;
> +    default:
> +        /* Everything else is spoofable. Make best guess based on signal */
> +        switch (sig) {
> +        case TARGET_SIGCHLD:
> +            tinfo->_sifields._sigchld._pid = info->si_pid;
> +            tinfo->_sifields._sigchld._uid = info->si_uid;
> +            tinfo->_sifields._sigchld._status
>                  = host_to_target_waitstatus(info->si_status);
> -        tinfo->_sifields._sigchld._utime = info->si_utime;
> -        tinfo->_sifields._sigchld._stime = info->si_stime;
> -    } else if (sig >= TARGET_SIGRTMIN) {
> -        tinfo->_sifields._rt._pid = info->si_pid;
> -        tinfo->_sifields._rt._uid = info->si_uid;
> -        /* XXX: potential problem if 64 bit */
> -        tinfo->_sifields._rt._sigval.sival_ptr
> +            tinfo->_sifields._sigchld._utime = info->si_utime;
> +            tinfo->_sifields._sigchld._stime = info->si_stime;
> +            si_type = QEMU_SI_CHLD;
> +            break;
> +        case TARGET_SIGIO:
> +            tinfo->_sifields._sigpoll._band = info->si_band;
> +            tinfo->_sifields._sigpoll._fd = info->si_fd;
> +            si_type = QEMU_SI_POLL;
> +            break;
> +        default:
> +            /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source. */
> +            tinfo->_sifields._rt._pid = info->si_pid;
> +            tinfo->_sifields._rt._uid = info->si_uid;
> +            /* XXX: potential problem if 64 bit */
> +            tinfo->_sifields._rt._sigval.sival_ptr
>                  = (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> +            si_type = QEMU_SI_RT;
> +            break;
> +        }
> +        break;
>      }
> +
> +    tinfo->si_code = deposit32(si_code, 16, 16, si_type);
>  }
>  
>  static void tswap_siginfo(target_siginfo_t *tinfo,
>                            const target_siginfo_t *info)
>  {
> -    int sig = info->si_signo;
> -    tinfo->si_signo = tswap32(sig);
> -    tinfo->si_errno = tswap32(info->si_errno);
> -    tinfo->si_code = tswap32(info->si_code);
> -
> -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> -        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> -        tinfo->_sifields._sigfault._addr
> -            = tswapal(info->_sifields._sigfault._addr);
> -    } else if (sig == TARGET_SIGIO) {
> -        tinfo->_sifields._sigpoll._band
> -            = tswap32(info->_sifields._sigpoll._band);
> -        tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
> -    } else if (sig == TARGET_SIGCHLD) {
> -        tinfo->_sifields._sigchld._pid
> -            = tswap32(info->_sifields._sigchld._pid);
> -        tinfo->_sifields._sigchld._uid
> -            = tswap32(info->_sifields._sigchld._uid);
> -        tinfo->_sifields._sigchld._status
> -            = tswap32(info->_sifields._sigchld._status);
> -        tinfo->_sifields._sigchld._utime
> -            = tswapal(info->_sifields._sigchld._utime);
> -        tinfo->_sifields._sigchld._stime
> -            = tswapal(info->_sifields._sigchld._stime);
> -    } else if (sig >= TARGET_SIGRTMIN) {
> -        tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
> -        tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
> -        tinfo->_sifields._rt._sigval.sival_ptr
> -            = tswapal(info->_sifields._rt._sigval.sival_ptr);
> +    int si_type = extract32(info->si_code, 16, 16);
> +    int si_code = sextract32(info->si_code, 0, 16);
> +
> +    __put_user(info->si_signo, &tinfo->si_signo);
> +    __put_user(info->si_errno, &tinfo->si_errno);
> +    __put_user(si_code, &tinfo->si_code);
> +
> +    /* We can use our internal marker of which fields in the structure
> +     * are valid, rather than duplicating the guesswork of
> +     * host_to_target_siginfo_noswap() here.
> +     */
> +    switch (si_type) {
> +    case QEMU_SI_KILL:
> +        __put_user(info->_sifields._kill._pid, &tinfo->_sifields._kill._pid);
> +        __put_user(info->_sifields._kill._uid, &tinfo->_sifields._kill._uid);
> +        break;
> +    case QEMU_SI_TIMER:
> +        __put_user(info->_sifields._timer._timer1,
> +                   &tinfo->_sifields._timer._timer1);
> +        __put_user(info->_sifields._timer._timer2,
> +                   &tinfo->_sifields._timer._timer2);
> +        break;
> +    case QEMU_SI_POLL:
> +        __put_user(info->_sifields._sigpoll._band,
> +                   &tinfo->_sifields._sigpoll._band);
> +        __put_user(info->_sifields._sigpoll._fd,
> +                   &tinfo->_sifields._sigpoll._fd);
> +        break;
> +    case QEMU_SI_FAULT:
> +        __put_user(info->_sifields._sigfault._addr,
> +                   &tinfo->_sifields._sigfault._addr);
> +        break;
> +    case QEMU_SI_CHLD:
> +        __put_user(info->_sifields._sigchld._pid,
> +                   &tinfo->_sifields._sigchld._pid);
> +        __put_user(info->_sifields._sigchld._uid,
> +                   &tinfo->_sifields._sigchld._uid);
> +        __put_user(info->_sifields._sigchld._status,
> +                   &tinfo->_sifields._sigchld._status);
> +        __put_user(info->_sifields._sigchld._utime,
> +                   &tinfo->_sifields._sigchld._utime);
> +        __put_user(info->_sifields._sigchld._stime,
> +                   &tinfo->_sifields._sigchld._stime);
> +        break;
> +    case QEMU_SI_RT:
> +        __put_user(info->_sifields._rt._pid, &tinfo->_sifields._rt._pid);
> +        __put_user(info->_sifields._rt._uid, &tinfo->_sifields._rt._uid);
> +        __put_user(info->_sifields._rt._sigval.sival_ptr,
> +                   &tinfo->_sifields._rt._sigval.sival_ptr);
> +        break;
> +    default:
> +        g_assert_not_reached();
>      }
>  }
>  
> -
>  void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
>  {
>      host_to_target_siginfo_noswap(tinfo, info);
> @@ -505,6 +565,13 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
>  
>      trace_user_queue_signal(env, sig);
>  
> +    /* Currently all callers define siginfo structures which
> +     * use the _sifields._sigfault union member, so we can
> +     * set the type here. If that changes we should push this
> +     * out so the si_type is passed in by callers.
> +     */
> +    info->si_code = deposit32(info->si_code, 16, 16, QEMU_SI_FAULT);
> +
>      ts->sync_signal.info = *info;
>      ts->sync_signal.pending = sig;
>      /* signal that a new signal is pending */
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 34af15a..124754f 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -673,6 +673,21 @@ typedef struct {
>  
>  #define TARGET_SI_PAD_SIZE ((TARGET_SI_MAX_SIZE - TARGET_SI_PREAMBLE_SIZE) / sizeof(int))
>  
> +/* Within QEMU the top 16 bits of si_code indicate which of the parts of
> + * the union in target_siginfo is valid. This only applies between
> + * host_to_target_siginfo_noswap() and tswap_siginfo(); it does not
> + * appear either within host siginfo_t or in target_siginfo structures
> + * which we get from the guest userspace program. (The Linux kernel
> + * does a similar thing with using the top bits for its own internal
> + * purposes but not letting them be visible to userspace.)
> + */
> +#define QEMU_SI_KILL 0
> +#define QEMU_SI_TIMER 1
> +#define QEMU_SI_POLL 2
> +#define QEMU_SI_FAULT 3
> +#define QEMU_SI_CHLD 4
> +#define QEMU_SI_RT 5
> +
>  typedef struct target_siginfo {
>  #ifdef TARGET_MIPS
>  	int si_signo;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Riku Voipio]

On Wed, Jun 08, 2016 at 09:30:35AM +0300, Riku Voipio wrote:
> On Fri, May 27, 2016 at 03:51:59PM +0100, Peter Maydell wrote:
> > The siginfo_t struct includes a union. The correct way to identify
> > which fields of the union are relevant is complicated, because we
> > have to use a combination of the si_code and si_signo to figure out
> > which of the union's members are valid.  (Within the host kernel it
> > is always possible to tell, but the kernel carefully avoids giving
> > userspace the high 16 bits of si_code, so we don't have the
> > information to do this the easy way...) We therefore make our best
> > guess, bearing in mind that a guest can spoof most of the si_codes
> > via rt_sigqueueinfo() if it likes.  Once we have made our guess, we
> > record it in the top 16 bits of the si_code, so that tswap_siginfo()
> > later can use it.  tswap_siginfo() then strips these top bits out
> > before writing si_code to the guest (sign-extending the lower bits).
> > 
> > This fixes a bug where fields were sometimes wrong; in particular
> > the LTP kill10 test went into an infinite loop because its signal
> > handler got a si_pid value of 0 rather than the pid of the sending
> > process.
> > 
> > As part of this change, we switch to using __put_user() in the
> > tswap_siginfo code which writes out the byteswapped values to
> > the target memory, in case the target memory pointer is not
> > sufficiently aligned for the host CPU's requirements.
> 
> At least on Debian jessie, this blows up a selection of architectures:
> 
>   CC    microblazeel-linux-user/linux-user/signal.o
>   CC    cris-linux-user/linux-user/signal.o
>   CC    microblaze-linux-user/linux-user/signal.o
>   CC    sparc-linux-user/linux-user/signal.o
>   CC    sparc64-linux-user/linux-user/signal.o
>   CC    x86_64-linux-user/linux-user/signal.o
>   CC    unicore32-linux-user/linux-user/signal.o
>   CC    sparc32plus-linux-user/linux-user/signal.o
> /home/voipio/linaro/qemu/linux-user/signal.c: In function ‘host_to_target_siginfo’:
> /home/voipio/linaro/qemu/linux-user/signal.c:387:10: error: ‘tgt_tmp._sifields._sigchld._stime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._stime,

On the second look, this compile fail only happens if the next patch
that introduces tgt_tmp variable is also applied. Dropping that patch
instead.

> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._stime’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> /home/voipio/linaro/qemu/linux-user/signal.c:385:10: error: ‘tgt_tmp._sifields._sigchld._utime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._utime,
>           ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._utime’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> /home/voipio/linaro/qemu/linux-user/signal.c:383:10: error: ‘tgt_tmp._sifields._sigchld._status’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._status,
>           ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._status’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> cc1: all warnings being treated as errors
> 
> These appear to be the architectures where setup_rt_frame isn't implemented.
> 
> 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  linux-user/signal.c       | 165 ++++++++++++++++++++++++++++++++--------------
> >  linux-user/syscall_defs.h |  15 +++++
> >  2 files changed, 131 insertions(+), 49 deletions(-)
> > 
> > diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index b21d6bf..8ea0cbf 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -17,6 +17,7 @@
> >   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> >   */
> >  #include "qemu/osdep.h"
> > +#include "qemu/bitops.h"
> >  #include <sys/ucontext.h>
> >  #include <sys/resource.h>
> >  
> > @@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> >                                                   const siginfo_t *info)
> >  {
> >      int sig = host_to_target_signal(info->si_signo);
> > +    int si_code = info->si_code;
> > +    int si_type;
> >      tinfo->si_signo = sig;
> >      tinfo->si_errno = 0;
> >      tinfo->si_code = info->si_code;
> >  
> > -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> > -            || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> > -        /* Should never come here, but who knows. The information for
> > -           the target is irrelevant.  */
> > -        tinfo->_sifields._sigfault._addr = 0;
> > -    } else if (sig == TARGET_SIGIO) {
> > -        tinfo->_sifields._sigpoll._band = info->si_band;
> > -        tinfo->_sifields._sigpoll._fd = info->si_fd;
> > -    } else if (sig == TARGET_SIGCHLD) {
> > -        tinfo->_sifields._sigchld._pid = info->si_pid;
> > -        tinfo->_sifields._sigchld._uid = info->si_uid;
> > -        tinfo->_sifields._sigchld._status
> > +    /* This is awkward, because we have to use a combination of
> > +     * the si_code and si_signo to figure out which of the union's
> > +     * members are valid. (Within the host kernel it is always possible
> > +     * to tell, but the kernel carefully avoids giving userspace the
> > +     * high 16 bits of si_code, so we don't have the information to
> > +     * do this the easy way...) We therefore make our best guess,
> > +     * bearing in mind that a guest can spoof most of the si_codes
> > +     * via rt_sigqueueinfo() if it likes.
> > +     *
> > +     * Once we have made our guess, we record it in the top 16 bits of
> > +     * the si_code, so that tswap_siginfo() later can use it.
> > +     * tswap_siginfo() will strip these top bits out before writing
> > +     * si_code to the guest (sign-extending the lower bits).
> > +     */
> > +
> > +    switch (si_code) {
> > +    case SI_USER:
> > +    case SI_TKILL:
> > +    case SI_KERNEL:
> > +        /* Sent via kill(), tkill() or tgkill(), or direct from the kernel.
> > +         * These are the only unspoofable si_code values.
> > +         */
> > +        tinfo->_sifields._kill._pid = info->si_pid;
> > +        tinfo->_sifields._kill._uid = info->si_uid;
> > +        si_type = QEMU_SI_KILL;
> > +        break;
> > +    default:
> > +        /* Everything else is spoofable. Make best guess based on signal */
> > +        switch (sig) {
> > +        case TARGET_SIGCHLD:
> > +            tinfo->_sifields._sigchld._pid = info->si_pid;
> > +            tinfo->_sifields._sigchld._uid = info->si_uid;
> > +            tinfo->_sifields._sigchld._status
> >                  = host_to_target_waitstatus(info->si_status);
> > -        tinfo->_sifields._sigchld._utime = info->si_utime;
> > -        tinfo->_sifields._sigchld._stime = info->si_stime;
> > -    } else if (sig >= TARGET_SIGRTMIN) {
> > -        tinfo->_sifields._rt._pid = info->si_pid;
> > -        tinfo->_sifields._rt._uid = info->si_uid;
> > -        /* XXX: potential problem if 64 bit */
> > -        tinfo->_sifields._rt._sigval.sival_ptr
> > +            tinfo->_sifields._sigchld._utime = info->si_utime;
> > +            tinfo->_sifields._sigchld._stime = info->si_stime;
> > +            si_type = QEMU_SI_CHLD;
> > +            break;
> > +        case TARGET_SIGIO:
> > +            tinfo->_sifields._sigpoll._band = info->si_band;
> > +            tinfo->_sifields._sigpoll._fd = info->si_fd;
> > +            si_type = QEMU_SI_POLL;
> > +            break;
> > +        default:
> > +            /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source. */
> > +            tinfo->_sifields._rt._pid = info->si_pid;
> > +            tinfo->_sifields._rt._uid = info->si_uid;
> > +            /* XXX: potential problem if 64 bit */
> > +            tinfo->_sifields._rt._sigval.sival_ptr
> >                  = (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> > +            si_type = QEMU_SI_RT;
> > +            break;
> > +        }
> > +        break;
> >      }
> > +
> > +    tinfo->si_code = deposit32(si_code, 16, 16, si_type);
> >  }
> >  
> >  static void tswap_siginfo(target_siginfo_t *tinfo,
> >                            const target_siginfo_t *info)
> >  {
> > -    int sig = info->si_signo;
> > -    tinfo->si_signo = tswap32(sig);
> > -    tinfo->si_errno = tswap32(info->si_errno);
> > -    tinfo->si_code = tswap32(info->si_code);
> > -
> > -    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> > -        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> > -        tinfo->_sifields._sigfault._addr
> > -            = tswapal(info->_sifields._sigfault._addr);
> > -    } else if (sig == TARGET_SIGIO) {
> > -        tinfo->_sifields._sigpoll._band
> > -            = tswap32(info->_sifields._sigpoll._band);
> > -        tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
> > -    } else if (sig == TARGET_SIGCHLD) {
> > -        tinfo->_sifields._sigchld._pid
> > -            = tswap32(info->_sifields._sigchld._pid);
> > -        tinfo->_sifields._sigchld._uid
> > -            = tswap32(info->_sifields._sigchld._uid);
> > -        tinfo->_sifields._sigchld._status
> > -            = tswap32(info->_sifields._sigchld._status);
> > -        tinfo->_sifields._sigchld._utime
> > -            = tswapal(info->_sifields._sigchld._utime);
> > -        tinfo->_sifields._sigchld._stime
> > -            = tswapal(info->_sifields._sigchld._stime);
> > -    } else if (sig >= TARGET_SIGRTMIN) {
> > -        tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
> > -        tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
> > -        tinfo->_sifields._rt._sigval.sival_ptr
> > -            = tswapal(info->_sifields._rt._sigval.sival_ptr);
> > +    int si_type = extract32(info->si_code, 16, 16);
> > +    int si_code = sextract32(info->si_code, 0, 16);
> > +
> > +    __put_user(info->si_signo, &tinfo->si_signo);
> > +    __put_user(info->si_errno, &tinfo->si_errno);
> > +    __put_user(si_code, &tinfo->si_code);
> > +
> > +    /* We can use our internal marker of which fields in the structure
> > +     * are valid, rather than duplicating the guesswork of
> > +     * host_to_target_siginfo_noswap() here.
> > +     */
> > +    switch (si_type) {
> > +    case QEMU_SI_KILL:
> > +        __put_user(info->_sifields._kill._pid, &tinfo->_sifields._kill._pid);
> > +        __put_user(info->_sifields._kill._uid, &tinfo->_sifields._kill._uid);
> > +        break;
> > +    case QEMU_SI_TIMER:
> > +        __put_user(info->_sifields._timer._timer1,
> > +                   &tinfo->_sifields._timer._timer1);
> > +        __put_user(info->_sifields._timer._timer2,
> > +                   &tinfo->_sifields._timer._timer2);
> > +        break;
> > +    case QEMU_SI_POLL:
> > +        __put_user(info->_sifields._sigpoll._band,
> > +                   &tinfo->_sifields._sigpoll._band);
> > +        __put_user(info->_sifields._sigpoll._fd,
> > +                   &tinfo->_sifields._sigpoll._fd);
> > +        break;
> > +    case QEMU_SI_FAULT:
> > +        __put_user(info->_sifields._sigfault._addr,
> > +                   &tinfo->_sifields._sigfault._addr);
> > +        break;
> > +    case QEMU_SI_CHLD:
> > +        __put_user(info->_sifields._sigchld._pid,
> > +                   &tinfo->_sifields._sigchld._pid);
> > +        __put_user(info->_sifields._sigchld._uid,
> > +                   &tinfo->_sifields._sigchld._uid);
> > +        __put_user(info->_sifields._sigchld._status,
> > +                   &tinfo->_sifields._sigchld._status);
> > +        __put_user(info->_sifields._sigchld._utime,
> > +                   &tinfo->_sifields._sigchld._utime);
> > +        __put_user(info->_sifields._sigchld._stime,
> > +                   &tinfo->_sifields._sigchld._stime);
> > +        break;
> > +    case QEMU_SI_RT:
> > +        __put_user(info->_sifields._rt._pid, &tinfo->_sifields._rt._pid);
> > +        __put_user(info->_sifields._rt._uid, &tinfo->_sifields._rt._uid);
> > +        __put_user(info->_sifields._rt._sigval.sival_ptr,
> > +                   &tinfo->_sifields._rt._sigval.sival_ptr);
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> >      }
> >  }
> >  
> > -
> >  void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
> >  {
> >      host_to_target_siginfo_noswap(tinfo, info);
> > @@ -505,6 +565,13 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
> >  
> >      trace_user_queue_signal(env, sig);
> >  
> > +    /* Currently all callers define siginfo structures which
> > +     * use the _sifields._sigfault union member, so we can
> > +     * set the type here. If that changes we should push this
> > +     * out so the si_type is passed in by callers.
> > +     */
> > +    info->si_code = deposit32(info->si_code, 16, 16, QEMU_SI_FAULT);
> > +
> >      ts->sync_signal.info = *info;
> >      ts->sync_signal.pending = sig;
> >      /* signal that a new signal is pending */
> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > index 34af15a..124754f 100644
> > --- a/linux-user/syscall_defs.h
> > +++ b/linux-user/syscall_defs.h
> > @@ -673,6 +673,21 @@ typedef struct {
> >  
> >  #define TARGET_SI_PAD_SIZE ((TARGET_SI_MAX_SIZE - TARGET_SI_PREAMBLE_SIZE) / sizeof(int))
> >  
> > +/* Within QEMU the top 16 bits of si_code indicate which of the parts of
> > + * the union in target_siginfo is valid. This only applies between
> > + * host_to_target_siginfo_noswap() and tswap_siginfo(); it does not
> > + * appear either within host siginfo_t or in target_siginfo structures
> > + * which we get from the guest userspace program. (The Linux kernel
> > + * does a similar thing with using the top bits for its own internal
> > + * purposes but not letting them be visible to userspace.)
> > + */
> > +#define QEMU_SI_KILL 0
> > +#define QEMU_SI_TIMER 1
> > +#define QEMU_SI_POLL 2
> > +#define QEMU_SI_FAULT 3
> > +#define QEMU_SI_CHLD 4
> > +#define QEMU_SI_RT 5
> > +
> >  typedef struct target_siginfo {
> >  #ifdef TARGET_MIPS
> >  	int si_signo;
> > -- 
> > 1.9.1
> > 

Re: [Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

[Laurent Vivier]

Le 07/06/2016 à 23:08, Peter Maydell a écrit :
> On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>>
>> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>>> host_to_target_siginfo() is implemented by a combination of
>>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>>> The first of these two functions assumes that the target_siginfo_t
>>> it is writing to is correctly aligned, but the pointer passed
>>> into host_to_target_siginfo() is directly from the guest and
>>> might be misaligned. Use a local variable to avoid this problem.
>>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>>
>> You mean the pointer from the guest can not be correctly aligned for the
>> guest?
> 
> Might not be correctly aligned for the host (for that matter
> it might not be correctly aligned for the guest,
> if the guest is being malicious or buggy, but it's the
> host alignment we care about.)

Because of the "abi_ulong _addr", I think this structure is always
aligned for the guest.

Laurent

Re: [Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

[Peter Maydell]

On 8 June 2016 at 10:29, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 07/06/2016 à 23:08, Peter Maydell a écrit :
>> On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>>
>>> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>>>> host_to_target_siginfo() is implemented by a combination of
>>>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>>>> The first of these two functions assumes that the target_siginfo_t
>>>> it is writing to is correctly aligned, but the pointer passed
>>>> into host_to_target_siginfo() is directly from the guest and
>>>> might be misaligned. Use a local variable to avoid this problem.
>>>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>>>
>>> You mean the pointer from the guest can not be correctly aligned for the
>>> guest?
>>
>> Might not be correctly aligned for the host (for that matter
>> it might not be correctly aligned for the guest,
>> if the guest is being malicious or buggy, but it's the
>> host alignment we care about.)
>
> Because of the "abi_ulong _addr", I think this structure is always
> aligned for the guest.

No, because the address of the structure in guest memory comes from
a pointer passed to us by the guest. The guest could pass any value
at all that it likes for that pointer including one that is arbitrarily
misaligned. I think it's pretty much always a bug to do a direct
access to a structure field that's in guest memory, though we do
it a fair bit and get away with it because x86-64 doesn't have
alignment restrictions.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Peter Maydell]

On 8 June 2016 at 07:30, Riku Voipio <riku.voipio@iki.fi> wrote:
> At least on Debian jessie, this blows up a selection of architectures:
>
> /home/voipio/linaro/qemu/linux-user/signal.c: In function ‘host_to_target_siginfo’:
> /home/voipio/linaro/qemu/linux-user/signal.c:387:10: error: ‘tgt_tmp._sifields._sigchld._stime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._stime,
>           ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._stime’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> /home/voipio/linaro/qemu/linux-user/signal.c:385:10: error: ‘tgt_tmp._sifields._sigchld._utime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._utime,
>           ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._utime’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> /home/voipio/linaro/qemu/linux-user/signal.c:383:10: error: ‘tgt_tmp._sifields._sigchld._status’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          __put_user(info->_sifields._sigchld._status,
>           ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._status’ was declared here
>      target_siginfo_t tgt_tmp;
>                       ^
> cc1: all warnings being treated as errors
>
> These appear to be the architectures where setup_rt_frame isn't implemented.

So as far as I can tell this is a combination of:
 * without setup_rt_frame() the compiler makes different decisions
   about whether to inline tswap_siginfo() into host_to_target_siginfo()
   [you can provoke it on all targets by marking tswap_siginfo 'inline']
 * gcc not being able to figure out that the _sigchld fields of the union
   are only read in the tswap_siginfo() switch if they were set in the
   host_to_target_siginfo_noswap() switch (likely because the type info
   is pushed in and out of the top 16 bits of the si_code field)

The simplest fix seems to be to add this to the top of
host_to_target_siginfo_noswap():

+    /* This memset serves two purposes:
+     * (1) ensure we don't leak random junk to the guest later
+     * (2) placate false positives from gcc about fields
+     *     being used uninitialized if it chooses to inline both this
+     *     function and tswap_siginfo() into host_to_target_siginfo().
+     */
+    memset(tinfo->_sifields._pad, 0, sizeof(tinfo->_sifields._pad));

I have no idea why gcc only complains about the _sigchld fields and
not any others, though.

thanks
-- PMM