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 <richard@xxxxxx>
> ---
> 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 <james.hogan@xxxxxxxxxx>
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,
Attachment:
signature.asc
Description: OpenPGP digital signature
