Re: [PATCH -tip v9 20/26] kprobes: Support blacklist functions in module

[Ingo Molnar <mingo at kernel dot org>]

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

> To blacklist the functions in a module (e.g. user-defined
> kprobe handler and the functions invoked from it), expand
> blacklist support for modules.
> With this change, users can use NOKPROBE_SYMBOL() macro in
> their own modules.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Rob Landley <rob@landley.net>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  Documentation/kprobes.txt |    8 ++++++
>  include/linux/module.h    |    5 ++++
>  kernel/kprobes.c          |   63 ++++++++++++++++++++++++++++++++++++++-------
>  kernel/module.c           |    6 ++++
>  4 files changed, 72 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 4bbeca8..2845956 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -512,6 +512,14 @@ int enable_jprobe(struct jprobe *jp);
>  Enables *probe which has been disabled by disable_*probe(). You must specify
>  the probe which has been registered.
>  
> +4.9 NOKPROBE_SYMBOL()
> +
> +#include <linux/kprobes.h>
> +NOKPROBE_SYMBOL(FUNCTION);
> +
> +Protects given FUNCTION from other kprobes. This is useful for handler
> +functions and functions called from the handlers.
> +
>  5. Kprobes Features and Limitations
>  
>  Kprobes allows multiple probes at the same address.  Currently,
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f520a76..2fdb673 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -16,6 +16,7 @@
>  #include <linux/kobject.h>
>  #include <linux/moduleparam.h>
>  #include <linux/jump_label.h>
> +#include <linux/kprobes.h>

This include breaks the x86 build:

  CC      arch/x86/kernel/jump_label.o
  In file included from arch/x86/kernel/jump_label.c:14:0:
  /fast/mingo/tip/arch/x86/include/asm/kprobes.h:35:12: error: conflicting types for âkprobe_opcode_t' typedef u8 kprobe_opcode_t;
  [...]

But the #include kprobes.h is unnecessary to begin with, as no kprobe 
specific types are used.

>  #include <linux/export.h>
>  
>  #include <linux/percpu.h>
> @@ -357,6 +358,10 @@ struct module {
>  	unsigned int num_ftrace_callsites;
>  	unsigned long *ftrace_callsites;
>  #endif
> +#ifdef CONFIG_KPROBES
> +	unsigned int num_kprobe_blacklist;
> +	unsigned long  *kprobe_blacklist;
> +#endif

There's a small coding style problem here.

More importantly, I think more should be done to make sure that module 
symbols are marked properly: since the module is going to register the 
kprobes handler, that would be a perfect place to emit a warning, 
right?

In fact, why don't kprobe handlers get added to the exclusion list 
explicitly, when the handler gets registered? With such an approach 
handlers are automatically nokprobe and don't need any annotation - 
which is a far more robust usage model.

So I'm skipping this patch and the next one that makes use of it.

Thanks,

	Ingo