[PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Pratyush Anand]

uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
debug exception handler, so blacklist them for kprobing.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 kernel/events/uprobes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b7a525ab2083..206e594cb65e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -37,6 +37,7 @@
 #include <linux/percpu-rwsem.h>
 #include <linux/task_work.h>
 #include <linux/shmem_fs.h>
+#include <linux/kprobes.h>
 
 #include <linux/uprobes.h>
 
@@ -1997,6 +1998,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
 	set_thread_flag(TIF_UPROBE);
 	return 1;
 }
+NOKPROBE_SYMBOL(uprobe_pre_sstep_notifier);
 
 /*
  * uprobe_post_sstep_notifier gets called in interrupt context as part of notifier
@@ -2014,6 +2016,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
 	set_thread_flag(TIF_UPROBE);
 	return 1;
 }
+NOKPROBE_SYMBOL(uprobe_post_sstep_notifier);
 
 static struct notifier_block uprobe_exception_nb = {
 	.notifier_call		= arch_uprobe_exception_notify,
-- 
2.5.5

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Masami Hiramatsu]

On Tue,  2 Aug 2016 12:14:06 +0530
Pratyush Anand <panand@redhat.com> wrote:

> uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
> debug exception handler, so blacklist them for kprobing.

Actually, these exception notifers are kicked only if the debug exception
is not related to kprobes (at least on x86). In that case, we don't have
to take care about that. Or, would you hit any problem on it?

IOW, where do we have to prohibit kprobes are, the code path from where 
right after the breakpoint (debug) exception is occurred, to where right
before the kprobe is handled. After that, it should be safe.

Thank you,


> 
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  kernel/events/uprobes.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b7a525ab2083..206e594cb65e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -37,6 +37,7 @@
>  #include <linux/percpu-rwsem.h>
>  #include <linux/task_work.h>
>  #include <linux/shmem_fs.h>
> +#include <linux/kprobes.h>
>  
>  #include <linux/uprobes.h>
>  
> @@ -1997,6 +1998,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
>  	set_thread_flag(TIF_UPROBE);
>  	return 1;
>  }
> +NOKPROBE_SYMBOL(uprobe_pre_sstep_notifier);
>  
>  /*
>   * uprobe_post_sstep_notifier gets called in interrupt context as part of notifier
> @@ -2014,6 +2016,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
>  	set_thread_flag(TIF_UPROBE);
>  	return 1;
>  }
> +NOKPROBE_SYMBOL(uprobe_post_sstep_notifier);
>  
>  static struct notifier_block uprobe_exception_nb = {
>  	.notifier_call		= arch_uprobe_exception_notify,
> -- 
> 2.5.5
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Pratyush Anand]

Hi Masami,

On 03/08/2016:12:45:24 AM, Masami Hiramatsu wrote:
> On Tue,  2 Aug 2016 12:14:06 +0530
> Pratyush Anand <panand@redhat.com> wrote:
> 
> > uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
> > debug exception handler, so blacklist them for kprobing.
> 
> Actually, these exception notifers are kicked only if the debug exception
> is not related to kprobes (at least on x86). In that case, we don't have
> to take care about that. Or, would you hit any problem on it?

Well, I have faced issue on ARM64. So, if I have a kprobe instrumented at these
functions and then if I hit a uprobe then kernel goes into an infinite loop of
"Unexpected kernel single-step exception at EL1".

On x86 I have not tested, but I see that all functions except
arch_uprobe_exception_notify() in the call stack of
uprobe_pre/post_sstep_notifier() are blacklisted for kprobe. So, I am unable to
understand that why arch_uprobe_exception_notify() and
uprobe_pre/post_sstep_notifier() are not blacklisted.

> 
> IOW, where do we have to prohibit kprobes are, the code path from where 
> right after the breakpoint (debug) exception is occurred, to where right
> before the kprobe is handled. After that, it should be safe.

Hummmm...My understanding was that if a function a() is not good to be kprobed
then we can not kprobe any function called by a() as well. Thanks for the
clarification. So, if I go with your definition then, something is still wrong on
ARM64 which is causing issue when I kprobe uprobe_pre/post_sstep_notifier().

~Pratyush

> 
> Thank you,
> 
> 
> > 
> > Signed-off-by: Pratyush Anand <panand@redhat.com>
> > ---
> >  kernel/events/uprobes.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index b7a525ab2083..206e594cb65e 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/percpu-rwsem.h>
> >  #include <linux/task_work.h>
> >  #include <linux/shmem_fs.h>
> > +#include <linux/kprobes.h>
> >  
> >  #include <linux/uprobes.h>
> >  
> > @@ -1997,6 +1998,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
> >  	set_thread_flag(TIF_UPROBE);
> >  	return 1;
> >  }
> > +NOKPROBE_SYMBOL(uprobe_pre_sstep_notifier);
> >  
> >  /*
> >   * uprobe_post_sstep_notifier gets called in interrupt context as part of notifier
> > @@ -2014,6 +2016,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
> >  	set_thread_flag(TIF_UPROBE);
> >  	return 1;
> >  }
> > +NOKPROBE_SYMBOL(uprobe_post_sstep_notifier);
> >  
> >  static struct notifier_block uprobe_exception_nb = {
> >  	.notifier_call		= arch_uprobe_exception_notify,
> > -- 
> > 2.5.5
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Pratyush Anand]

On Wed, Aug 3, 2016 at 9:54 AM, Pratyush Anand <panand@redhat.com> wrote:
> Hi Masami,
>
> On 03/08/2016:12:45:24 AM, Masami Hiramatsu wrote:
>> On Tue,  2 Aug 2016 12:14:06 +0530
>> Pratyush Anand <panand@redhat.com> wrote:
>>
>> > uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
>> > debug exception handler, so blacklist them for kprobing.
>>
>> Actually, these exception notifers are kicked only if the debug exception
>> is not related to kprobes (at least on x86). In that case, we don't have
>> to take care about that. Or, would you hit any problem on it?
>
> Well, I have faced issue on ARM64. So, if I have a kprobe instrumented at these
> functions and then if I hit a uprobe then kernel goes into an infinite loop of
> "Unexpected kernel single-step exception at EL1".
>
> On x86 I have not tested, but I see that all functions except
> arch_uprobe_exception_notify() in the call stack of
> uprobe_pre/post_sstep_notifier() are blacklisted for kprobe. So, I am unable to
> understand that why arch_uprobe_exception_notify() and
> uprobe_pre/post_sstep_notifier() are not blacklisted.
>
>>
>> IOW, where do we have to prohibit kprobes are, the code path from where
>> right after the breakpoint (debug) exception is occurred, to where right
>> before the kprobe is handled. After that, it should be safe.
>
> Hummmm...My understanding was that if a function a() is not good to be kprobed
> then we can not kprobe any function called by a() as well. Thanks for the
> clarification. So, if I go with your definition then, something is still wrong on
> ARM64 which is causing issue when I kprobe uprobe_pre/post_sstep_notifier().

I found that one modification in ARM64 kprobe code allows me to kprobe
uprobe_pre/post_sstep_notifier(). So, taking back this patch. Will
discuss ARM64 modification on arm mailing list.

Thanks Masami for your input.

~Pratyush

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Masami Hiramatsu]

On Wed, 3 Aug 2016 16:05:34 +0530
Pratyush Anand <panand@redhat.com> wrote:

> On Wed, Aug 3, 2016 at 9:54 AM, Pratyush Anand <panand@redhat.com> wrote:
> > Hi Masami,
> >
> > On 03/08/2016:12:45:24 AM, Masami Hiramatsu wrote:
> >> On Tue,  2 Aug 2016 12:14:06 +0530
> >> Pratyush Anand <panand@redhat.com> wrote:
> >>
> >> > uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
> >> > debug exception handler, so blacklist them for kprobing.
> >>
> >> Actually, these exception notifers are kicked only if the debug exception
> >> is not related to kprobes (at least on x86). In that case, we don't have
> >> to take care about that. Or, would you hit any problem on it?
> >
> > Well, I have faced issue on ARM64. So, if I have a kprobe instrumented at these
> > functions and then if I hit a uprobe then kernel goes into an infinite loop of
> > "Unexpected kernel single-step exception at EL1".
> >
> > On x86 I have not tested, but I see that all functions except
> > arch_uprobe_exception_notify() in the call stack of
> > uprobe_pre/post_sstep_notifier() are blacklisted for kprobe. So, I am unable to
> > understand that why arch_uprobe_exception_notify() and
> > uprobe_pre/post_sstep_notifier() are not blacklisted.
> >
> >>
> >> IOW, where do we have to prohibit kprobes are, the code path from where
> >> right after the breakpoint (debug) exception is occurred, to where right
> >> before the kprobe is handled. After that, it should be safe.
> >
> > Hummmm...My understanding was that if a function a() is not good to be kprobed
> > then we can not kprobe any function called by a() as well. Thanks for the
> > clarification. So, if I go with your definition then, something is still wrong on
> > ARM64 which is causing issue when I kprobe uprobe_pre/post_sstep_notifier().
> 
> I found that one modification in ARM64 kprobe code allows me to kprobe
> uprobe_pre/post_sstep_notifier(). So, taking back this patch. Will
> discuss ARM64 modification on arm mailing list.

OK, so that will be ARM64 specific issue. We'd better to trace it.
Thank you for your effort!!


> 
> Thanks Masami for your input.
> 
> ~Pratyush


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Oleg Nesterov]

On 08/02, Pratyush Anand wrote:
>
> uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
> debug exception handler, so blacklist them for kprobing.

Let me add kprobes maintainers, I am a bit confused...

> @@ -1997,6 +1998,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
>  	set_thread_flag(TIF_UPROBE);
>  	return 1;
>  }
> +NOKPROBE_SYMBOL(uprobe_pre_sstep_notifier);
>
>  /*
>   * uprobe_post_sstep_notifier gets called in interrupt context as part of notifier
> @@ -2014,6 +2016,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
>  	set_thread_flag(TIF_UPROBE);
>  	return 1;
>  }
> +NOKPROBE_SYMBOL(uprobe_post_sstep_notifier);

but if we need to blacklist uprobe_pre/post_sstep_notifier then we
also need to blacklist their caller, arch_uprobe_exception_notify() ?

and every .notifier_call used in register_die_notifier() ?

Oleg.

Re: [PATCH] uprobe: Add uprobe_pre/post_sstep_notifier to NOKPROBE_SYMBOL

[Pratyush Anand]

Hi Oleg,

On 02/08/2016:10:30:35 PM, Oleg Nesterov wrote:
> On 08/02, Pratyush Anand wrote:
> >
> > uprobe_pre_sstep_notifier and uprobe_post_sstep_notifier are called from
> > debug exception handler, so blacklist them for kprobing.
> 
> Let me add kprobes maintainers, I am a bit confused...
> 
> > @@ -1997,6 +1998,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
> >  	set_thread_flag(TIF_UPROBE);
> >  	return 1;
> >  }
> > +NOKPROBE_SYMBOL(uprobe_pre_sstep_notifier);
> >
> >  /*
> >   * uprobe_post_sstep_notifier gets called in interrupt context as part of notifier
> > @@ -2014,6 +2016,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
> >  	set_thread_flag(TIF_UPROBE);
> >  	return 1;
> >  }
> > +NOKPROBE_SYMBOL(uprobe_post_sstep_notifier);
> 
> but if we need to blacklist uprobe_pre/post_sstep_notifier then we
> also need to blacklist their caller, arch_uprobe_exception_notify() ?

I think yes, in ARM64 I have done that. However, arm64 does not use notifier
method, so arch_uprobe_exception_notify() is just a dummy function for it.

> 
> and every .notifier_call used in register_die_notifier() ?

I tried to look into x86 notify path related to uprobe_pre/post_sstep_notifier().
I see that calling sequence is like do_int3()-> notify_die() ->
atomic_notifier_call_chain() -> __atomic_notifier_call_chain() ->
notifier_call_chain() -> arch_uprobe_exception_notify().

In this sequence, every function is blacklisted for kprobe except
arch_uprobe_exception_notify(). So, I am unable to understand, if
notifier_call_chain() is not safe for kprobe then how can it be safe for a
function it calls.

~Pratyush