There are actually two problems here:
1. We absolutely cannot enable IRQs in case the fault was caused by
kmemcheck.
2. We cannot enable preemption and then return from the debug handler
without disabling preemption afterwards.
The problem seems to be a merge fallout between three commits:
commit 3d2a71a596bd9c761c8487a2178e95f8a61da083
Author: Alexander van Heukelum <[email protected]>
Date: Tue Sep 30 18:41:37 2008 +0200
x86, traps: converge do_debug handlers
commit 08d68323d1f0c34452e614263b212ca556dae47f
Author: K.Prasad <[email protected]>
Date: Mon Jun 1 23:44:08 2009 +0530
hw-breakpoints: modifying generic debug exception to use thread-specific deb
commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
Author: Vegard Nossum <[email protected]>
Date: Fri Apr 4 00:53:23 2008 +0200
x86: add hooks for kmemcheck
I encourage the kprobe developers to check whether their code is correct
as it stands in current tip/master. Also, comments on this particular
change is welcome.
Reported-by: Ingo Molnar <[email protected]>
Cc: Alexander van Heukelum <[email protected]>
Cc: K.Prasad <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Pekka Enberg <[email protected]>
Signed-off-by: Vegard Nossum <[email protected]>
---
arch/x86/kernel/traps.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c8a7f87..a898c6b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -550,6 +550,10 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
SIGTRAP) == NOTIFY_STOP)
return;
+ /* Catch kmemcheck conditions first of all! */
+ if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
+ return;
+
/* It's safe to allow irq's after DR6 has been saved */
preempt_conditional_sti(regs);
@@ -559,10 +563,6 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
return;
}
- /* Catch kmemcheck conditions first of all! */
- if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
- return;
-
/*
* Single-stepping through system calls: ignore any exceptions in
* kernel space, but re-enable TF when returning to user mode.
--
1.6.0.6
* Vegard Nossum <[email protected]> wrote:
> There are actually two problems here:
>
> 1. We absolutely cannot enable IRQs in case the fault was caused by
> kmemcheck.
>
> 2. We cannot enable preemption and then return from the debug handler
> without disabling preemption afterwards.
>
> The problem seems to be a merge fallout between three commits:
>
> commit 3d2a71a596bd9c761c8487a2178e95f8a61da083
> Author: Alexander van Heukelum <[email protected]>
> Date: Tue Sep 30 18:41:37 2008 +0200
>
> x86, traps: converge do_debug handlers
>
> commit 08d68323d1f0c34452e614263b212ca556dae47f
> Author: K.Prasad <[email protected]>
> Date: Mon Jun 1 23:44:08 2009 +0530
>
> hw-breakpoints: modifying generic debug exception to use thread-specific deb
>
> commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
> Author: Vegard Nossum <[email protected]>
> Date: Fri Apr 4 00:53:23 2008 +0200
>
> x86: add hooks for kmemcheck
>
> I encourage the kprobe developers to check whether their code is correct
> as it stands in current tip/master. Also, comments on this particular
> change is welcome.
>
> Reported-by: Ingo Molnar <[email protected]>
> Cc: Alexander van Heukelum <[email protected]>
> Cc: K.Prasad <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Signed-off-by: Vegard Nossum <[email protected]>
> ---
> arch/x86/kernel/traps.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c8a7f87..a898c6b 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -550,6 +550,10 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> SIGTRAP) == NOTIFY_STOP)
> return;
>
> + /* Catch kmemcheck conditions first of all! */
> + if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
> + return;
> +
> /* It's safe to allow irq's after DR6 has been saved */
> preempt_conditional_sti(regs);
>
> @@ -559,10 +563,6 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> return;
> }
>
> - /* Catch kmemcheck conditions first of all! */
> - if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
> - return;
> -
> /*
> * Single-stepping through system calls: ignore any exceptions in
> * kernel space, but re-enable TF when returning to user mode.
Yeah - this could solve the crash i saw. Mind sending a pull request
too?
Ingo
2009/6/7 Ingo Molnar <[email protected]>:
>
> * Vegard Nossum <[email protected]> wrote:
>
>> There are actually two problems here:
>>
>> 1. We absolutely cannot enable IRQs in case the fault was caused by
>> kmemcheck.
>>
>> 2. We cannot enable preemption and then return from the debug handler
>> without disabling preemption afterwards.
>>
>> The problem seems to be a merge fallout between three commits:
>>
>> commit 3d2a71a596bd9c761c8487a2178e95f8a61da083
>> Author: Alexander van Heukelum <[email protected]>
>> Date: Tue Sep 30 18:41:37 2008 +0200
>>
>> x86, traps: converge do_debug handlers
>>
>> commit 08d68323d1f0c34452e614263b212ca556dae47f
>> Author: K.Prasad <[email protected]>
>> Date: Mon Jun 1 23:44:08 2009 +0530
>>
>> hw-breakpoints: modifying generic debug exception to use thread-specific deb
>>
>> commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
>> Author: Vegard Nossum <[email protected]>
>> Date: Fri Apr 4 00:53:23 2008 +0200
>>
>> x86: add hooks for kmemcheck
>>
>> I encourage the kprobe developers to check whether their code is correct
>> as it stands in current tip/master. Also, comments on this particular
>> change is welcome.
>>
>> Reported-by: Ingo Molnar <[email protected]>
>> Cc: Alexander van Heukelum <[email protected]>
>> Cc: K.Prasad <[email protected]>
>> Cc: Alan Stern <[email protected]>
>> Cc: Frederic Weisbecker <[email protected]>
>> Cc: Pekka Enberg <[email protected]>
>> Signed-off-by: Vegard Nossum <[email protected]>
>> ---
>> arch/x86/kernel/traps.c | 8 ++++----
>> 1 files changed, 4 insertions(+), 4 deletions(-)
>
>>
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index c8a7f87..a898c6b 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -550,6 +550,10 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>> SIGTRAP) == NOTIFY_STOP)
>> return;
>>
>> + /* Catch kmemcheck conditions first of all! */
>> + if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
>> + return;
>> +
>> /* It's safe to allow irq's after DR6 has been saved */
>> preempt_conditional_sti(regs);
>>
>> @@ -559,10 +563,6 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>> return;
>> }
>>
>> - /* Catch kmemcheck conditions first of all! */
>> - if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
>> - return;
>> -
>> /*
>> * Single-stepping through system calls: ignore any exceptions in
>> * kernel space, but re-enable TF when returning to user mode.
>
> Yeah - this could solve the crash i saw. Mind sending a pull request
> too?
It did solve it, I tested it :-D
This patch was against tip/master; tip/kmemcheck does not have the
problem. I think it might have been introduced in this merge:
commit 85b9b2801e46a147330b8a0f321bc40342ff5b4c
Merge: bf8d9b3... 7387400...
Author: Ingo Molnar <[email protected]>
Date: Thu Jun 4 13:56:43 2009 +0200
Merge branch 'tracing/hw-breakpoints'
Conflicts:
arch/x86/Kconfig
arch/x86/kernel/traps.c
kernel/Makefile
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
* Vegard Nossum <[email protected]> wrote:
> 2009/6/7 Ingo Molnar <[email protected]>:
> >
> > * Vegard Nossum <[email protected]> wrote:
> >
> >> There are actually two problems here:
> >>
> >> 1. We absolutely cannot enable IRQs in case the fault was caused by
> >> ? ?kmemcheck.
> >>
> >> 2. We cannot enable preemption and then return from the debug handler
> >> ? ?without disabling preemption afterwards.
> >>
> >> The problem seems to be a merge fallout between three commits:
> >>
> >> commit 3d2a71a596bd9c761c8487a2178e95f8a61da083
> >> Author: Alexander van Heukelum <[email protected]>
> >> Date: ? Tue Sep 30 18:41:37 2008 +0200
> >>
> >> ? ? x86, traps: converge do_debug handlers
> >>
> >> commit 08d68323d1f0c34452e614263b212ca556dae47f
> >> Author: K.Prasad <[email protected]>
> >> Date: ? Mon Jun 1 23:44:08 2009 +0530
> >>
> >> ? ? hw-breakpoints: modifying generic debug exception to use thread-specific deb
> >>
> >> commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
> >> Author: Vegard Nossum <[email protected]>
> >> Date: ? Fri Apr 4 00:53:23 2008 +0200
> >>
> >> ? ? x86: add hooks for kmemcheck
> >>
> >> I encourage the kprobe developers to check whether their code is correct
> >> as it stands in current tip/master. Also, comments on this particular
> >> change is welcome.
> >>
> >> Reported-by: Ingo Molnar <[email protected]>
> >> Cc: Alexander van Heukelum <[email protected]>
> >> Cc: K.Prasad <[email protected]>
> >> Cc: Alan Stern <[email protected]>
> >> Cc: Frederic Weisbecker <[email protected]>
> >> Cc: Pekka Enberg <[email protected]>
> >> Signed-off-by: Vegard Nossum <[email protected]>
> >> ---
> >> ?arch/x86/kernel/traps.c | ? ?8 ++++----
> >> ?1 files changed, 4 insertions(+), 4 deletions(-)
> >
> >>
> >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> >> index c8a7f87..a898c6b 100644
> >> --- a/arch/x86/kernel/traps.c
> >> +++ b/arch/x86/kernel/traps.c
> >> @@ -550,6 +550,10 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SIGTRAP) == NOTIFY_STOP)
> >> ? ? ? ? ? ? ? return;
> >>
> >> + ? ? /* Catch kmemcheck conditions first of all! */
> >> + ? ? if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
> >> + ? ? ? ? ? ? return;
> >> +
> >> ? ? ? /* It's safe to allow irq's after DR6 has been saved */
> >> ? ? ? preempt_conditional_sti(regs);
> >>
> >> @@ -559,10 +563,6 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> >> ? ? ? ? ? ? ? return;
> >> ? ? ? }
> >>
> >> - ? ? /* Catch kmemcheck conditions first of all! */
> >> - ? ? if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
> >> - ? ? ? ? ? ? return;
> >> -
> >> ? ? ? /*
> >> ? ? ? ?* Single-stepping through system calls: ignore any exceptions in
> >> ? ? ? ?* kernel space, but re-enable TF when returning to user mode.
> >
> > Yeah - this could solve the crash i saw. Mind sending a pull request
> > too?
>
> It did solve it, I tested it :-D
>
> This patch was against tip/master; tip/kmemcheck does not have the
> problem. I think it might have been introduced in this merge:
>
> commit 85b9b2801e46a147330b8a0f321bc40342ff5b4c
> Merge: bf8d9b3... 7387400...
> Author: Ingo Molnar <[email protected]>
> Date: Thu Jun 4 13:56:43 2009 +0200
>
> Merge branch 'tracing/hw-breakpoints'
>
> Conflicts:
> arch/x86/Kconfig
> arch/x86/kernel/traps.c
> kernel/Makefile
Ah. kmemcheck + hw-breakpoints interaction. Fun.
Ingo
On Sun, Jun 07, 2009 at 06:26:52PM +0200, Vegard Nossum wrote:
> There are actually two problems here:
>
> 1. We absolutely cannot enable IRQs in case the fault was caused by
> kmemcheck.
>
> 2. We cannot enable preemption and then return from the debug handler
> without disabling preemption afterwards.
>
> The problem seems to be a merge fallout between three commits:
>
> commit 3d2a71a596bd9c761c8487a2178e95f8a61da083
> Author: Alexander van Heukelum <[email protected]>
> Date: Tue Sep 30 18:41:37 2008 +0200
>
> x86, traps: converge do_debug handlers
>
> commit 08d68323d1f0c34452e614263b212ca556dae47f
> Author: K.Prasad <[email protected]>
> Date: Mon Jun 1 23:44:08 2009 +0530
>
> hw-breakpoints: modifying generic debug exception to use thread-specific deb
>
> commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
> Author: Vegard Nossum <[email protected]>
> Date: Fri Apr 4 00:53:23 2008 +0200
>
> x86: add hooks for kmemcheck
>
> I encourage the kprobe developers to check whether their code is correct
> as it stands in current tip/master. Also, comments on this particular
> change is welcome.
I see no problem with this change wrt kprobes, since the changes happen
after the notify_die, by which time, kprobes would've returned
NOTIFY_DONE since the per-cpu current_kprobe == NULL.
Ananth
Ananth N Mavinakayanahalli wrote:
> On Sun, Jun 07, 2009 at 06:26:52PM +0200, Vegard Nossum wrote:
>> commit 787ecfaa503dc63ff1831ddc74b15dad49bace1d
>> Author: Vegard Nossum <[email protected]>
>> Date: Fri Apr 4 00:53:23 2008 +0200
>>
>> x86: add hooks for kmemcheck
>>
>> I encourage the kprobe developers to check whether their code is correct
>> as it stands in current tip/master. Also, comments on this particular
>> change is welcome.
>
> I see no problem with this change wrt kprobes, since the changes happen
> after the notify_die, by which time, kprobes would've returned
> NOTIFY_DONE since the per-cpu current_kprobe == NULL.
BTW, what will happen if kmemchecked page is accessed from
kprobe's single-stepping instruction? :-)
If a pagefault occurs before (kprobe's)single-step is done,
may kmemcheck's single-step handler be ignored by kprobe's
single-step handler? If it's true, I think kmemcheck hook
should be done before notify_die. (and add __kprobes to some
kmemcheck functions)
Thank you,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: [email protected]