Hi Richard, (cc'd linux-metag)
On 08/10/13 12:33, Richard Weinberger wrote: > Use the more generic functions get_signal() signal_setup_done() > for signal delivery. > > Signed-off-by: Richard Weinberger <[email protected]> > --- > arch/metag/kernel/signal.c | 55 > ++++++++++++++++++++-------------------------- > 1 file changed, 24 insertions(+), 31 deletions(-) > > diff --git a/arch/metag/kernel/signal.c b/arch/metag/kernel/signal.c > index 3be61cf..9fa27c2 100644 > --- a/arch/metag/kernel/signal.c > +++ b/arch/metag/kernel/signal.c > @@ -152,18 +152,18 @@ static void __user *get_sigframe(struct k_sigaction > *ka, unsigned long sp, > return (void __user *)sp; > } > > -static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info, > - sigset_t *set, struct pt_regs *regs) > +static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, > + struct pt_regs *regs) > { > struct rt_sigframe __user *frame; > - int err = -EFAULT; > + int err; > unsigned long code; > > - frame = get_sigframe(ka, regs->REG_SP, sizeof(*frame)); > + frame = get_sigframe(&ksig->ka, regs->REG_SP, sizeof(*frame)); > if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame))) > - goto out; > + return -EFAULT; > > - err = copy_siginfo_to_user(&frame->info, info); > + err = copy_siginfo_to_user(&frame->info, &ksig->info); > > /* Create the ucontext. */ > err |= __put_user(0, &frame->uc.uc_flags); > @@ -174,7 +174,7 @@ static int setup_rt_frame(int sig, struct k_sigaction > *ka, siginfo_t *info, > err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set)); > > if (err) > - goto out; > + return -EFAULT; > > /* Set up to return from userspace. */ > > @@ -187,15 +187,15 @@ static int setup_rt_frame(int sig, struct k_sigaction > *ka, siginfo_t *info, > err |= __put_user(code, (unsigned long __user *)(&frame->retcode[1])); > > if (err) > - goto out; > + return -EFAULT; > > /* Set up registers for signal handler */ > regs->REG_RTP = (unsigned long) frame->retcode; > regs->REG_SP = (unsigned long) frame + sizeof(*frame); > - regs->REG_ARG1 = sig; > + regs->REG_ARG1 = ksig->sig; > regs->REG_ARG2 = (unsigned long) &frame->info; > regs->REG_ARG3 = (unsigned long) &frame->uc; > - regs->REG_PC = (unsigned long) ka->sa.sa_handler; > + regs->REG_PC = (unsigned long) ksig->ka.sa.sa_handler; > > pr_debug("SIG deliver (%s:%d): sp=%p pc=%08x pr=%08x\n", > current->comm, current->pid, frame, regs->REG_PC, > @@ -205,24 +205,19 @@ static int setup_rt_frame(int sig, struct k_sigaction > *ka, siginfo_t *info, > * effective cache flush - directed rather than 'full flush'. > */ > flush_cache_sigtramp(regs->REG_RTP, sizeof(frame->retcode)); > -out: > - if (err) { > - force_sigsegv(sig, current); > - return -EFAULT; > - } > + > return 0; > } > > -static void handle_signal(unsigned long sig, siginfo_t *info, > - struct k_sigaction *ka, struct pt_regs *regs) > +static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) > { > sigset_t *oldset = sigmask_to_save(); > + int ret; > > /* Set up the stack frame */ > - if (setup_rt_frame(sig, ka, info, oldset, regs)) > - return; > + ret = setup_rt_frame(ksig, oldset, regs); > > - signal_delivered(sig, info, ka, regs, test_thread_flag(TIF_SINGLESTEP)); > + signal_setup_done(ret, ksig, test_thread_flag(TIF_SINGLESTEP)); > } > > /* > @@ -235,10 +230,8 @@ static void handle_signal(unsigned long sig, siginfo_t > *info, > static int do_signal(struct pt_regs *regs, int syscall) > { > unsigned int retval = 0, continue_addr = 0, restart_addr = 0; > - struct k_sigaction ka; > - siginfo_t info; > - int signr; > int restart = 0; > + struct ksignal ksig; > > /* > * By the end of rt_sigreturn the context describes the point that the > @@ -272,30 +265,30 @@ static int do_signal(struct pt_regs *regs, int syscall) > } > > /* > - * Get the signal to deliver. When running under ptrace, at this point > - * the debugger may change all our registers ... > - */ > - signr = get_signal_to_deliver(&info, &ka, regs, NULL); > - /* > * Depending on the signal settings we may need to revert the decision > * to restart the system call. But skip this if a debugger has chosen to > * restart at a different PC. > */ > if (regs->REG_PC != restart_addr) > restart = 0; You've reordered the point where a ptrace debugger can change the registers (get_signal_to_deliver) and the check of whether the PC has been changed by the debugger (REG_PC != restart_addr). I suggest this is changed to do something like 7e243643 (arm: switch to struct ksignal * passing) does, moving the above REG_PC != restart_addr check into the restart conditions below. > - if (signr > 0) { > + > + /* > + * Get the signal to deliver. When running under ptrace, at this point > + * the debugger may change all our registers ... > + */ > + if (get_signal(&ksig)) { > if (unlikely(restart)) { > if (retval == -ERESTARTNOHAND > || retval == -ERESTART_RESTARTBLOCK > || (retval == -ERESTARTSYS > - && !(ka.sa.sa_flags & SA_RESTART))) { > + && !(ksig.ka.sa.sa_flags & SA_RESTART))) { > regs->REG_RETVAL = -EINTR; > regs->REG_PC = continue_addr; > } > } > > /* Whee! Actually deliver the signal. */ > - handle_signal(signr, &info, &ka, regs); > + handle_signal(&ksig, regs); > return 0; > } > > How does the fixup patch below look? The rest looks good, so other than that problem: Acked-by: James Hogan <[email protected]> Thanks James diff --git a/arch/metag/kernel/signal.c b/arch/metag/kernel/signal.c index 9fa27c2..6f64ea2 100644 --- a/arch/metag/kernel/signal.c +++ b/arch/metag/kernel/signal.c @@ -265,19 +265,16 @@ static int do_signal(struct pt_regs *regs, int syscall) } /* + * Get the signal to deliver. When running under ptrace, at this point + * the debugger may change all our registers ... + * * Depending on the signal settings we may need to revert the decision * to restart the system call. But skip this if a debugger has chosen to * restart at a different PC. */ - if (regs->REG_PC != restart_addr) - restart = 0; - - /* - * Get the signal to deliver. When running under ptrace, at this point - * the debugger may change all our registers ... - */ if (get_signal(&ksig)) { - if (unlikely(restart)) { + /* Handler */ + if (unlikely(restart) && regs->REG_PC == restart_addr) { if (retval == -ERESTARTNOHAND || retval == -ERESTART_RESTARTBLOCK || (retval == -ERESTARTSYS @@ -289,19 +286,24 @@ static int do_signal(struct pt_regs *regs, int syscall) /* Whee! Actually deliver the signal. */ handle_signal(&ksig, regs); - return 0; - } - - /* Handlerless -ERESTART_RESTARTBLOCK re-enters via restart_syscall */ - if (unlikely(restart < 0)) - regs->REG_SYSCALL = __NR_restart_syscall; - - /* - * If there's no signal to deliver, we just put the saved sigmask back. - */ - restore_saved_sigmask(); + } else { + /* + * If there's no signal to deliver, we just put the saved + * sigmask back. + */ + restore_saved_sigmask(); - return restart; + /* + * Handlerless -ERESTART_RESTARTBLOCK re-enters via + * restart_syscall + */ + if (unlikely(restart) && regs->REG_PC == restart_addr) { + if (restart < 0) + regs->REG_SYSCALL = __NR_restart_syscall; + return restart; + } + } + return 0; } int do_work_pending(struct pt_regs *regs, unsigned int thread_flags,
signature.asc
Description: OpenPGP digital signature
