Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753653AbaKCVkf (ORCPT ); Mon, 3 Nov 2014 16:40:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35818 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751428AbaKCVke (ORCPT ); Mon, 3 Nov 2014 16:40:34 -0500 Message-ID: <5457F647.7020208@redhat.com> Date: Mon, 03 Nov 2014 21:40:23 +0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Oleg Nesterov , Andrew Morton CC: Alexander Viro , Evan Teran , Jan Kratochvil , Roland McGrath , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal() References: <20141103201256.GA5213@redhat.com> <20141103201317.GA5221@redhat.com> In-Reply-To: <20141103201317.GA5221@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks a lot Oleg. Question - shouldn't ptrace tests be put in tools/testing/selftests/ptrace/ in the kernel tree nowadays? Thanks, Pedro Alves On 11/03/2014 08:13 PM, Oleg Nesterov wrote: > When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal() > clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set. > > If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets > X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent > PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the > wrong SIGTRAP. > > Test-case (needs -O2 to avoid prologue insns in signal handler): > > #include > #include > #include > #include > #include > #include > #include > > void handler(int n) > { > asm("nop"); > } > > int child(void) > { > assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0); > signal(SIGALRM, handler); > kill(getpid(), SIGALRM); > return 0x23; > } > > void *getip(int pid) > { > return (void*)ptrace(PTRACE_PEEKUSER, pid, > offsetof(struct user, regs.rip), 0); > } > > int main(void) > { > int pid, status; > > pid = fork(); > if (!pid) > return child(); > > assert(wait(&status) == pid); > assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM); > > assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0); > assert(wait(&status) == pid); > assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP); > assert((getip(pid) - (void*)handler) == 0); > > assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0); > assert(wait(&status) == pid); > assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP); > assert((getip(pid) - (void*)handler) == 1); > > assert(ptrace(PTRACE_CONT, pid, 0,0) == 0); > assert(wait(&status) == pid); > assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23); > > return 0; > } > > The last assert() fails because PTRACE_CONT wrongly triggers another > single-step and X86_EFLAGS_TF can't be cleared by debugger until the > tracee does sys_rt_sigreturn(). > > Change handle_signal() to do user_disable_single_step() if stepping, > we do not need to preserve TIF_SINGLESTEP because we are going to do > ptrace_notify(), and it is simply wrong to leak this bit. > > While at it, change the comment to explain why we also need to clear > TF unconditionally after setup_rt_frame(). > > Note: in the longer term we should probably change setup_sigcontext() > to use get_flags() and then just remove this user_disable_single_step(). > And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext() > which can set/clear TF, this needs another fix. > > Reported-by: Evan Teran > Reported-by: Pedro Alves > Signed-off-by: Oleg Nesterov > --- > arch/x86/kernel/signal.c | 22 +++++++++++----------- > 1 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > index ed37a76..9d3a15b 100644 > --- a/arch/x86/kernel/signal.c > +++ b/arch/x86/kernel/signal.c > @@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs) > static void > handle_signal(struct ksignal *ksig, struct pt_regs *regs) > { > - bool failed; > + bool stepping, failed; > + > /* Are we from a system call? */ > if (syscall_get_nr(current, regs) >= 0) { > /* If so, check system call restarting.. */ > @@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) > } > > /* > - * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF > - * flag so that register information in the sigcontext is correct. > + * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now > + * so that register information in the sigcontext is correct and > + * then notify the tracer before entering the signal handler. > */ > - if (unlikely(regs->flags & X86_EFLAGS_TF) && > - likely(test_and_clear_thread_flag(TIF_FORCED_TF))) > - regs->flags &= ~X86_EFLAGS_TF; > + stepping = test_thread_flag(TIF_SINGLESTEP); > + if (stepping) > + user_disable_single_step(current); > > failed = (setup_rt_frame(ksig, regs) < 0); > if (!failed) { > @@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) > * it might disable possible debug exception from the > * signal handler. > * > - * Clear TF when entering the signal handler, but > - * notify any tracer that was single-stepping it. > - * The tracer may want to single-step inside the > - * handler too. > + * Clear TF for the case when it wasn't set by debugger to > + * avoid the recursive send_sigtrap() in SIGTRAP handler. > */ > regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF); > /* > @@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) > if (used_math()) > drop_init_fpu(current); > } > - signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP)); > + signal_setup_done(failed, ksig, stepping); > } > > #ifdef CONFIG_X86_32 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/