Prev: [PATCH 2/3] Convert arm to arch_gettimeoffset()
Next: [GIT -maybe- PULL] notification - fanotify
From: Oleg Nesterov on 17 Dec 2009 20:10
Hi.

do_debug() is obviously wrong wrt PTRACE_SINGLESTEP/TIF_SINGLESTEP, no?

Afaics this was broken by

hw-breakpoints: modifying generic debug exception to use thread-specific debug registers
commit 08d68323d1f0c34452e614263b212ca556dae47f

To verify, the "patch" below fixes the stepping for me, not sure what
is the proper fix...

Oleg.

--- arch/x86/kernel/traps.c~ 2009-12-18 00:20:49.000000000 +0100
+++ arch/x86/kernel/traps.c 2009-12-18 01:44:05.000000000 +0100
@@ -575,7 +575,7 @@ dotraplinkage void __kprobes do_debug(st
regs->flags &= ~X86_EFLAGS_TF;
}
si_code = get_si_code(tsk->thread.debugreg6);
- if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
+// if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
send_sigtrap(tsk, regs, error_code, si_code);
preempt_conditional_cli(regs);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Roland McGrath on 17 Dec 2009 20:50
Comparing to the old (2.6.32) logic, I think it might be this (untested).
I also note this is the sole use of get_si_code, seems like it should
just be rolled in here.


Thanks,
Roland


diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 3339917..16a88f5 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -530,7 +530,6 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
{
struct task_struct *tsk = current;
unsigned long dr6;
- int si_code;

get_debugreg(dr6, 6);

@@ -569,14 +568,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
* We already checked v86 mode above, so we can check for kernel mode
* by just checking the CPL of CS.
*/
+ dr6 = tsk->thread.debugreg6;
if ((dr6 & DR_STEP) && !user_mode(regs)) {
tsk->thread.debugreg6 &= ~DR_STEP;
set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
regs->flags &= ~X86_EFLAGS_TF;
+ } else if (dr6 & (DR_STEP | DR_TRAP_BITS)) {
+ send_sigtrap(tsk, regs, error_code, get_si_code(dr6));
}
- si_code = get_si_code(tsk->thread.debugreg6);
- if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
- send_sigtrap(tsk, regs, error_code, si_code);
+
preempt_conditional_cli(regs);

return;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 17 Dec 2009 21:20
On 12/17, Roland McGrath wrote:
>
> Comparing to the old (2.6.32) logic, I think it might be this (untested).
> I also note this is the sole use of get_si_code, seems like it should
> just be rolled in here.

Well, it is too late for me to even try to read this patch ;)

but...

> @@ -569,14 +568,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> * We already checked v86 mode above, so we can check for kernel mode
> * by just checking the CPL of CS.
> */
> + dr6 = tsk->thread.debugreg6;

why? we have "tsk->thread.debugreg6 = dr6" above

> if ((dr6 & DR_STEP) && !user_mode(regs)) {
> tsk->thread.debugreg6 &= ~DR_STEP;
> set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
> regs->flags &= ~X86_EFLAGS_TF;

this looks strange... we set TIF_SINGLESTEP but clear X86_EFLAGS_TF

> + } else if (dr6 & (DR_STEP | DR_TRAP_BITS)) {
> + send_sigtrap(tsk, regs, error_code, get_si_code(dr6));
> }
> - si_code = get_si_code(tsk->thread.debugreg6);
> - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> - send_sigtrap(tsk, regs, error_code, si_code);
> +

can't understand how this change can fix the problem. We should always
send SIGTRAP if the task returns to user-mode with X86_EFLAGS_TF?

OK. I blindly applied this patch, step-simple still fails.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 17 Dec 2009 22:00
On Fri, Dec 18, 2009 at 03:10:42AM +0100, Oleg Nesterov wrote:
> On 12/17, Roland McGrath wrote:
> >
> > Comparing to the old (2.6.32) logic, I think it might be this (untested).
> > I also note this is the sole use of get_si_code, seems like it should
> > just be rolled in here.
>
> Well, it is too late for me to even try to read this patch ;)
>
> but...
>
> > @@ -569,14 +568,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> > * We already checked v86 mode above, so we can check for kernel mode
> > * by just checking the CPL of CS.
> > */
> > + dr6 = tsk->thread.debugreg6;
>
> why? we have "tsk->thread.debugreg6 = dr6" above


Yeah.



> > if ((dr6 & DR_STEP) && !user_mode(regs)) {
> > tsk->thread.debugreg6 &= ~DR_STEP;
> > set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
> > regs->flags &= ~X86_EFLAGS_TF;
>
> this looks strange... we set TIF_SINGLESTEP but clear X86_EFLAGS_TF


Yep, I don't understand what happens here either. This logic
was there before the refactoring and the comment indicates we want
to ignore traps from kernel. Why do we set this flag in a random
thread?



> > + } else if (dr6 & (DR_STEP | DR_TRAP_BITS)) {
> > + send_sigtrap(tsk, regs, error_code, get_si_code(dr6));
> > }
> > - si_code = get_si_code(tsk->thread.debugreg6);
> > - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> > - send_sigtrap(tsk, regs, error_code, si_code);
> > +
>
> can't understand how this change can fix the problem. We should always
> send SIGTRAP if the task returns to user-mode with X86_EFLAGS_TF?
>
> OK. I blindly applied this patch, step-simple still fails.


Yep, that doesn't fix your problem but this patch makes sense
in that if we were not in user mode while the step occured,
we shouldn't send the signal.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 17 Dec 2009 22:10
On Fri, Dec 18, 2009 at 03:58:09AM +0100, Frederic Weisbecker wrote:
> Yep, that doesn't fix your problem but this patch makes sense
> in that if we were not in user mode while the step occured,
> we shouldn't send the signal.


That wouldn't happen actually, as we clear the STEP flag before.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH 2/3] Convert arm to arch_gettimeoffset()
Next: [GIT -maybe- PULL] notification - fanotify