[PATCH RFC 0/1] linux-user/s390x: save/restore condition code state during signal handling

[PATCH RFC 0/1] linux-user/s390x: save/restore condition code state during signal handling

[Jonathan Albrecht]

Peter Bao <ruixin.bao@ibm.com> and I have been looking at some issues with
qemu user mode x86_64 host/s390x guest when running go1.14+ executables.
From the qemu cpu traces, it looks like the condition code is not restored
after a signal handler is run. This affects go1.14+ because it uses signals
heavily to implement the async preempt feature in the goroutine scheduler
that was added in go1.14.

This patch tries save and restore the condition code related fields when
handling a signal. We're submitting it as an RFC since we're new to qemu and
not sure if this is s390x specific. We have some C code (s390x specific)
that reproduces the issue and can try to add it as a unit test.

Jonathan Albrecht (1):
  linux-user/s390x: save/restore condition code state during signal
    handling

 linux-user/s390x/signal.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

-- 
2.31.1

[PATCH RFC 1/1] linux-user/s390x: save/restore condition code state during signal handling

[Jonathan Albrecht]

When handling a signal, the signal handler may have clobbered the
condition code set by the interrupted thread.

Signed-off-by: Jonathan Albrecht <jonathan.albrecht@linux.vnet.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1886793
Buglink: https://bugs.launchpad.net/qemu/+bug/1893040
---
 linux-user/s390x/signal.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ef136dae33..03cacb2829 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -65,6 +65,10 @@ typedef struct {
     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
     target_sigcontext sc;
     target_sigregs sregs;
+    uint32_t scc_op;
+    uint64_t scc_src;
+    uint64_t scc_dst;
+    uint64_t scc_vr;
     int signo;
     target_sigregs_ext sregs_ext;
     uint16_t retcode;
@@ -86,6 +90,10 @@ typedef struct {
     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
     uint16_t retcode;
     struct target_siginfo info;
+    uint32_t scc_op;
+    uint64_t scc_src;
+    uint64_t scc_dst;
+    uint64_t scc_vr;
     struct target_ucontext uc;
 } rt_sigframe;
 
@@ -224,6 +232,12 @@ void setup_frame(int sig, struct target_sigaction *ka,
     env->regs[5] = 0; /* FIXME: regs->int_parm_long */
     env->regs[6] = 0; /* FIXME: current->thread.last_break */
 
+    /* Some programs could clobber the condition code, so save it here */
+    __put_user(env->cc_op, &frame->scc_op);
+    __put_user(env->cc_src, &frame->scc_src);
+    __put_user(env->cc_dst, &frame->scc_dst);
+    __put_user(env->cc_vr, &frame->scc_vr);
+
     unlock_user_struct(frame, frame_addr, 1);
 }
 
@@ -273,6 +287,12 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     save_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
     tswap_sigset(&frame->uc.tuc_sigmask, set);
 
+    /* Some programs could clobber the condition code, so save it here */
+    __put_user(env->cc_op, &frame->scc_op);
+    __put_user(env->cc_src, &frame->scc_src);
+    __put_user(env->cc_dst, &frame->scc_dst);
+    __put_user(env->cc_vr, &frame->scc_vr);
+
     /* Set up registers for signal handler */
     env->regs[14] = restorer;
     env->regs[15] = frame_addr;
@@ -285,6 +305,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
     env->regs[4] = frame_addr + offsetof(typeof(*frame), uc);
     env->regs[5] = 0; /* FIXME: current->thread.last_break */
+
+    /* QUESTION: Was there a missing unlock user struct here? */
+    unlock_user_struct(frame, frame_addr, 1);
+    return;
 }
 
 static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
@@ -350,6 +374,12 @@ long do_sigreturn(CPUS390XState *env)
     restore_sigregs(env, &frame->sregs);
     restore_sigregs_ext(env, &frame->sregs_ext);
 
+    /* restore the condition code */
+    __get_user(env->cc_op, &frame->scc_op);
+    __get_user(env->cc_src, &frame->scc_src);
+    __get_user(env->cc_dst, &frame->scc_dst);
+    __get_user(env->cc_vr, &frame->scc_vr);
+
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
 }
@@ -372,6 +402,11 @@ long do_rt_sigreturn(CPUS390XState *env)
     restore_sigregs(env, &frame->uc.tuc_mcontext);
     restore_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
 
+    /* restore the condition code */
+    __get_user(env->cc_op, &frame->scc_op);
+    __get_user(env->cc_src, &frame->scc_src);
+    __get_user(env->cc_dst, &frame->scc_dst);
+    __get_user(env->cc_vr, &frame->scc_vr);
     target_restore_altstack(&frame->uc.tuc_stack, env);
 
     unlock_user_struct(frame, frame_addr, 0);
-- 
2.31.1

Re: [PATCH RFC 1/1] linux-user/s390x: save/restore condition code state during signal handling

[Richard Henderson]

On 6/10/21 11:58 AM, Jonathan Albrecht wrote:
> @@ -65,6 +65,10 @@ typedef struct {
>       uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
>       target_sigcontext sc;
>       target_sigregs sregs;
> +    uint32_t scc_op;
> +    uint64_t scc_src;
> +    uint64_t scc_dst;
> +    uint64_t scc_vr;
>       int signo;

Nope.  The layout of the stack frame is fixed by the kernel. Moreover, all of 
these variables are internal to qemu; you should only be exposing architectural 
state.

The bug is in save_sigregs:

>     __put_user(env->psw.mask, &sregs->regs.psw.mask);

This should use get_psw_mask(), currently declared in target/s390x/internal.h 
instead of cpu.h.

and correspondingly, in restore_sigregs:

>     __get_user(env->psw.mask, &sc->regs.psw.mask);
>     __get_user(env->psw.addr, &sc->regs.psw.addr);

this should use load_psw, and in addition it should not be allowing completely 
arbitrary changes to psw.mask.  From the kernel:

>         /* Use regs->psw.mask instead of PSW_USER_BITS to preserve PER bit. */
>         regs->psw.mask = (regs->psw.mask & ~(PSW_MASK_USER | PSW_MASK_RI)) |
>                 (user_sregs.regs.psw.mask & (PSW_MASK_USER | PSW_MASK_RI));
>         /* Check for invalid user address space control. */
>         if ((regs->psw.mask & PSW_MASK_ASC) == PSW_ASC_HOME)
>                 regs->psw.mask = PSW_ASC_PRIMARY |
>                         (regs->psw.mask & ~PSW_MASK_ASC);
>         /* Check for invalid amode */
>         if (regs->psw.mask & PSW_MASK_EA)
>                 regs->psw.mask |= PSW_MASK_BA;



r~