| Message ID | 1441497448-32489-11-git-send-email-T.E.Baldwin99@members.leeds.ac.uk |
|---|---|
| State | New |
| Headers | show |
On 6 September 2015 at 00:57, Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk> wrote: > Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk> > --- > > Works without signals, but my signal test case > crashes with or without my changes. > > linux-user/main.c | 14 +++++++++----- > linux-user/microblaze/syscall.h | 2 ++ > linux-user/signal.c | 2 +- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index d47e33f..3eacc9c 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -2911,14 +2911,14 @@ void cpu_loop(CPUMBState *env) > queue_signal(env, info.si_signo, &info); > } > break; > - case EXCP_INTERRUPT: > - /* just indicate that signals should be handled asap */ > - break; > + case EXCP_INTERRUPT: > + /* just indicate that signals should be handled asap */ > + break; > case EXCP_BREAK: > /* Return address is 4 bytes after the call. */ > env->regs[14] += 4; > env->sregs[SR_PC] = env->regs[14]; > - ret = do_syscall(env, > + ret = do_syscall(env, > env->regs[12], > env->regs[5], > env->regs[6], > @@ -2927,7 +2927,11 @@ void cpu_loop(CPUMBState *env) > env->regs[9], > env->regs[10], > 0, 0); > - env->regs[3] = ret; > + if (ret == -TARGET_ERESTARTSYS) { > + env->sregs[SR_PC] -= 4; This isn't going to cleanly undo the changes to regs[14] and sregs[SR_PC] that we do on entry, so I think the restart isn't going to work right. > + } else if (ret != -TARGET_QEMU_ESIGRETURN) { > + env->regs[3] = ret; > + } > break; > case EXCP_HW_EXCP: > env->regs[17] = env->sregs[SR_PC] + 4; thanks -- PMM
Hi Edgar -- I'm just looking back at these signal handling race condition fix patches, and with this one I have a confusion about the Microblaze Linux syscall code that I hope you can clear up for me. Looking at the kernel entry.S code it looks to me like the way syscalls work on microblaze is: * syscall insn is brki r14 * the insn itself saves the PC of the brki into r14 * on entry the kernel advances r14 by 4 to skip the brki * then SAVE_REGS saves r14 into the 'PC' slot in the pt_regs struct * for syscall restart handle_restart() may wind the PC value in the pt_regs back by 4 * in any case, on syscall exit we pull the PC value out of pt_regs into r14, and do a return with rtbd r14, 0 I think what this implies is that: * r14 is a "used by the kernel, may be corrupted at any time, not to be touched by userspace" register * on exit from a syscall PC and r14 are always the same * this includes do_sigreturn, ie "taking a signal" is one of the things that can corrupt r14 Is that right? (For context, the original patch is this one: http://patchwork.ozlabs.org/patch/514879/ and I now suspect my review comments at the time to be wrong.) thanks -- PMM
On Thu, Mar 03, 2016 at 08:15:13PM +0000, Peter Maydell wrote: > Hi Edgar -- I'm just looking back at these signal handling > race condition fix patches, and with this one I have a confusion > about the Microblaze Linux syscall code that I hope you can > clear up for me. > > Looking at the kernel entry.S code it looks to me like > the way syscalls work on microblaze is: > * syscall insn is brki r14 > * the insn itself saves the PC of the brki into r14 > * on entry the kernel advances r14 by 4 to skip the brki > * then SAVE_REGS saves r14 into the 'PC' slot in the pt_regs > struct > * for syscall restart handle_restart() may wind the PC > value in the pt_regs back by 4 > * in any case, on syscall exit we pull the PC value out of > pt_regs into r14, and do a return with rtbd r14, 0 Yes, that sounds right. > > I think what this implies is that: > * r14 is a "used by the kernel, may be corrupted at any > time, not to be touched by userspace" register Yes. r14 is not really usable by user-space, interrupts will for example clobber r14 at any time aswell. > * on exit from a syscall PC and r14 are always the same Yes that's how it works but as far as user-space is concerned r14 may have any value at any time as it's not really observable in a safe way. > * this includes do_sigreturn, ie "taking a signal" is one > of the things that can corrupt r14 Yes. > > Is that right? Yes, I think so. > (For context, the original patch is this one: > http://patchwork.ozlabs.org/patch/514879/ > and I now suspect my review comments at the time to be wrong.) I see. Functionally I think the patch is OK. It seems to have some whitespace fixes mixed with functional changes (nitpick). Either way: Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Best regards, Edgar
On 4 March 2016 at 00:27, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Thu, Mar 03, 2016 at 08:15:13PM +0000, Peter Maydell wrote: >> Hi Edgar -- I'm just looking back at these signal handling >> race condition fix patches, and with this one I have a confusion >> about the Microblaze Linux syscall code that I hope you can >> clear up for me. >> >> Looking at the kernel entry.S code it looks to me like >> the way syscalls work on microblaze is: > Yes, that sounds right. Thanks for the confirmation. >> (For context, the original patch is this one: >> http://patchwork.ozlabs.org/patch/514879/ >> and I now suspect my review comments at the time to be wrong.) > > I see. Functionally I think the patch is OK. It seems to have > some whitespace fixes mixed with functional changes (nitpick). It also fixes a bug in do_sigreturn -- you'll notice that previously we were returning env->regs[10] and so would corrupt the guest r3 with the guest r10 value. Switching to using -TARGET_QEMU_ESIGRETURN avoids that. > Either way: > > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> (I'm going to add a brief comment about why not updating r14 is ok.) Thanks -- PMM
diff --git a/linux-user/main.c b/linux-user/main.c index d47e33f..3eacc9c 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -2911,14 +2911,14 @@ void cpu_loop(CPUMBState *env) queue_signal(env, info.si_signo, &info); } break; - case EXCP_INTERRUPT: - /* just indicate that signals should be handled asap */ - break; + case EXCP_INTERRUPT: + /* just indicate that signals should be handled asap */ + break; case EXCP_BREAK: /* Return address is 4 bytes after the call. */ env->regs[14] += 4; env->sregs[SR_PC] = env->regs[14]; - ret = do_syscall(env, + ret = do_syscall(env, env->regs[12], env->regs[5], env->regs[6], @@ -2927,7 +2927,11 @@ void cpu_loop(CPUMBState *env) env->regs[9], env->regs[10], 0, 0); - env->regs[3] = ret; + if (ret == -TARGET_ERESTARTSYS) { + env->sregs[SR_PC] -= 4; + } else if (ret != -TARGET_QEMU_ESIGRETURN) { + env->regs[3] = ret; + } break; case EXCP_HW_EXCP: env->regs[17] = env->sregs[SR_PC] + 4; diff --git a/linux-user/microblaze/syscall.h b/linux-user/microblaze/syscall.h index 3c1ed27..c38e700 100644 --- a/linux-user/microblaze/syscall.h +++ b/linux-user/microblaze/syscall.h @@ -54,3 +54,5 @@ struct target_pt_regs { #define TARGET_MLOCKALL_MCL_FUTURE 2 #endif + +#define TARGET_USE_ERESTARTSYS 1 diff --git a/linux-user/signal.c b/linux-user/signal.c index e432f97..abc7e30 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -3636,7 +3636,7 @@ long do_sigreturn(CPUMBState *env) env->regs[14] = env->sregs[SR_PC]; unlock_user_struct(frame, frame_addr, 0); - return env->regs[10]; + return -TARGET_QEMU_ESIGRETURN; badframe: force_sig(TARGET_SIGSEGV); }
Signed-off-by: Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk> --- Works without signals, but my signal test case crashes with or without my changes. linux-user/main.c | 14 +++++++++----- linux-user/microblaze/syscall.h | 2 ++ linux-user/signal.c | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-)