[PATCH -tip v2 0/3] kprobes: introduce nokprobe and updating blacklist

[PATCH -tip v2 0/3] kprobes: introduce nokprobe and updating blacklist

[Masami Hiramatsu]

According to Ingo's suggestion, I've introduced 'nokprobe'
new annotation as like as 'notrace' annotation, since
__kprobes annotation is too confusing to mark non-kprobe
able functions.
I took 'nokprobe' instead of 'noprobe' since we already
have irq_set_noprobe() etc.

Also, I've found that putting probes on some functions
could lock or reboot the kernel. These two patches fix
those bugs by prohibiting probing such functions by
adding those to blacklist(kprobes.text).

In both cases, those functions are related to int3 handling
execution path. One is related to irqoff-tracer and the other
is related to notifier debug option.

Thank you,

 Changes in the latest version:
  - introduce nokprobe annotation
  - change __kprobes to nokprobe in bugfixes
  - remove unneeded linux/kprobes.h includings.

---

Masami Hiramatsu (3):
      kprobes: Introduce nokprobe annotation for non-probe-able functions
      [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_*
      [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text


 arch/x86/kernel/cpu/common.c |    6 +++---
 include/linux/compiler.h     |    6 ++++--
 kernel/extable.c             |    2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH -tip v2 1/3] kprobes: Introduce nokprobe annotation for non-probe-able functions

[Masami Hiramatsu]

Instead of __kprobes annotation, introduce 'nokprobe' new annotation
to annotate that the function is not probed by kprobes.

Previously the '__kprobes' is used just for avoiding probes on
kprobes-related functions which will be used from kprobes. However
nowadays we use it for prohibiting probing the functions implicitly
invoked from kprobes int3 handler, since that causes infinit-loop
lockup or sudden reboot. In this case, the annotated functions are
not limited in the kprobes-related functions, and __kprobes looks
very confusing. (Moreover, actually, most of control-side kprobes
functions like as register_kprobes() are safely probed by kprobes)

Thus, we decide to introduce 'nokprobe' new annotation. We leave
"__kprobes" just for compatibility but it should be replaced or
removed eventually.

New commits must use 'nokprobe' for this purpose.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 include/linux/compiler.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 92669cd..173c64e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -353,8 +353,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 
 /* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
 #ifdef CONFIG_KPROBES
-# define __kprobes	__attribute__((__section__(".kprobes.text")))
+# define nokprobe	__attribute__((__section__(".kprobes.text")))
 #else
-# define __kprobes
+# define nokprobe
 #endif
+#define __kprobes nokprobe
+
 #endif /* __LINUX_COMPILER_H */

Re: [PATCH -tip v2 1/3] kprobes: Introduce nokprobe annotation for non-probe-able functions

[Steven Rostedt]

On Fri, 01 Nov 2013 11:25:32 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Instead of __kprobes annotation, introduce 'nokprobe' new annotation
> to annotate that the function is not probed by kprobes.
> 
> Previously the '__kprobes' is used just for avoiding probes on
> kprobes-related functions which will be used from kprobes. However
> nowadays we use it for prohibiting probing the functions implicitly
> invoked from kprobes int3 handler, since that causes infinit-loop
> lockup or sudden reboot. In this case, the annotated functions are
> not limited in the kprobes-related functions, and __kprobes looks
> very confusing. (Moreover, actually, most of control-side kprobes
> functions like as register_kprobes() are safely probed by kprobes)
> 
> Thus, we decide to introduce 'nokprobe' new annotation. We leave
> "__kprobes" just for compatibility but it should be replaced or
> removed eventually.
> 
> New commits must use 'nokprobe' for this purpose.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
>  include/linux/compiler.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 92669cd..173c64e 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -353,8 +353,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  
>  /* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
>  #ifdef CONFIG_KPROBES
> -# define __kprobes	__attribute__((__section__(".kprobes.text")))
> +# define nokprobe	__attribute__((__section__(".kprobes.text")))

I wonder if we should have both a __kprobes and nokprobe annotation,
such that we have:

# define __kprobes	__attribute__((__section__(".kprobes.text")))
# define nokprobe	__attribute__((__section__(".nokprobes.text")))

Then use __kprobes for the actual kprobes code, and nokprobe for all
the places that must not be traced by kprobes.

It just seems strange to me grouping kprobes code with non kprobes code.

-- Steve


>  #else
> -# define __kprobes
> +# define nokprobe
>  #endif
> +#define __kprobes nokprobe
> +
>  #endif /* __LINUX_COMPILER_H */
> 

Re: Re: [PATCH -tip v2 1/3] kprobes: Introduce nokprobe annotation for non-probe-able functions

[Masami Hiramatsu]

(2013/11/01 22:55), Steven Rostedt wrote:
> On Fri, 01 Nov 2013 11:25:32 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Instead of __kprobes annotation, introduce 'nokprobe' new annotation
>> to annotate that the function is not probed by kprobes.
>>
>> Previously the '__kprobes' is used just for avoiding probes on
>> kprobes-related functions which will be used from kprobes. However
>> nowadays we use it for prohibiting probing the functions implicitly
>> invoked from kprobes int3 handler, since that causes infinit-loop
>> lockup or sudden reboot. In this case, the annotated functions are
>> not limited in the kprobes-related functions, and __kprobes looks
>> very confusing. (Moreover, actually, most of control-side kprobes
>> functions like as register_kprobes() are safely probed by kprobes)
>>
>> Thus, we decide to introduce 'nokprobe' new annotation. We leave
>> "__kprobes" just for compatibility but it should be replaced or
>> removed eventually.
>>
>> New commits must use 'nokprobe' for this purpose.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> ---
>>  include/linux/compiler.h |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 92669cd..173c64e 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -353,8 +353,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>  
>>  /* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
>>  #ifdef CONFIG_KPROBES
>> -# define __kprobes	__attribute__((__section__(".kprobes.text")))
>> +# define nokprobe	__attribute__((__section__(".kprobes.text")))
> 
> I wonder if we should have both a __kprobes and nokprobe annotation,
> such that we have:
> 
> # define __kprobes	__attribute__((__section__(".kprobes.text")))
> # define nokprobe	__attribute__((__section__(".nokprobes.text")))
> 
> Then use __kprobes for the actual kprobes code, and nokprobe for all
> the places that must not be traced by kprobes.

No, actually, we don't need __kprobes anymore. That has started
historically by misunderstanding the problem. kprobes is using
the .kprobes.text only for blacklisting the non probe-able functions.
Thus, eventually it should be renamed .nokprobe.text, not be added.

> It just seems strange to me grouping kprobes code with non kprobes code.

Yeah, so I'd like to cleanup all the __kprobes finally (and classify which
is not needed).

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH -tip v2 2/3] [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_*

[Masami Hiramatsu]

Prohibit probing on debug_stack_reset and debug_stack_set_zero.
Since the both functions are called from TRACE_IRQS_ON/OFF_DEBUG
macros which run in int3 ist entry, probing it may cause a soft
lockup.

This happens when the kernel built with CONFIG_DYNAMIC_FTRACE=y
and CONFIG_TRACE_IRQFLAGS=y.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
---
 arch/x86/kernel/cpu/common.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 1789b06..928a4fd 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1157,7 +1157,7 @@ DEFINE_PER_CPU(struct orig_ist, orig_ist);
 static DEFINE_PER_CPU(unsigned long, debug_stack_addr);
 DEFINE_PER_CPU(int, debug_stack_usage);
 
-int is_debug_stack(unsigned long addr)
+int nokprobe is_debug_stack(unsigned long addr)
 {
 	return __get_cpu_var(debug_stack_usage) ||
 		(addr <= __get_cpu_var(debug_stack_addr) &&
@@ -1166,13 +1166,13 @@ int is_debug_stack(unsigned long addr)
 
 DEFINE_PER_CPU(u32, debug_idt_ctr);
 
-void debug_stack_set_zero(void)
+void nokprobe debug_stack_set_zero(void)
 {
 	this_cpu_inc(debug_idt_ctr);
 	load_current_idt();
 }
 
-void debug_stack_reset(void)
+void nokprobe debug_stack_reset(void)
 {
 	if (WARN_ON(!this_cpu_read(debug_idt_ctr)))
 		return;

Re: [PATCH -tip v2 2/3] [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_*

[Steven Rostedt]

On Fri, 01 Nov 2013 11:25:34 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Prohibit probing on debug_stack_reset and debug_stack_set_zero.
> Since the both functions are called from TRACE_IRQS_ON/OFF_DEBUG
> macros which run in int3 ist entry, probing it may cause a soft
> lockup.
> 
> This happens when the kernel built with CONFIG_DYNAMIC_FTRACE=y
> and CONFIG_TRACE_IRQFLAGS=y.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> ---

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

[PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

Prohibit probing on func_ptr_is_kernel_text().
Since the func_ptr_is_kernel_text() is called from
notifier_call_chain() which is called from int3 handler,
probing it may cause double int3 fault and kernel will
reboot.

This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
---
 kernel/extable.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/extable.c b/kernel/extable.c
index 832cb28..022fb25 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
  * pointer is part of the kernel text, we need to do some
  * special dereferencing first.
  */
-int func_ptr_is_kernel_text(void *ptr)
+int nokprobe func_ptr_is_kernel_text(void *ptr)
 {
 	unsigned long addr;
 	addr = (unsigned long) dereference_function_descriptor(ptr);

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Steven Rostedt]

On Fri, 01 Nov 2013 11:25:37 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Prohibit probing on func_ptr_is_kernel_text().
> Since the func_ptr_is_kernel_text() is called from
> notifier_call_chain() which is called from int3 handler,
> probing it may cause double int3 fault and kernel will
> reboot.
> 
> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/extable.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/extable.c b/kernel/extable.c
> index 832cb28..022fb25 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>   * pointer is part of the kernel text, we need to do some
>   * special dereferencing first.
>   */
> -int func_ptr_is_kernel_text(void *ptr)
> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>  {
>  	unsigned long addr;
>  	addr = (unsigned long) dereference_function_descriptor(ptr);
> 

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Steven Rostedt]

On Fri, 01 Nov 2013 11:25:37 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Prohibit probing on func_ptr_is_kernel_text().
> Since the func_ptr_is_kernel_text() is called from
> notifier_call_chain() which is called from int3 handler,
> probing it may cause double int3 fault and kernel will
> reboot.
> 
> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/extable.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/extable.c b/kernel/extable.c
> index 832cb28..022fb25 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>   * pointer is part of the kernel text, we need to do some
>   * special dereferencing first.
>   */
> -int func_ptr_is_kernel_text(void *ptr)
> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>  {
>  	unsigned long addr;
>  	addr = (unsigned long) dereference_function_descriptor(ptr);
> 

One thing I worry about the "nokprobe" annotation, is that it moves the
location of the function out of local. This function no exists in
the section with its users. Same with the debug functions in the
other patch.

Now these may be a slow path where we really don't care, but if the
nokprobe expands this can cause issues.

The "nokprobe" works differently than "notrace" as "notrace" is just an
attribute that tells gcc not to add mcount to it. The "nokprobe"
actually moves the function into a different section.

-- Steve

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

(2013/11/05 11:00), Steven Rostedt wrote:
> On Fri, 01 Nov 2013 11:25:37 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Prohibit probing on func_ptr_is_kernel_text().
>> Since the func_ptr_is_kernel_text() is called from
>> notifier_call_chain() which is called from int3 handler,
>> probing it may cause double int3 fault and kernel will
>> reboot.
>>
>> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> ---
>>  kernel/extable.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/extable.c b/kernel/extable.c
>> index 832cb28..022fb25 100644
>> --- a/kernel/extable.c
>> +++ b/kernel/extable.c
>> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>>   * pointer is part of the kernel text, we need to do some
>>   * special dereferencing first.
>>   */
>> -int func_ptr_is_kernel_text(void *ptr)
>> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>>  {
>>  	unsigned long addr;
>>  	addr = (unsigned long) dereference_function_descriptor(ptr);
>>
> 
> One thing I worry about the "nokprobe" annotation, is that it moves the
> location of the function out of local. This function no exists in
> the section with its users. Same with the debug functions in the
> other patch.
> 
> Now these may be a slow path where we really don't care, but if the
> nokprobe expands this can cause issues.
> 
> The "nokprobe" works differently than "notrace" as "notrace" is just an
> attribute that tells gcc not to add mcount to it. The "nokprobe"
> actually moves the function into a different section.

Well, in that case, I can put it in the opt-out type blacklist(kprobe_blacklist). :)
Hmm, I think if I can list nokprobe functions up at build time, we can almost
remove the .kprobes.text (Note that some of entry functions in asm still require it.)

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 01 Nov 2013 11:25:37 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
> > Prohibit probing on func_ptr_is_kernel_text().
> > Since the func_ptr_is_kernel_text() is called from
> > notifier_call_chain() which is called from int3 handler,
> > probing it may cause double int3 fault and kernel will
> > reboot.
> > 
> > This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
> > 
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> >  kernel/extable.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/extable.c b/kernel/extable.c
> > index 832cb28..022fb25 100644
> > --- a/kernel/extable.c
> > +++ b/kernel/extable.c
> > @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
> >   * pointer is part of the kernel text, we need to do some
> >   * special dereferencing first.
> >   */
> > -int func_ptr_is_kernel_text(void *ptr)
> > +int nokprobe func_ptr_is_kernel_text(void *ptr)
> >  {
> >  	unsigned long addr;
> >  	addr = (unsigned long) dereference_function_descriptor(ptr);
> > 
> 
> One thing I worry about the "nokprobe" annotation, is that it moves the 
> location of the function out of local. This function no exists in the 
> section with its users. Same with the debug functions in the other 
> patch.

Well, it's a bit like noinline, that changes the position of the function 
as well. So it's not true that 'noxyz' attributes don't affect function 
placement - they often don't, but some do.

The more important aspect is that 'noprobe' makes it really, really 
apparent what the tag is about, at first sight.

_How_ the 'non probing' is achived is an implementational detail when 
kprobes are enabled: right now it puts a function into a separate section, 
but we could just a much build a list of function names and check against 
it at probe insertion time.

Thanks,

	Ingo

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

(2013/11/05 15:09), Ingo Molnar wrote:
> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> On Fri, 01 Nov 2013 11:25:37 +0000
>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>
>>> Prohibit probing on func_ptr_is_kernel_text().
>>> Since the func_ptr_is_kernel_text() is called from
>>> notifier_call_chain() which is called from int3 handler,
>>> probing it may cause double int3 fault and kernel will
>>> reboot.
>>>
>>> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
>>>
>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
>>> Cc: Borislav Petkov <bp@suse.de>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> ---
>>>  kernel/extable.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/extable.c b/kernel/extable.c
>>> index 832cb28..022fb25 100644
>>> --- a/kernel/extable.c
>>> +++ b/kernel/extable.c
>>> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>>>   * pointer is part of the kernel text, we need to do some
>>>   * special dereferencing first.
>>>   */
>>> -int func_ptr_is_kernel_text(void *ptr)
>>> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>>>  {
>>>  	unsigned long addr;
>>>  	addr = (unsigned long) dereference_function_descriptor(ptr);
>>>
>>
>> One thing I worry about the "nokprobe" annotation, is that it moves the 
>> location of the function out of local. This function no exists in the 
>> section with its users. Same with the debug functions in the other 
>> patch.
> 
> Well, it's a bit like noinline, that changes the position of the function 
> as well. So it's not true that 'noxyz' attributes don't affect function 
> placement - they often don't, but some do.
> 
> The more important aspect is that 'noprobe' makes it really, really 
> apparent what the tag is about, at first sight.
> 
> _How_ the 'non probing' is achived is an implementational detail when 
> kprobes are enabled: right now it puts a function into a separate section, 
> but we could just a much build a list of function names and check against 
> it at probe insertion time.

Actually, kprobes already has it -- kprobes_blacklist. Currently
the list is manually maintained in kprobes.c separated from the
function definition. I hope to build the list when the kernel
build time if possible... Would you have any idea to classify
some annotated(but no side-effect) functions?

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2013/11/05 15:09), Ingo Molnar wrote:
> > 
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> >> On Fri, 01 Nov 2013 11:25:37 +0000
> >> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> >>
> >>> Prohibit probing on func_ptr_is_kernel_text().
> >>> Since the func_ptr_is_kernel_text() is called from
> >>> notifier_call_chain() which is called from int3 handler,
> >>> probing it may cause double int3 fault and kernel will
> >>> reboot.
> >>>
> >>> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
> >>>
> >>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> >>> Cc: Borislav Petkov <bp@suse.de>
> >>> Cc: Ingo Molnar <mingo@kernel.org>
> >>> ---
> >>>  kernel/extable.c |    2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/kernel/extable.c b/kernel/extable.c
> >>> index 832cb28..022fb25 100644
> >>> --- a/kernel/extable.c
> >>> +++ b/kernel/extable.c
> >>> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
> >>>   * pointer is part of the kernel text, we need to do some
> >>>   * special dereferencing first.
> >>>   */
> >>> -int func_ptr_is_kernel_text(void *ptr)
> >>> +int nokprobe func_ptr_is_kernel_text(void *ptr)
> >>>  {
> >>>  	unsigned long addr;
> >>>  	addr = (unsigned long) dereference_function_descriptor(ptr);
> >>>
> >>
> >> One thing I worry about the "nokprobe" annotation, is that it moves the 
> >> location of the function out of local. This function no exists in the 
> >> section with its users. Same with the debug functions in the other 
> >> patch.
> > 
> > Well, it's a bit like noinline, that changes the position of the function 
> > as well. So it's not true that 'noxyz' attributes don't affect function 
> > placement - they often don't, but some do.
> > 
> > The more important aspect is that 'noprobe' makes it really, really 
> > apparent what the tag is about, at first sight.
> > 
> > _How_ the 'non probing' is achived is an implementational detail when 
> > kprobes are enabled: right now it puts a function into a separate section, 
> > but we could just a much build a list of function names and check against 
> > it at probe insertion time.
> 
> Actually, kprobes already has it -- kprobes_blacklist. Currently the 
> list is manually maintained in kprobes.c separated from the function 
> definition. [...]

Yes, I meant a list that is built automatically from the 'noprobe' 
annotations.

> [...] I hope to build the list when the kernel build time if possible... 
> Would you have any idea to classify some annotated(but no side-effect) 
> functions?

The macro magic I can think of would need to change the syntax of the 
function definition - for example that is how the SYSCALL_DEFINE*() macros 
work.

It would be nice if there was a GCC extension that marked a function 
noinline and allowed the emitting of the function's address (and size) 
into a special section - but I'm not aware of any such compiler feature 
today.

Thanks,

	Ingo

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

(2013/11/05 16:05), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2013/11/05 15:09), Ingo Molnar wrote:
>>>
>>> * Steven Rostedt <rostedt@goodmis.org> wrote:
>>>
>>>> On Fri, 01 Nov 2013 11:25:37 +0000
>>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>>
>>>>> Prohibit probing on func_ptr_is_kernel_text().
>>>>> Since the func_ptr_is_kernel_text() is called from
>>>>> notifier_call_chain() which is called from int3 handler,
>>>>> probing it may cause double int3 fault and kernel will
>>>>> reboot.
>>>>>
>>>>> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
>>>>>
>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
>>>>> Cc: Borislav Petkov <bp@suse.de>
>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>> ---
>>>>>  kernel/extable.c |    2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kernel/extable.c b/kernel/extable.c
>>>>> index 832cb28..022fb25 100644
>>>>> --- a/kernel/extable.c
>>>>> +++ b/kernel/extable.c
>>>>> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>>>>>   * pointer is part of the kernel text, we need to do some
>>>>>   * special dereferencing first.
>>>>>   */
>>>>> -int func_ptr_is_kernel_text(void *ptr)
>>>>> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>>>>>  {
>>>>>  	unsigned long addr;
>>>>>  	addr = (unsigned long) dereference_function_descriptor(ptr);
>>>>>
>>>>
>>>> One thing I worry about the "nokprobe" annotation, is that it moves the 
>>>> location of the function out of local. This function no exists in the 
>>>> section with its users. Same with the debug functions in the other 
>>>> patch.
>>>
>>> Well, it's a bit like noinline, that changes the position of the function 
>>> as well. So it's not true that 'noxyz' attributes don't affect function 
>>> placement - they often don't, but some do.
>>>
>>> The more important aspect is that 'noprobe' makes it really, really 
>>> apparent what the tag is about, at first sight.
>>>
>>> _How_ the 'non probing' is achived is an implementational detail when 
>>> kprobes are enabled: right now it puts a function into a separate section, 
>>> but we could just a much build a list of function names and check against 
>>> it at probe insertion time.
>>
>> Actually, kprobes already has it -- kprobes_blacklist. Currently the 
>> list is manually maintained in kprobes.c separated from the function 
>> definition. [...]
> 
> Yes, I meant a list that is built automatically from the 'noprobe' 
> annotations.

Agreed. That makes maintenance work simple, and we can remove
".kprobes.text" section.

>> [...] I hope to build the list when the kernel build time if possible... 
>> Would you have any idea to classify some annotated(but no side-effect) 
>> functions?
> 
> The macro magic I can think of would need to change the syntax of the 
> function definition - for example that is how the SYSCALL_DEFINE*() macros 
> work.

Would you mean something like the below macro? :)

NOKPROBE_SYMBOL(int, func_ptr_is_kernel_text)(void *ptr)

which is expanded as;

static struct nokprobe_entry __used
__nokprobe_entry_func_ptr_is_kernel_text = {
  .name = "func_ptr_is_kernel_text"
};
static struct kprobe_blacklist_entry __used
__attribute__((section("_nokprobe_list")))
__p_nokprobe_entry_func_ptr_is_kernel_text = &__nokprobe_entry_func_ptr_is_kernel_text;
int func_ptr_is_kernel_text(void *ptr)

Hmm, this looks worth to try.

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

(2013/11/05 20:38), Masami Hiramatsu wrote:
> (2013/11/05 16:05), Ingo Molnar wrote:
>>
>> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>
>>> (2013/11/05 15:09), Ingo Molnar wrote:
>>>>
>>>> * Steven Rostedt <rostedt@goodmis.org> wrote:
>>>>
>>>>> On Fri, 01 Nov 2013 11:25:37 +0000
>>>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>>>
>>>>>> Prohibit probing on func_ptr_is_kernel_text().
>>>>>> Since the func_ptr_is_kernel_text() is called from
>>>>>> notifier_call_chain() which is called from int3 handler,
>>>>>> probing it may cause double int3 fault and kernel will
>>>>>> reboot.
>>>>>>
>>>>>> This happenes when the kernel built with CONFIG_DEBUG_NOTIFIERS=y.
>>>>>>
>>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
>>>>>> Cc: Borislav Petkov <bp@suse.de>
>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>> ---
>>>>>>  kernel/extable.c |    2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/kernel/extable.c b/kernel/extable.c
>>>>>> index 832cb28..022fb25 100644
>>>>>> --- a/kernel/extable.c
>>>>>> +++ b/kernel/extable.c
>>>>>> @@ -129,7 +129,7 @@ int kernel_text_address(unsigned long addr)
>>>>>>   * pointer is part of the kernel text, we need to do some
>>>>>>   * special dereferencing first.
>>>>>>   */
>>>>>> -int func_ptr_is_kernel_text(void *ptr)
>>>>>> +int nokprobe func_ptr_is_kernel_text(void *ptr)
>>>>>>  {
>>>>>>  	unsigned long addr;
>>>>>>  	addr = (unsigned long) dereference_function_descriptor(ptr);
>>>>>>
>>>>>
>>>>> One thing I worry about the "nokprobe" annotation, is that it moves the 
>>>>> location of the function out of local. This function no exists in the 
>>>>> section with its users. Same with the debug functions in the other 
>>>>> patch.
>>>>
>>>> Well, it's a bit like noinline, that changes the position of the function 
>>>> as well. So it's not true that 'noxyz' attributes don't affect function 
>>>> placement - they often don't, but some do.
>>>>
>>>> The more important aspect is that 'noprobe' makes it really, really 
>>>> apparent what the tag is about, at first sight.
>>>>
>>>> _How_ the 'non probing' is achived is an implementational detail when 
>>>> kprobes are enabled: right now it puts a function into a separate section, 
>>>> but we could just a much build a list of function names and check against 
>>>> it at probe insertion time.
>>>
>>> Actually, kprobes already has it -- kprobes_blacklist. Currently the 
>>> list is manually maintained in kprobes.c separated from the function 
>>> definition. [...]
>>
>> Yes, I meant a list that is built automatically from the 'noprobe' 
>> annotations.
> 
> Agreed. That makes maintenance work simple, and we can remove
> ".kprobes.text" section.
> 
>>> [...] I hope to build the list when the kernel build time if possible... 
>>> Would you have any idea to classify some annotated(but no side-effect) 
>>> functions?
>>
>> The macro magic I can think of would need to change the syntax of the 
>> function definition - for example that is how the SYSCALL_DEFINE*() macros 
>> work.
> 
> Would you mean something like the below macro? :)
> 
> NOKPROBE_SYMBOL(int, func_ptr_is_kernel_text)(void *ptr)
> 
> which is expanded as;
> 
> static struct nokprobe_entry __used
> __nokprobe_entry_func_ptr_is_kernel_text = {
>   .name = "func_ptr_is_kernel_text"
> };
> static struct kprobe_blacklist_entry __used
> __attribute__((section("_nokprobe_list")))
> __p_nokprobe_entry_func_ptr_is_kernel_text = &__nokprobe_entry_func_ptr_is_kernel_text;
> int func_ptr_is_kernel_text(void *ptr)
> 

By the way, this method can be done in C file, but not in asm file.
And there are many sensitive entries in the entry_*.S. I think we have
two options.

(A) keep the kprobes.text(or nokprobe.text) section for such assembler parts.
(B) Put a starter symbol and end symbol in such region and make a list
 of symbols between them in build time by using nm.

Anyway, until removing all __kprobes from kernel, we can not remove the
kprobes.text section. So, at the first step I just try to introduce
above macro and apply it to only the symbols in kprobes_blacklist.
After that, I'll try to classify the real unsafe entries and apply
the new macro. Eventually, I think I can remove all __kprobes.

Thank you,


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> >> [...] I hope to build the list when the kernel build time if 
> >> possible... Would you have any idea to classify some annotated(but no 
> >> side-effect) functions?
> > 
> > The macro magic I can think of would need to change the syntax of the 
> > function definition - for example that is how the SYSCALL_DEFINE*() 
> > macros work.
> 
> Would you mean something like the below macro? :)
> 
> NOKPROBE_SYMBOL(int, func_ptr_is_kernel_text)(void *ptr)

I think this is rather ugly and harder to maintain. The whole _point_ of 
such annotations is to make them 'easy on the eyes', to make it easy to 
skip a 'noinline', 'noprobe' or 'notrace' tag.

Using something like NOKPROBE_SYMBOL() makes the whole construct ugly and 
attention seeking.

So until compilers get smarter (or there's some compiler trick I haven't 
noticed) lets stay with the separate section - it's not the end of the 
world, the (effective) 'noinline' aspect of noprobes changes code 
generation anyway.

Thanks,

	Ingo

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Masami Hiramatsu]

(2013/11/06 15:07), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>>>> [...] I hope to build the list when the kernel build time if 
>>>> possible... Would you have any idea to classify some annotated(but no 
>>>> side-effect) functions?
>>>
>>> The macro magic I can think of would need to change the syntax of the 
>>> function definition - for example that is how the SYSCALL_DEFINE*() 
>>> macros work.
>>
>> Would you mean something like the below macro? :)
>>
>> NOKPROBE_SYMBOL(int, func_ptr_is_kernel_text)(void *ptr)
> 
> I think this is rather ugly and harder to maintain. The whole _point_ of 
> such annotations is to make them 'easy on the eyes', to make it easy to 
> skip a 'noinline', 'noprobe' or 'notrace' tag.
> 
> Using something like NOKPROBE_SYMBOL() makes the whole construct ugly and 
> attention seeking.

Hmm, by the way how about Steven's idea? A macro like EXPORT_SYMBOL?
At least for kprobes_blacklist, which is defined/maintained in kprobes.c
for some symbols(*), that is useful for updating it because we can
put it near the function definition.

* These symbols can not moves to other section because it already
in a different section.

Of course, still this is not a big problem since there are a few symbols
in the kprobe_blacklist.

> So until compilers get smarter (or there's some compiler trick I haven't 
> noticed) lets stay with the separate section - it's not the end of the 
> world, the (effective) 'noinline' aspect of noprobes changes code 
> generation anyway.

I see. :)

So, would you pull this series ? Or I need any update?

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2013/11/06 15:07), Ingo Molnar wrote:
> > 
> > * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> > 
> >>>> [...] I hope to build the list when the kernel build time if 
> >>>> possible... Would you have any idea to classify some annotated(but no 
> >>>> side-effect) functions?
> >>>
> >>> The macro magic I can think of would need to change the syntax of the 
> >>> function definition - for example that is how the SYSCALL_DEFINE*() 
> >>> macros work.
> >>
> >> Would you mean something like the below macro? :)
> >>
> >> NOKPROBE_SYMBOL(int, func_ptr_is_kernel_text)(void *ptr)
> > 
> > I think this is rather ugly and harder to maintain. The whole _point_ of 
> > such annotations is to make them 'easy on the eyes', to make it easy to 
> > skip a 'noinline', 'noprobe' or 'notrace' tag.
> > 
> > Using something like NOKPROBE_SYMBOL() makes the whole construct ugly and 
> > attention seeking.
> 
> Hmm, by the way how about Steven's idea? A macro like EXPORT_SYMBOL? At 
> least for kprobes_blacklist, which is defined/maintained in kprobes.c 
> for some symbols(*), that is useful for updating it because we can put 
> it near the function definition.

Dunno, it feels a bit fragile to me.

Thanks,

	Ingo

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Steven Rostedt]

On Wed, 6 Nov 2013 07:07:37 +0100
Ingo Molnar <mingo@kernel.org> wrote:


> So until compilers get smarter (or there's some compiler trick I haven't 
> noticed) lets stay with the separate section - it's not the end of the 
> world, the (effective) 'noinline' aspect of noprobes changes code 
> generation anyway.
> 

It's fine now. I just wanted everyone to be ware if this annotation
starts to get used a bit more, that it can cause unwanted side effects.

-- Steve

Re: [PATCH -tip v2 3/3] [BUGFIX] kprobes: Prohibit probing on func_ptr_is_kernel_text

[Steven Rostedt]

On Tue, 5 Nov 2013 08:05:37 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> The macro magic I can think of would need to change the syntax of the 
> function definition - for example that is how the SYSCALL_DEFINE*() macros 
> work.

Or something like the EXPORT_SYMBOL(), but that wouldn't include the
size of the function. But using the name we could use kallsyms to see
if a probe is placed in a function that is blacklisted. Not very pretty
to do though.

> 
> It would be nice if there was a GCC extension that marked a function 
> noinline and allowed the emitting of the function's address (and size) 
> into a special section - but I'm not aware of any such compiler feature 
> today.

Yeah, I was wishing the same thing. Maybe I'll try to talk with the gcc
folks about adding such a feature. Something like

void  __attribute__((save_loc_and_size(".section"))) function(void)
{
}

-- Steve