Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Wed, 11 Jul 2018 20:40:45 -0400
Francis Deslauriers  wrote:

> Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt
>  a écrit :
> >
> > On Wed, 11 Jul 2018 15:34:30 -0400
> > Francis Deslauriers  wrote:
> >
> > > Hi Steven,
> > > I tested it and it prevents the kernel crash I am witnessing.
> > > As for the side-effect that Masami mentioned regarding not being able to 
> > > probe
> > > function inside the trace_kprobe.c file, I suggest we move the target
> > > function in
> > > its own separate compile unit so it can be compiled with the ftrace 
> > > cflags.
> > > See patch below.
> > >
> >
> > The patch below looks fine and so does Masami's. But there's too many
> > patches within emails (not separated out). I have no idea what to
> > apply. I'm not going to apply anything that is not sent as a proper
> > patch (ie. any patch within a separate thread, like the patch below).
> >
> I will put together a proper patch set with both commits.
> 
> Masami, you mentioned: "So anyway we still need to mark those functions
> NOKPROBE_SYMBOL." in a reply. What functions were you talking about?
> ftrace_ops_assist_func? Aren't those functions covered by your
> within_notrace_func check?

Ah, I thought that we'd better to consider a "pure" kprobe without ftrace
on notrace functions. From ftrace, we can prohibit probing on notrace funcs,
but if user makes an out-of-tree kprobe module and put it on notrace funcs,
that can cause a problem (if they enable some kprobe events at same time).

Thank you,



-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Wed, 11 Jul 2018 20:40:45 -0400
Francis Deslauriers  wrote:

> Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt
>  a écrit :
> >
> > On Wed, 11 Jul 2018 15:34:30 -0400
> > Francis Deslauriers  wrote:
> >
> > > Hi Steven,
> > > I tested it and it prevents the kernel crash I am witnessing.
> > > As for the side-effect that Masami mentioned regarding not being able to 
> > > probe
> > > function inside the trace_kprobe.c file, I suggest we move the target
> > > function in
> > > its own separate compile unit so it can be compiled with the ftrace 
> > > cflags.
> > > See patch below.
> > >
> >
> > The patch below looks fine and so does Masami's. But there's too many
> > patches within emails (not separated out). I have no idea what to
> > apply. I'm not going to apply anything that is not sent as a proper
> > patch (ie. any patch within a separate thread, like the patch below).
> >
> I will put together a proper patch set with both commits.
> 
> Masami, you mentioned: "So anyway we still need to mark those functions
> NOKPROBE_SYMBOL." in a reply. What functions were you talking about?
> ftrace_ops_assist_func? Aren't those functions covered by your
> within_notrace_func check?

Ah, I thought that we'd better to consider a "pure" kprobe without ftrace
on notrace functions. From ftrace, we can prohibit probing on notrace funcs,
but if user makes an out-of-tree kprobe module and put it on notrace funcs,
that can cause a problem (if they enable some kprobe events at same time).

Thank you,



-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Wed, 11 Jul 2018 15:34:30 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> I tested it and it prevents the kernel crash I am witnessing.
> As for the side-effect that Masami mentioned regarding not being able to probe
> function inside the trace_kprobe.c file, I suggest we move the target
> function in
> its own separate compile unit so it can be compiled with the ftrace cflags.
> See patch below.

Ah, good catch.

> 
> Thanks
> Francis
> 
> From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
> From: Francis Deslauriers 
> Date: Wed, 11 Jul 2018 12:34:22 -0400
> Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
>  compile unit
> 
> Move selftest function to its own compile unit so it can be compiled
> with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> during the ftrace startup tests.

Looks good to me.

Acked-by: Masami Hiramatsu 

Thanks!

> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/Makefile|  5 +
>  kernel/trace/trace_kprobe.c  | 12 +---
>  kernel/trace/trace_kprobe_selftest.c | 10 ++
>  kernel/trace/trace_kprobe_selftest.h |  7 +++
>  4 files changed, 23 insertions(+), 11 deletions(-)
>  create mode 100644 kernel/trace/trace_kprobe_selftest.c
>  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index e2538c7..e38771e 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
>  endif
>  endif
> 
> +ifdef CONFIG_FTRACE_STARTUP_TEST
> +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> +endif
> +
>  # If unlikely tracing is enabled, do not trace these files
>  ifdef CONFIG_TRACING_BRANCHES
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 952dc2a..3fe966f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -24,6 +24,7 @@
>  #include 
> 
>  #include "trace_probe.h"
> +#include "trace_kprobe_selftest.h"
> 
>  #define KPROBE_EVENT_SYSTEM "kprobes"
>  #define KRETPROBE_MAXACTIVE_MAX 4096
> @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> 
> 
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
> -/*
> - * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table. 'noinline' makes sure that there
> - * isn't an inlined version used by the test method below
> - */
> -static __used __init noinline int
> -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
> -{
> - return a1 + a2 + a3 + a4 + a5 + a6;
> -}
> -
>  static __init struct trace_event_file *
>  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>  {
> diff --git a/kernel/trace/trace_kprobe_selftest.c
> b/kernel/trace/trace_kprobe_selftest.c
> new file mode 100644
> index 000..a3d2090
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6)
> +{
> + return a1 + a2 + a3 + a4 + a5 + a6;
> +}
> diff --git a/kernel/trace/trace_kprobe_selftest.h
> b/kernel/trace/trace_kprobe_selftest.h
> new file mode 100644
> index 000..9243d4e
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.h
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6);
> -- 
> 2.7.4
> 
> 
> Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt  a 
> écrit :
> >
> > Mathieu and Francis,
> >
> > Looking back, this thread never got further. Would Masami's patch work
> > for you?
> >
> > -- Steve
> >
> >
> > On Sat, 17 Mar 2018 10:22:11 +0900
> > Masami Hiramatsu  wrote:
> >
> > > On Sat, 17 Mar 2018 09:13:34 +0900
> > > Masami Hiramatsu  wrote:
> > >
> > > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > > > Mathieu Desnoyers  wrote:
> > > >
> > > > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > > > >
> > > > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > > > Steven Rostedt  wrote:
> > > > > >
> > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, 
> > > > > >> I'm
> > > > > >> saying that I don't have time to fix it now, but would be happy to
> > > > > >> accept patches if someone else does so.
> > > > > >
> > > > > > And looking at what I replied before for the original patch. It 
> > > > > > 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Wed, 11 Jul 2018 15:34:30 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> I tested it and it prevents the kernel crash I am witnessing.
> As for the side-effect that Masami mentioned regarding not being able to probe
> function inside the trace_kprobe.c file, I suggest we move the target
> function in
> its own separate compile unit so it can be compiled with the ftrace cflags.
> See patch below.

Ah, good catch.

> 
> Thanks
> Francis
> 
> From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
> From: Francis Deslauriers 
> Date: Wed, 11 Jul 2018 12:34:22 -0400
> Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
>  compile unit
> 
> Move selftest function to its own compile unit so it can be compiled
> with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> during the ftrace startup tests.

Looks good to me.

Acked-by: Masami Hiramatsu 

Thanks!

> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/Makefile|  5 +
>  kernel/trace/trace_kprobe.c  | 12 +---
>  kernel/trace/trace_kprobe_selftest.c | 10 ++
>  kernel/trace/trace_kprobe_selftest.h |  7 +++
>  4 files changed, 23 insertions(+), 11 deletions(-)
>  create mode 100644 kernel/trace/trace_kprobe_selftest.c
>  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index e2538c7..e38771e 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
>  endif
>  endif
> 
> +ifdef CONFIG_FTRACE_STARTUP_TEST
> +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> +endif
> +
>  # If unlikely tracing is enabled, do not trace these files
>  ifdef CONFIG_TRACING_BRANCHES
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 952dc2a..3fe966f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -24,6 +24,7 @@
>  #include 
> 
>  #include "trace_probe.h"
> +#include "trace_kprobe_selftest.h"
> 
>  #define KPROBE_EVENT_SYSTEM "kprobes"
>  #define KRETPROBE_MAXACTIVE_MAX 4096
> @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> 
> 
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
> -/*
> - * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table. 'noinline' makes sure that there
> - * isn't an inlined version used by the test method below
> - */
> -static __used __init noinline int
> -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
> -{
> - return a1 + a2 + a3 + a4 + a5 + a6;
> -}
> -
>  static __init struct trace_event_file *
>  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>  {
> diff --git a/kernel/trace/trace_kprobe_selftest.c
> b/kernel/trace/trace_kprobe_selftest.c
> new file mode 100644
> index 000..a3d2090
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6)
> +{
> + return a1 + a2 + a3 + a4 + a5 + a6;
> +}
> diff --git a/kernel/trace/trace_kprobe_selftest.h
> b/kernel/trace/trace_kprobe_selftest.h
> new file mode 100644
> index 000..9243d4e
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.h
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6);
> -- 
> 2.7.4
> 
> 
> Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt  a 
> écrit :
> >
> > Mathieu and Francis,
> >
> > Looking back, this thread never got further. Would Masami's patch work
> > for you?
> >
> > -- Steve
> >
> >
> > On Sat, 17 Mar 2018 10:22:11 +0900
> > Masami Hiramatsu  wrote:
> >
> > > On Sat, 17 Mar 2018 09:13:34 +0900
> > > Masami Hiramatsu  wrote:
> > >
> > > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > > > Mathieu Desnoyers  wrote:
> > > >
> > > > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > > > >
> > > > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > > > Steven Rostedt  wrote:
> > > > > >
> > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, 
> > > > > >> I'm
> > > > > >> saying that I don't have time to fix it now, but would be happy to
> > > > > >> accept patches if someone else does so.
> > > > > >
> > > > > > And looking at what I replied before for the original patch. It 
> > > > > > 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt
 a écrit :
>
> On Wed, 11 Jul 2018 15:34:30 -0400
> Francis Deslauriers  wrote:
>
> > Hi Steven,
> > I tested it and it prevents the kernel crash I am witnessing.
> > As for the side-effect that Masami mentioned regarding not being able to 
> > probe
> > function inside the trace_kprobe.c file, I suggest we move the target
> > function in
> > its own separate compile unit so it can be compiled with the ftrace cflags.
> > See patch below.
> >
>
> The patch below looks fine and so does Masami's. But there's too many
> patches within emails (not separated out). I have no idea what to
> apply. I'm not going to apply anything that is not sent as a proper
> patch (ie. any patch within a separate thread, like the patch below).
>
I will put together a proper patch set with both commits.

Masami, you mentioned: "So anyway we still need to mark those functions
NOKPROBE_SYMBOL." in a reply. What functions were you talking about?
ftrace_ops_assist_func? Aren't those functions covered by your
within_notrace_func check?

Thank you,
Francis

> -- Steve
>
>
> > Thanks
> > Francis
> >
> > >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
> > From: Francis Deslauriers 
> > Date: Wed, 11 Jul 2018 12:34:22 -0400
> > Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
> >  compile unit
> >
> > Move selftest function to its own compile unit so it can be compiled
> > with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> > during the ftrace startup tests.
> >
> > Signed-off-by: Francis Deslauriers 
> > ---
> >  kernel/trace/Makefile|  5 +
> >  kernel/trace/trace_kprobe.c  | 12 +---
> >  kernel/trace/trace_kprobe_selftest.c | 10 ++
> >  kernel/trace/trace_kprobe_selftest.h |  7 +++
> >  4 files changed, 23 insertions(+), 11 deletions(-)
> >  create mode 100644 kernel/trace/trace_kprobe_selftest.c
> >  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> >
> > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> > index e2538c7..e38771e 100644
> > --- a/kernel/trace/Makefile
> > +++ b/kernel/trace/Makefile
> > @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
> >  endif
> >  endif
> >
> > +ifdef CONFIG_FTRACE_STARTUP_TEST
> > +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> > +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> > +endif
> > +
> >  # If unlikely tracing is enabled, do not trace these files
> >  ifdef CONFIG_TRACING_BRANCHES
> >  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 952dc2a..3fe966f 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >
> >  #include "trace_probe.h"
> > +#include "trace_kprobe_selftest.h"
> >
> >  #define KPROBE_EVENT_SYSTEM "kprobes"
> >  #define KRETPROBE_MAXACTIVE_MAX 4096
> > @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> >
> >
> >  #ifdef CONFIG_FTRACE_STARTUP_TEST
> > -/*
> > - * The "__used" keeps gcc from removing the function symbol
> > - * from the kallsyms table. 'noinline' makes sure that there
> > - * isn't an inlined version used by the test method below
> > - */
> > -static __used __init noinline int
> > -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int 
> > a6)
> > -{
> > - return a1 + a2 + a3 + a4 + a5 + a6;
> > -}
> > -
> >  static __init struct trace_event_file *
> >  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
> >  {
> > diff --git a/kernel/trace/trace_kprobe_selftest.c
> > b/kernel/trace/trace_kprobe_selftest.c
> > new file mode 100644
> > index 000..a3d2090
> > --- /dev/null
> > +++ b/kernel/trace/trace_kprobe_selftest.c
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Function used during the kprobe self test. This function is in a 
> > seperate
> > + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> > + * can be probed by the selftests.
> > + */
> > +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> > a5, int a6)
> > +{
> > + return a1 + a2 + a3 + a4 + a5 + a6;
> > +}
> > diff --git a/kernel/trace/trace_kprobe_selftest.h
> > b/kernel/trace/trace_kprobe_selftest.h
> > new file mode 100644
> > index 000..9243d4e
> > --- /dev/null
> > +++ b/kernel/trace/trace_kprobe_selftest.h
> > @@ -0,0 +1,7 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Function used during the kprobe self test. This function is in a 
> > seperate
> > + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> > + * can be probed by the selftests.
> > + */
> > +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> > a5, int a6);
>


--
Francis Deslauriers
Software developer
EfficiOS inc.

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt
 a écrit :
>
> On Wed, 11 Jul 2018 15:34:30 -0400
> Francis Deslauriers  wrote:
>
> > Hi Steven,
> > I tested it and it prevents the kernel crash I am witnessing.
> > As for the side-effect that Masami mentioned regarding not being able to 
> > probe
> > function inside the trace_kprobe.c file, I suggest we move the target
> > function in
> > its own separate compile unit so it can be compiled with the ftrace cflags.
> > See patch below.
> >
>
> The patch below looks fine and so does Masami's. But there's too many
> patches within emails (not separated out). I have no idea what to
> apply. I'm not going to apply anything that is not sent as a proper
> patch (ie. any patch within a separate thread, like the patch below).
>
I will put together a proper patch set with both commits.

Masami, you mentioned: "So anyway we still need to mark those functions
NOKPROBE_SYMBOL." in a reply. What functions were you talking about?
ftrace_ops_assist_func? Aren't those functions covered by your
within_notrace_func check?

Thank you,
Francis

> -- Steve
>
>
> > Thanks
> > Francis
> >
> > >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
> > From: Francis Deslauriers 
> > Date: Wed, 11 Jul 2018 12:34:22 -0400
> > Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
> >  compile unit
> >
> > Move selftest function to its own compile unit so it can be compiled
> > with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> > during the ftrace startup tests.
> >
> > Signed-off-by: Francis Deslauriers 
> > ---
> >  kernel/trace/Makefile|  5 +
> >  kernel/trace/trace_kprobe.c  | 12 +---
> >  kernel/trace/trace_kprobe_selftest.c | 10 ++
> >  kernel/trace/trace_kprobe_selftest.h |  7 +++
> >  4 files changed, 23 insertions(+), 11 deletions(-)
> >  create mode 100644 kernel/trace/trace_kprobe_selftest.c
> >  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> >
> > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> > index e2538c7..e38771e 100644
> > --- a/kernel/trace/Makefile
> > +++ b/kernel/trace/Makefile
> > @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
> >  endif
> >  endif
> >
> > +ifdef CONFIG_FTRACE_STARTUP_TEST
> > +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> > +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> > +endif
> > +
> >  # If unlikely tracing is enabled, do not trace these files
> >  ifdef CONFIG_TRACING_BRANCHES
> >  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 952dc2a..3fe966f 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >
> >  #include "trace_probe.h"
> > +#include "trace_kprobe_selftest.h"
> >
> >  #define KPROBE_EVENT_SYSTEM "kprobes"
> >  #define KRETPROBE_MAXACTIVE_MAX 4096
> > @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> >
> >
> >  #ifdef CONFIG_FTRACE_STARTUP_TEST
> > -/*
> > - * The "__used" keeps gcc from removing the function symbol
> > - * from the kallsyms table. 'noinline' makes sure that there
> > - * isn't an inlined version used by the test method below
> > - */
> > -static __used __init noinline int
> > -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int 
> > a6)
> > -{
> > - return a1 + a2 + a3 + a4 + a5 + a6;
> > -}
> > -
> >  static __init struct trace_event_file *
> >  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
> >  {
> > diff --git a/kernel/trace/trace_kprobe_selftest.c
> > b/kernel/trace/trace_kprobe_selftest.c
> > new file mode 100644
> > index 000..a3d2090
> > --- /dev/null
> > +++ b/kernel/trace/trace_kprobe_selftest.c
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Function used during the kprobe self test. This function is in a 
> > seperate
> > + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> > + * can be probed by the selftests.
> > + */
> > +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> > a5, int a6)
> > +{
> > + return a1 + a2 + a3 + a4 + a5 + a6;
> > +}
> > diff --git a/kernel/trace/trace_kprobe_selftest.h
> > b/kernel/trace/trace_kprobe_selftest.h
> > new file mode 100644
> > index 000..9243d4e
> > --- /dev/null
> > +++ b/kernel/trace/trace_kprobe_selftest.h
> > @@ -0,0 +1,7 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Function used during the kprobe self test. This function is in a 
> > seperate
> > + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> > + * can be probed by the selftests.
> > + */
> > +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> > a5, int a6);
>


--
Francis Deslauriers
Software developer
EfficiOS inc.

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Wed, 11 Jul 2018 15:34:30 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> I tested it and it prevents the kernel crash I am witnessing.
> As for the side-effect that Masami mentioned regarding not being able to probe
> function inside the trace_kprobe.c file, I suggest we move the target
> function in
> its own separate compile unit so it can be compiled with the ftrace cflags.
> See patch below.
> 

The patch below looks fine and so does Masami's. But there's too many
patches within emails (not separated out). I have no idea what to
apply. I'm not going to apply anything that is not sent as a proper
patch (ie. any patch within a separate thread, like the patch below).

-- Steve


> Thanks
> Francis
> 
> >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001  
> From: Francis Deslauriers 
> Date: Wed, 11 Jul 2018 12:34:22 -0400
> Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
>  compile unit
> 
> Move selftest function to its own compile unit so it can be compiled
> with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> during the ftrace startup tests.
> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/Makefile|  5 +
>  kernel/trace/trace_kprobe.c  | 12 +---
>  kernel/trace/trace_kprobe_selftest.c | 10 ++
>  kernel/trace/trace_kprobe_selftest.h |  7 +++
>  4 files changed, 23 insertions(+), 11 deletions(-)
>  create mode 100644 kernel/trace/trace_kprobe_selftest.c
>  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index e2538c7..e38771e 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
>  endif
>  endif
> 
> +ifdef CONFIG_FTRACE_STARTUP_TEST
> +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> +endif
> +
>  # If unlikely tracing is enabled, do not trace these files
>  ifdef CONFIG_TRACING_BRANCHES
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 952dc2a..3fe966f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -24,6 +24,7 @@
>  #include 
> 
>  #include "trace_probe.h"
> +#include "trace_kprobe_selftest.h"
> 
>  #define KPROBE_EVENT_SYSTEM "kprobes"
>  #define KRETPROBE_MAXACTIVE_MAX 4096
> @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> 
> 
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
> -/*
> - * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table. 'noinline' makes sure that there
> - * isn't an inlined version used by the test method below
> - */
> -static __used __init noinline int
> -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
> -{
> - return a1 + a2 + a3 + a4 + a5 + a6;
> -}
> -
>  static __init struct trace_event_file *
>  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>  {
> diff --git a/kernel/trace/trace_kprobe_selftest.c
> b/kernel/trace/trace_kprobe_selftest.c
> new file mode 100644
> index 000..a3d2090
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6)
> +{
> + return a1 + a2 + a3 + a4 + a5 + a6;
> +}
> diff --git a/kernel/trace/trace_kprobe_selftest.h
> b/kernel/trace/trace_kprobe_selftest.h
> new file mode 100644
> index 000..9243d4e
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.h
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6);

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Wed, 11 Jul 2018 15:34:30 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> I tested it and it prevents the kernel crash I am witnessing.
> As for the side-effect that Masami mentioned regarding not being able to probe
> function inside the trace_kprobe.c file, I suggest we move the target
> function in
> its own separate compile unit so it can be compiled with the ftrace cflags.
> See patch below.
> 

The patch below looks fine and so does Masami's. But there's too many
patches within emails (not separated out). I have no idea what to
apply. I'm not going to apply anything that is not sent as a proper
patch (ie. any patch within a separate thread, like the patch below).

-- Steve


> Thanks
> Francis
> 
> >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001  
> From: Francis Deslauriers 
> Date: Wed, 11 Jul 2018 12:34:22 -0400
> Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
>  compile unit
> 
> Move selftest function to its own compile unit so it can be compiled
> with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
> during the ftrace startup tests.
> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/Makefile|  5 +
>  kernel/trace/trace_kprobe.c  | 12 +---
>  kernel/trace/trace_kprobe_selftest.c | 10 ++
>  kernel/trace/trace_kprobe_selftest.h |  7 +++
>  4 files changed, 23 insertions(+), 11 deletions(-)
>  create mode 100644 kernel/trace/trace_kprobe_selftest.c
>  create mode 100644 kernel/trace/trace_kprobe_selftest.h
> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index e2538c7..e38771e 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
>  endif
>  endif
> 
> +ifdef CONFIG_FTRACE_STARTUP_TEST
> +CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
> +obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
> +endif
> +
>  # If unlikely tracing is enabled, do not trace these files
>  ifdef CONFIG_TRACING_BRANCHES
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 952dc2a..3fe966f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -24,6 +24,7 @@
>  #include 
> 
>  #include "trace_probe.h"
> +#include "trace_kprobe_selftest.h"
> 
>  #define KPROBE_EVENT_SYSTEM "kprobes"
>  #define KRETPROBE_MAXACTIVE_MAX 4096
> @@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);
> 
> 
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
> -/*
> - * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table. 'noinline' makes sure that there
> - * isn't an inlined version used by the test method below
> - */
> -static __used __init noinline int
> -kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
> -{
> - return a1 + a2 + a3 + a4 + a5 + a6;
> -}
> -
>  static __init struct trace_event_file *
>  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>  {
> diff --git a/kernel/trace/trace_kprobe_selftest.c
> b/kernel/trace/trace_kprobe_selftest.c
> new file mode 100644
> index 000..a3d2090
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6)
> +{
> + return a1 + a2 + a3 + a4 + a5 + a6;
> +}
> diff --git a/kernel/trace/trace_kprobe_selftest.h
> b/kernel/trace/trace_kprobe_selftest.h
> new file mode 100644
> index 000..9243d4e
> --- /dev/null
> +++ b/kernel/trace/trace_kprobe_selftest.h
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Function used during the kprobe self test. This function is in a seperate
> + * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
> + * can be probed by the selftests.
> + */
> +int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
> a5, int a6);

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Hi Steven,
I tested it and it prevents the kernel crash I am witnessing.
As for the side-effect that Masami mentioned regarding not being able to probe
function inside the trace_kprobe.c file, I suggest we move the target
function in
its own separate compile unit so it can be compiled with the ftrace cflags.
See patch below.

Thanks
Francis

>From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
From: Francis Deslauriers 
Date: Wed, 11 Jul 2018 12:34:22 -0400
Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
 compile unit

Move selftest function to its own compile unit so it can be compiled
with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
during the ftrace startup tests.

Signed-off-by: Francis Deslauriers 
---
 kernel/trace/Makefile|  5 +
 kernel/trace/trace_kprobe.c  | 12 +---
 kernel/trace/trace_kprobe_selftest.c | 10 ++
 kernel/trace/trace_kprobe_selftest.h |  7 +++
 4 files changed, 23 insertions(+), 11 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h

diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index e2538c7..e38771e 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
 endif
 endif

+ifdef CONFIG_FTRACE_STARTUP_TEST
+CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
+obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
+endif
+
 # If unlikely tracing is enabled, do not trace these files
 ifdef CONFIG_TRACING_BRANCHES
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 952dc2a..3fe966f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -24,6 +24,7 @@
 #include 

 #include "trace_probe.h"
+#include "trace_kprobe_selftest.h"

 #define KPROBE_EVENT_SYSTEM "kprobes"
 #define KRETPROBE_MAXACTIVE_MAX 4096
@@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);


 #ifdef CONFIG_FTRACE_STARTUP_TEST
-/*
- * The "__used" keeps gcc from removing the function symbol
- * from the kallsyms table. 'noinline' makes sure that there
- * isn't an inlined version used by the test method below
- */
-static __used __init noinline int
-kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
-{
- return a1 + a2 + a3 + a4 + a5 + a6;
-}
-
 static __init struct trace_event_file *
 find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
 {
diff --git a/kernel/trace/trace_kprobe_selftest.c
b/kernel/trace/trace_kprobe_selftest.c
new file mode 100644
index 000..a3d2090
--- /dev/null
+++ b/kernel/trace/trace_kprobe_selftest.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Function used during the kprobe self test. This function is in a seperate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
a5, int a6)
+{
+ return a1 + a2 + a3 + a4 + a5 + a6;
+}
diff --git a/kernel/trace/trace_kprobe_selftest.h
b/kernel/trace/trace_kprobe_selftest.h
new file mode 100644
index 000..9243d4e
--- /dev/null
+++ b/kernel/trace/trace_kprobe_selftest.h
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Function used during the kprobe self test. This function is in a seperate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
a5, int a6);
-- 
2.7.4


Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt  a écrit :
>
> Mathieu and Francis,
>
> Looking back, this thread never got further. Would Masami's patch work
> for you?
>
> -- Steve
>
>
> On Sat, 17 Mar 2018 10:22:11 +0900
> Masami Hiramatsu  wrote:
>
> > On Sat, 17 Mar 2018 09:13:34 +0900
> > Masami Hiramatsu  wrote:
> >
> > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > > Mathieu Desnoyers  wrote:
> > >
> > > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > > >
> > > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > > Steven Rostedt  wrote:
> > > > >
> > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > > > >> saying that I don't have time to fix it now, but would be happy to
> > > > >> accept patches if someone else does so.
> > > > >
> > > > > And looking at what I replied before for the original patch. It would
> > > > > probably be a good idea to blacklist directories. Like we do with
> > > > > function tracing. We probably should black list both kernel/tracing 
> > > > > and
> > > > > kernel/events from being probed.
> > > > >
> > > > > Did this come up at plumbers? You were there too, I don't remember
> > > > > discussing it there.
> > > >
> > > > I don't remember this coming up last Plumbers nor KS neither, given
> > > > that we were focused on 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Hi Steven,
I tested it and it prevents the kernel crash I am witnessing.
As for the side-effect that Masami mentioned regarding not being able to probe
function inside the trace_kprobe.c file, I suggest we move the target
function in
its own separate compile unit so it can be compiled with the ftrace cflags.
See patch below.

Thanks
Francis

>From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001
From: Francis Deslauriers 
Date: Wed, 11 Jul 2018 12:34:22 -0400
Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate
 compile unit

Move selftest function to its own compile unit so it can be compiled
with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed
during the ftrace startup tests.

Signed-off-by: Francis Deslauriers 
---
 kernel/trace/Makefile|  5 +
 kernel/trace/trace_kprobe.c  | 12 +---
 kernel/trace/trace_kprobe_selftest.c | 10 ++
 kernel/trace/trace_kprobe_selftest.h |  7 +++
 4 files changed, 23 insertions(+), 11 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h

diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index e2538c7..e38771e 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
 endif
 endif

+ifdef CONFIG_FTRACE_STARTUP_TEST
+CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
+obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
+endif
+
 # If unlikely tracing is enabled, do not trace these files
 ifdef CONFIG_TRACING_BRANCHES
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 952dc2a..3fe966f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -24,6 +24,7 @@
 #include 

 #include "trace_probe.h"
+#include "trace_kprobe_selftest.h"

 #define KPROBE_EVENT_SYSTEM "kprobes"
 #define KRETPROBE_MAXACTIVE_MAX 4096
@@ -1560,17 +1561,6 @@ fs_initcall(init_kprobe_trace);


 #ifdef CONFIG_FTRACE_STARTUP_TEST
-/*
- * The "__used" keeps gcc from removing the function symbol
- * from the kallsyms table. 'noinline' makes sure that there
- * isn't an inlined version used by the test method below
- */
-static __used __init noinline int
-kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
-{
- return a1 + a2 + a3 + a4 + a5 + a6;
-}
-
 static __init struct trace_event_file *
 find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
 {
diff --git a/kernel/trace/trace_kprobe_selftest.c
b/kernel/trace/trace_kprobe_selftest.c
new file mode 100644
index 000..a3d2090
--- /dev/null
+++ b/kernel/trace/trace_kprobe_selftest.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Function used during the kprobe self test. This function is in a seperate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
a5, int a6)
+{
+ return a1 + a2 + a3 + a4 + a5 + a6;
+}
diff --git a/kernel/trace/trace_kprobe_selftest.h
b/kernel/trace/trace_kprobe_selftest.h
new file mode 100644
index 000..9243d4e
--- /dev/null
+++ b/kernel/trace/trace_kprobe_selftest.h
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Function used during the kprobe self test. This function is in a seperate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int
a5, int a6);
-- 
2.7.4


Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt  a écrit :
>
> Mathieu and Francis,
>
> Looking back, this thread never got further. Would Masami's patch work
> for you?
>
> -- Steve
>
>
> On Sat, 17 Mar 2018 10:22:11 +0900
> Masami Hiramatsu  wrote:
>
> > On Sat, 17 Mar 2018 09:13:34 +0900
> > Masami Hiramatsu  wrote:
> >
> > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > > Mathieu Desnoyers  wrote:
> > >
> > > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > > >
> > > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > > Steven Rostedt  wrote:
> > > > >
> > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > > > >> saying that I don't have time to fix it now, but would be happy to
> > > > >> accept patches if someone else does so.
> > > > >
> > > > > And looking at what I replied before for the original patch. It would
> > > > > probably be a good idea to blacklist directories. Like we do with
> > > > > function tracing. We probably should black list both kernel/tracing 
> > > > > and
> > > > > kernel/events from being probed.
> > > > >
> > > > > Did this come up at plumbers? You were there too, I don't remember
> > > > > discussing it there.
> > > >
> > > > I don't remember this coming up last Plumbers nor KS neither, given
> > > > that we were focused on 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

Mathieu and Francis,

Looking back, this thread never got further. Would Masami's patch work
for you?

-- Steve


On Sat, 17 Mar 2018 10:22:11 +0900
Masami Hiramatsu  wrote:

> On Sat, 17 Mar 2018 09:13:34 +0900
> Masami Hiramatsu  wrote:
> 
> > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > Mathieu Desnoyers  wrote:
> >   
> > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > >   
> > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > Steven Rostedt  wrote:
> > > >   
> > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > > >> saying that I don't have time to fix it now, but would be happy to
> > > >> accept patches if someone else does so.  
> > > > 
> > > > And looking at what I replied before for the original patch. It would
> > > > probably be a good idea to blacklist directories. Like we do with
> > > > function tracing. We probably should black list both kernel/tracing and
> > > > kernel/events from being probed.
> > > > 
> > > > Did this come up at plumbers? You were there too, I don't remember
> > > > discussing it there.  
> > > 
> > > I don't remember this coming up last Plumbers nor KS neither, given
> > > that we were focused on other topics.
> > > 
> > > Would the general approach you envision be based on emitting all code
> > > generated by compilation of all objects under kernel/tracing and
> > > kernel/events into a specific "nokprobes" text section of the kernel ?
> > > Perhaps we could create a specific linker scripts for those directories,
> > > or do you have in mind a neater way to do this ?  
> > 
> > .kprobes.text section still exists. As I pointed in previous mail, I don't
> > think we have to put all those code into that section. But if you want,
> > it is acceptable to have a kconfig which push most of those ftrace related
> > code into .kprobes.text section.  
> 
> Or, we can check it by ftrace_location_range() as below patch.
> 
> Note that as a side-effect, we can not trace functions in trace_kprobe.c
> anymore, and this means it is hard to me to make a testcase of kprobe events.
> It was the easiest (and maintainable) way to make test cases... sigh.
> 
> -
> tracing: kprobes: Prohibit probing on notrace function
> 
> From: Masami Hiramatsu 
> 
> Prohibit kprobe-events probing on notrace function.
> Since probing on the notrace function can cause recursive
> event call. In most case those are just skipped, but
> in some case it falls into infinit recursive call.
> 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/trace/trace_kprobe.c |   23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5ce9b8cf7be3..7404b012e51a 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> trace_event_file *file)
>   return ret;
>  }
>  
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +static bool within_notrace_func(struct trace_kprobe *tk)
> +{
> + unsigned long offset, size, addr;
> +
> + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> + addr += trace_kprobe_offset(tk);
> +
> + if (!kallsyms_lookup_size_offset(addr, , ))
> + return true;/* Out of range. */
> +
> + return !ftrace_location_range(addr - offset, addr - offset + size);
> +}
> +#else
> +#define within_notrace_func(tk)  (false)
> +#endif
> +
>  /* Internal register function - just handle k*probes and flags */
>  static int __register_trace_kprobe(struct trace_kprobe *tk)
>  {
> @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> *tk)
>   if (trace_probe_is_registered(>tp))
>   return -EINVAL;
>  
> + if (within_notrace_func(tk)) {
> + pr_warn("Could not probe notrace function %s\n",
> + trace_kprobe_symbol(tk));
> + return -EINVAL;
> + }
> +
>   for (i = 0; i < tk->tp.nr_args; i++)
>   traceprobe_update_arg(>tp.args[i]);
>  
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

Mathieu and Francis,

Looking back, this thread never got further. Would Masami's patch work
for you?

-- Steve


On Sat, 17 Mar 2018 10:22:11 +0900
Masami Hiramatsu  wrote:

> On Sat, 17 Mar 2018 09:13:34 +0900
> Masami Hiramatsu  wrote:
> 
> > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> > Mathieu Desnoyers  wrote:
> >   
> > > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > >   
> > > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > > Steven Rostedt  wrote:
> > > >   
> > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > > >> saying that I don't have time to fix it now, but would be happy to
> > > >> accept patches if someone else does so.  
> > > > 
> > > > And looking at what I replied before for the original patch. It would
> > > > probably be a good idea to blacklist directories. Like we do with
> > > > function tracing. We probably should black list both kernel/tracing and
> > > > kernel/events from being probed.
> > > > 
> > > > Did this come up at plumbers? You were there too, I don't remember
> > > > discussing it there.  
> > > 
> > > I don't remember this coming up last Plumbers nor KS neither, given
> > > that we were focused on other topics.
> > > 
> > > Would the general approach you envision be based on emitting all code
> > > generated by compilation of all objects under kernel/tracing and
> > > kernel/events into a specific "nokprobes" text section of the kernel ?
> > > Perhaps we could create a specific linker scripts for those directories,
> > > or do you have in mind a neater way to do this ?  
> > 
> > .kprobes.text section still exists. As I pointed in previous mail, I don't
> > think we have to put all those code into that section. But if you want,
> > it is acceptable to have a kconfig which push most of those ftrace related
> > code into .kprobes.text section.  
> 
> Or, we can check it by ftrace_location_range() as below patch.
> 
> Note that as a side-effect, we can not trace functions in trace_kprobe.c
> anymore, and this means it is hard to me to make a testcase of kprobe events.
> It was the easiest (and maintainable) way to make test cases... sigh.
> 
> -
> tracing: kprobes: Prohibit probing on notrace function
> 
> From: Masami Hiramatsu 
> 
> Prohibit kprobe-events probing on notrace function.
> Since probing on the notrace function can cause recursive
> event call. In most case those are just skipped, but
> in some case it falls into infinit recursive call.
> 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/trace/trace_kprobe.c |   23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5ce9b8cf7be3..7404b012e51a 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> trace_event_file *file)
>   return ret;
>  }
>  
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +static bool within_notrace_func(struct trace_kprobe *tk)
> +{
> + unsigned long offset, size, addr;
> +
> + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> + addr += trace_kprobe_offset(tk);
> +
> + if (!kallsyms_lookup_size_offset(addr, , ))
> + return true;/* Out of range. */
> +
> + return !ftrace_location_range(addr - offset, addr - offset + size);
> +}
> +#else
> +#define within_notrace_func(tk)  (false)
> +#endif
> +
>  /* Internal register function - just handle k*probes and flags */
>  static int __register_trace_kprobe(struct trace_kprobe *tk)
>  {
> @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> *tk)
>   if (trace_probe_is_registered(>tp))
>   return -EINVAL;
>  
> + if (within_notrace_func(tk)) {
> + pr_warn("Could not probe notrace function %s\n",
> + trace_kprobe_symbol(tk));
> + return -EINVAL;
> + }
> +
>   for (i = 0; i < tk->tp.nr_args; i++)
>   traceprobe_update_arg(>tp.args[i]);
>  
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 23:01:54 -0400
Steven Rostedt  wrote:

> On Sat, 17 Mar 2018 10:22:11 +0900
> Masami Hiramatsu  wrote:
> 
> > Or, we can check it by ftrace_location_range() as below patch.
> 
> Cute ;-)
> 
> > 
> > Note that as a side-effect, we can not trace functions in trace_kprobe.c
> > anymore, and this means it is hard to me to make a testcase of kprobe 
> > events.
> > It was the easiest (and maintainable) way to make test cases... sigh.
> > 
> > -
> > tracing: kprobes: Prohibit probing on notrace function
> > 
> > From: Masami Hiramatsu 
> > 
> > Prohibit kprobe-events probing on notrace function.
> > Since probing on the notrace function can cause recursive
> > event call. In most case those are just skipped, but
> > in some case it falls into infinit recursive call.
> > 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/trace/trace_kprobe.c |   23 +++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 5ce9b8cf7be3..7404b012e51a 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> > trace_event_file *file)
> > return ret;
> >  }
> >  
> > +#ifdef CONFIG_KPROBES_ON_FTRACE
> > +static bool within_notrace_func(struct trace_kprobe *tk)
> > +{
> > +   unsigned long offset, size, addr;
> > +
> > +   addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> > +   addr += trace_kprobe_offset(tk);
> > +
> > +   if (!kallsyms_lookup_size_offset(addr, , ))
> > +   return true;/* Out of range. */
> > +
> > +   return !ftrace_location_range(addr - offset, addr - offset + size);
> > +}
> > +#else
> > +#define within_notrace_func(tk)(false)
> > +#endif
> > +
> >  /* Internal register function - just handle k*probes and flags */
> >  static int __register_trace_kprobe(struct trace_kprobe *tk)
> >  {
> > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> > *tk)
> > if (trace_probe_is_registered(>tp))
> > return -EINVAL;
> >  
> > +   if (within_notrace_func(tk)) {
> > +   pr_warn("Could not probe notrace function %s\n",
> > +   trace_kprobe_symbol(tk));
> > +   return -EINVAL;
> > +   }
> 
> This will prevent probing assembly code. Do we want to do that? Or is
> kprobes already prohibited from doing so?

No, kprobes itself can probe any assembly code except for the area where
can cause recursive hit as I explained. So anyway we still need to
mark those functions NOKPROBE_SYMBOL.

And note that this just prevent to probe those notrace code *via kprobe_events*
interface. So pure kprobe user (like systemtap) is not effected by this change.

So, maybe we would better to provide another debug knob which skips above check.
This is only for non-experienced admins :)

Thanks,

> 
> -- Steve
> 
> 
> > +
> > for (i = 0; i < tk->tp.nr_args; i++)
> > traceprobe_update_arg(>tp.args[i]);
> >  
> > 
> 


-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 23:01:54 -0400
Steven Rostedt  wrote:

> On Sat, 17 Mar 2018 10:22:11 +0900
> Masami Hiramatsu  wrote:
> 
> > Or, we can check it by ftrace_location_range() as below patch.
> 
> Cute ;-)
> 
> > 
> > Note that as a side-effect, we can not trace functions in trace_kprobe.c
> > anymore, and this means it is hard to me to make a testcase of kprobe 
> > events.
> > It was the easiest (and maintainable) way to make test cases... sigh.
> > 
> > -
> > tracing: kprobes: Prohibit probing on notrace function
> > 
> > From: Masami Hiramatsu 
> > 
> > Prohibit kprobe-events probing on notrace function.
> > Since probing on the notrace function can cause recursive
> > event call. In most case those are just skipped, but
> > in some case it falls into infinit recursive call.
> > 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/trace/trace_kprobe.c |   23 +++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 5ce9b8cf7be3..7404b012e51a 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> > trace_event_file *file)
> > return ret;
> >  }
> >  
> > +#ifdef CONFIG_KPROBES_ON_FTRACE
> > +static bool within_notrace_func(struct trace_kprobe *tk)
> > +{
> > +   unsigned long offset, size, addr;
> > +
> > +   addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> > +   addr += trace_kprobe_offset(tk);
> > +
> > +   if (!kallsyms_lookup_size_offset(addr, , ))
> > +   return true;/* Out of range. */
> > +
> > +   return !ftrace_location_range(addr - offset, addr - offset + size);
> > +}
> > +#else
> > +#define within_notrace_func(tk)(false)
> > +#endif
> > +
> >  /* Internal register function - just handle k*probes and flags */
> >  static int __register_trace_kprobe(struct trace_kprobe *tk)
> >  {
> > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> > *tk)
> > if (trace_probe_is_registered(>tp))
> > return -EINVAL;
> >  
> > +   if (within_notrace_func(tk)) {
> > +   pr_warn("Could not probe notrace function %s\n",
> > +   trace_kprobe_symbol(tk));
> > +   return -EINVAL;
> > +   }
> 
> This will prevent probing assembly code. Do we want to do that? Or is
> kprobes already prohibited from doing so?

No, kprobes itself can probe any assembly code except for the area where
can cause recursive hit as I explained. So anyway we still need to
mark those functions NOKPROBE_SYMBOL.

And note that this just prevent to probe those notrace code *via kprobe_events*
interface. So pure kprobe user (like systemtap) is not effected by this change.

So, maybe we would better to provide another debug knob which skips above check.
This is only for non-experienced admins :)

Thanks,

> 
> -- Steve
> 
> 
> > +
> > for (i = 0; i < tk->tp.nr_args; i++)
> > traceprobe_update_arg(>tp.args[i]);
> >  
> > 
> 


-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Sat, 17 Mar 2018 10:22:11 +0900
Masami Hiramatsu  wrote:

> Or, we can check it by ftrace_location_range() as below patch.

Cute ;-)

> 
> Note that as a side-effect, we can not trace functions in trace_kprobe.c
> anymore, and this means it is hard to me to make a testcase of kprobe events.
> It was the easiest (and maintainable) way to make test cases... sigh.
> 
> -
> tracing: kprobes: Prohibit probing on notrace function
> 
> From: Masami Hiramatsu 
> 
> Prohibit kprobe-events probing on notrace function.
> Since probing on the notrace function can cause recursive
> event call. In most case those are just skipped, but
> in some case it falls into infinit recursive call.
> 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/trace/trace_kprobe.c |   23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5ce9b8cf7be3..7404b012e51a 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> trace_event_file *file)
>   return ret;
>  }
>  
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +static bool within_notrace_func(struct trace_kprobe *tk)
> +{
> + unsigned long offset, size, addr;
> +
> + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> + addr += trace_kprobe_offset(tk);
> +
> + if (!kallsyms_lookup_size_offset(addr, , ))
> + return true;/* Out of range. */
> +
> + return !ftrace_location_range(addr - offset, addr - offset + size);
> +}
> +#else
> +#define within_notrace_func(tk)  (false)
> +#endif
> +
>  /* Internal register function - just handle k*probes and flags */
>  static int __register_trace_kprobe(struct trace_kprobe *tk)
>  {
> @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> *tk)
>   if (trace_probe_is_registered(>tp))
>   return -EINVAL;
>  
> + if (within_notrace_func(tk)) {
> + pr_warn("Could not probe notrace function %s\n",
> + trace_kprobe_symbol(tk));
> + return -EINVAL;
> + }

This will prevent probing assembly code. Do we want to do that? Or is
kprobes already prohibited from doing so?

-- Steve


> +
>   for (i = 0; i < tk->tp.nr_args; i++)
>   traceprobe_update_arg(>tp.args[i]);
>  
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Sat, 17 Mar 2018 10:22:11 +0900
Masami Hiramatsu  wrote:

> Or, we can check it by ftrace_location_range() as below patch.

Cute ;-)

> 
> Note that as a side-effect, we can not trace functions in trace_kprobe.c
> anymore, and this means it is hard to me to make a testcase of kprobe events.
> It was the easiest (and maintainable) way to make test cases... sigh.
> 
> -
> tracing: kprobes: Prohibit probing on notrace function
> 
> From: Masami Hiramatsu 
> 
> Prohibit kprobe-events probing on notrace function.
> Since probing on the notrace function can cause recursive
> event call. In most case those are just skipped, but
> in some case it falls into infinit recursive call.
> 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/trace/trace_kprobe.c |   23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5ce9b8cf7be3..7404b012e51a 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
> trace_event_file *file)
>   return ret;
>  }
>  
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +static bool within_notrace_func(struct trace_kprobe *tk)
> +{
> + unsigned long offset, size, addr;
> +
> + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
> + addr += trace_kprobe_offset(tk);
> +
> + if (!kallsyms_lookup_size_offset(addr, , ))
> + return true;/* Out of range. */
> +
> + return !ftrace_location_range(addr - offset, addr - offset + size);
> +}
> +#else
> +#define within_notrace_func(tk)  (false)
> +#endif
> +
>  /* Internal register function - just handle k*probes and flags */
>  static int __register_trace_kprobe(struct trace_kprobe *tk)
>  {
> @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe 
> *tk)
>   if (trace_probe_is_registered(>tp))
>   return -EINVAL;
>  
> + if (within_notrace_func(tk)) {
> + pr_warn("Could not probe notrace function %s\n",
> + trace_kprobe_symbol(tk));
> + return -EINVAL;
> + }

This will prevent probing assembly code. Do we want to do that? Or is
kprobes already prohibited from doing so?

-- Steve


> +
>   for (i = 0; i < tk->tp.nr_args; i++)
>   traceprobe_update_arg(>tp.args[i]);
>  
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Sat, 17 Mar 2018 09:13:34 +0900
Masami Hiramatsu  wrote:

> On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> Mathieu Desnoyers  wrote:
> 
> > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > 
> > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > Steven Rostedt  wrote:
> > > 
> > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > >> saying that I don't have time to fix it now, but would be happy to
> > >> accept patches if someone else does so.
> > > 
> > > And looking at what I replied before for the original patch. It would
> > > probably be a good idea to blacklist directories. Like we do with
> > > function tracing. We probably should black list both kernel/tracing and
> > > kernel/events from being probed.
> > > 
> > > Did this come up at plumbers? You were there too, I don't remember
> > > discussing it there.
> > 
> > I don't remember this coming up last Plumbers nor KS neither, given
> > that we were focused on other topics.
> > 
> > Would the general approach you envision be based on emitting all code
> > generated by compilation of all objects under kernel/tracing and
> > kernel/events into a specific "nokprobes" text section of the kernel ?
> > Perhaps we could create a specific linker scripts for those directories,
> > or do you have in mind a neater way to do this ?
> 
> .kprobes.text section still exists. As I pointed in previous mail, I don't
> think we have to put all those code into that section. But if you want,
> it is acceptable to have a kconfig which push most of those ftrace related
> code into .kprobes.text section.

Or, we can check it by ftrace_location_range() as below patch.

Note that as a side-effect, we can not trace functions in trace_kprobe.c
anymore, and this means it is hard to me to make a testcase of kprobe events.
It was the easiest (and maintainable) way to make test cases... sigh.

-
tracing: kprobes: Prohibit probing on notrace function

From: Masami Hiramatsu 

Prohibit kprobe-events probing on notrace function.
Since probing on the notrace function can cause recursive
event call. In most case those are just skipped, but
in some case it falls into infinit recursive call.

Signed-off-by: Masami Hiramatsu 
---
 kernel/trace/trace_kprobe.c |   23 +++
 1 file changed, 23 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5ce9b8cf7be3..7404b012e51a 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
trace_event_file *file)
return ret;
 }
 
+#ifdef CONFIG_KPROBES_ON_FTRACE
+static bool within_notrace_func(struct trace_kprobe *tk)
+{
+   unsigned long offset, size, addr;
+
+   addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
+   addr += trace_kprobe_offset(tk);
+
+   if (!kallsyms_lookup_size_offset(addr, , ))
+   return true;/* Out of range. */
+
+   return !ftrace_location_range(addr - offset, addr - offset + size);
+}
+#else
+#define within_notrace_func(tk)(false)
+#endif
+
 /* Internal register function - just handle k*probes and flags */
 static int __register_trace_kprobe(struct trace_kprobe *tk)
 {
@@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
if (trace_probe_is_registered(>tp))
return -EINVAL;
 
+   if (within_notrace_func(tk)) {
+   pr_warn("Could not probe notrace function %s\n",
+   trace_kprobe_symbol(tk));
+   return -EINVAL;
+   }
+
for (i = 0; i < tk->tp.nr_args; i++)
traceprobe_update_arg(>tp.args[i]);
 

-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Sat, 17 Mar 2018 09:13:34 +0900
Masami Hiramatsu  wrote:

> On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> Mathieu Desnoyers  wrote:
> 
> > - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> > 
> > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > Steven Rostedt  wrote:
> > > 
> > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > >> saying that I don't have time to fix it now, but would be happy to
> > >> accept patches if someone else does so.
> > > 
> > > And looking at what I replied before for the original patch. It would
> > > probably be a good idea to blacklist directories. Like we do with
> > > function tracing. We probably should black list both kernel/tracing and
> > > kernel/events from being probed.
> > > 
> > > Did this come up at plumbers? You were there too, I don't remember
> > > discussing it there.
> > 
> > I don't remember this coming up last Plumbers nor KS neither, given
> > that we were focused on other topics.
> > 
> > Would the general approach you envision be based on emitting all code
> > generated by compilation of all objects under kernel/tracing and
> > kernel/events into a specific "nokprobes" text section of the kernel ?
> > Perhaps we could create a specific linker scripts for those directories,
> > or do you have in mind a neater way to do this ?
> 
> .kprobes.text section still exists. As I pointed in previous mail, I don't
> think we have to put all those code into that section. But if you want,
> it is acceptable to have a kconfig which push most of those ftrace related
> code into .kprobes.text section.

Or, we can check it by ftrace_location_range() as below patch.

Note that as a side-effect, we can not trace functions in trace_kprobe.c
anymore, and this means it is hard to me to make a testcase of kprobe events.
It was the easiest (and maintainable) way to make test cases... sigh.

-
tracing: kprobes: Prohibit probing on notrace function

From: Masami Hiramatsu 

Prohibit kprobe-events probing on notrace function.
Since probing on the notrace function can cause recursive
event call. In most case those are just skipped, but
in some case it falls into infinit recursive call.

Signed-off-by: Masami Hiramatsu 
---
 kernel/trace/trace_kprobe.c |   23 +++
 1 file changed, 23 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5ce9b8cf7be3..7404b012e51a 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct 
trace_event_file *file)
return ret;
 }
 
+#ifdef CONFIG_KPROBES_ON_FTRACE
+static bool within_notrace_func(struct trace_kprobe *tk)
+{
+   unsigned long offset, size, addr;
+
+   addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
+   addr += trace_kprobe_offset(tk);
+
+   if (!kallsyms_lookup_size_offset(addr, , ))
+   return true;/* Out of range. */
+
+   return !ftrace_location_range(addr - offset, addr - offset + size);
+}
+#else
+#define within_notrace_func(tk)(false)
+#endif
+
 /* Internal register function - just handle k*probes and flags */
 static int __register_trace_kprobe(struct trace_kprobe *tk)
 {
@@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
if (trace_probe_is_registered(>tp))
return -EINVAL;
 
+   if (within_notrace_func(tk)) {
+   pr_warn("Could not probe notrace function %s\n",
+   trace_kprobe_symbol(tk));
+   return -EINVAL;
+   }
+
for (i = 0; i < tk->tp.nr_args; i++)
traceprobe_update_arg(>tp.args[i]);
 

-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
Mathieu Desnoyers  wrote:

> - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> 
> > On Fri, 16 Mar 2018 12:41:34 -0400
> > Steven Rostedt  wrote:
> > 
> >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> >> saying that I don't have time to fix it now, but would be happy to
> >> accept patches if someone else does so.
> > 
> > And looking at what I replied before for the original patch. It would
> > probably be a good idea to blacklist directories. Like we do with
> > function tracing. We probably should black list both kernel/tracing and
> > kernel/events from being probed.
> > 
> > Did this come up at plumbers? You were there too, I don't remember
> > discussing it there.
> 
> I don't remember this coming up last Plumbers nor KS neither, given
> that we were focused on other topics.
> 
> Would the general approach you envision be based on emitting all code
> generated by compilation of all objects under kernel/tracing and
> kernel/events into a specific "nokprobes" text section of the kernel ?
> Perhaps we could create a specific linker scripts for those directories,
> or do you have in mind a neater way to do this ?

.kprobes.text section still exists. As I pointed in previous mail, I don't
think we have to put all those code into that section. But if you want,
it is acceptable to have a kconfig which push most of those ftrace related
code into .kprobes.text section.

Thank,

> 
> Thanks,
> 
> Mathieu
> 
> > 
> > -- Steve
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com


-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
Mathieu Desnoyers  wrote:

> - On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:
> 
> > On Fri, 16 Mar 2018 12:41:34 -0400
> > Steven Rostedt  wrote:
> > 
> >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> >> saying that I don't have time to fix it now, but would be happy to
> >> accept patches if someone else does so.
> > 
> > And looking at what I replied before for the original patch. It would
> > probably be a good idea to blacklist directories. Like we do with
> > function tracing. We probably should black list both kernel/tracing and
> > kernel/events from being probed.
> > 
> > Did this come up at plumbers? You were there too, I don't remember
> > discussing it there.
> 
> I don't remember this coming up last Plumbers nor KS neither, given
> that we were focused on other topics.
> 
> Would the general approach you envision be based on emitting all code
> generated by compilation of all objects under kernel/tracing and
> kernel/events into a specific "nokprobes" text section of the kernel ?
> Perhaps we could create a specific linker scripts for those directories,
> or do you have in mind a neater way to do this ?

.kprobes.text section still exists. As I pointed in previous mail, I don't
think we have to put all those code into that section. But if you want,
it is acceptable to have a kconfig which push most of those ftrace related
code into .kprobes.text section.

Thank,

> 
> Thanks,
> 
> Mathieu
> 
> > 
> > -- Steve
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com


-- 
Masami Hiramatsu 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 12:28:59 -0400 (EDT)
Mathieu Desnoyers  wrote:

> - On Mar 16, 2018, at 11:25 AM, rostedt rost...@goodmis.org wrote:
> 
> > On Fri, 16 Mar 2018 11:18:25 -0400
> > Francis Deslauriers  wrote:
> > 
> >> Hi Steven,
> >> 
> >> I completely forgot about this issue until recently when I encountered it 
> >> again.
> >> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
> >> seems to be causing problems.
> >> 
> >> Placing kretprobes like in the following configuration crashes my
> >> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
> >> 
> >> config 1:
> >> echo "r:event_1 __fdget" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 2:
> >> echo "r:event_1 __fdget_pos" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 3:
> >> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> config 4:
> >> echo 'r:event_1 sys_open' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> Here is my kernel config [1]:
> >> 
> >> In a previous email [2], you mentioned that you would like to add the
> >> ftrace-related symbols to a section to un-blacklist them all at once
> >> on demand but wanted to discuss it at Linux Plumbers. Do you still
> >> think that it's the right approach?
> > 
> > We probably didn't discuss it (as there was a lot to discuss, and this
> > was probably overshadowed by that). But yes, you should not probe
> > ftrace called functions. That is guaranteed to crash and that crash is
> > not a bug, but a feature.
> 
> Are you really arguing that crashing the kernel from an ABI visible from
> userspace (even if it's only root user) is not a bug ? You are joking right ?
> Is there an EXPERIMENTAL config option that people need to select in order to
> make sure those ftrace interfaces don't end up on production systems ?
> 
> > 
> > The ftrace and ring buffer files should be blacklisted from being
> > probed. Perhaps the entire directory.
> 
> All code reachable from a kprobe handler should be blacklisted from
> kprobes, yes.

No, actually that is incorrect, since user can make a module to probe
those functions from outside of ftrace via kprobes.

The problematic functions are the functions called between probe-hit
and set current_kprobe (iow, kprobe_ftrace_handler()). After setting
current_kprobe, all probe hits are just skipped.

The issue that Francis faced is actual bug. It must be blacklisted,
since it is called before calling kprobe_ftrace_handler().

target_func
  -> fentry-code
 -> ftrace-code
  -> kprobe_ftrace_handler
(safe area)
  <- ret
  <- ret (ftrace-code)
  <- ret (fentry-code)

In above model, functions in fentry-code and ftrace-code must be
blacklisted.

Thanks,

> > Anyway, I don't see this as much of an urgent matter, as it's one of
> > those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> > cases. And there's a lot of urgent issues that currently need to be
> > dealt with.
> 
> OK, short-term we'll remove everything related to ftrace functions
> from our CI fuzzer coverage. Arguably, the fact that a root user can
> crash the kernel through tracefs files is not that great security-wise
> though.
> 
> Considering that our current focus is to test the kprobe instrumentation
> layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
> instead, which should take care of removing crashes introduced by ftrace
> from our fuzzing results.
> 
> Thanks,
> 
> Mathieu
> 
> 
> > 
> > -- Steve
> > 
> > 
> >> 
> >> I can easily test any patch regarding this issue.
> >> 
> >> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
> >> [2] https://lkml.org/lkml/2017/7/14/568
> >> 
> >> Thank you,
> >> 
> >> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> >> > On Fri, 14 Jul 2017 10:58:35 -0400
> >> > Francis Deslauriers  wrote:
> >> >  
> >> >> This function is called when a kprobe is hit. Thus it should be
> >> >> blacklisted to prevent kprobe to be triggered by kprobes.
> >> >>
> >> >> Signed-off-by: Francis Deslauriers 
> >> >> ---
> >> >>  kernel/trace/ftrace.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >>
> >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> >> index b308be3..c473d9b 100644
> >> >> --- a/kernel/trace/ftrace.c
> >> >> +++ b/kernel/trace/ftrace.c
> >> >> @@ -36,6 +36,7 @@
> >> >>
> >> >>  #include 
> >> >>
> >> >> +#include 
> >> >>  #include 
> >> >>  #include 
> >> >>
> >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long 
> >> >> ip,
> >> >> unsigned long parent_ip,
> >> >>   preempt_enable_notrace();
> >> >>   trace_clear_recursion(bit);
> >> >>  }
> >> >> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Masami Hiramatsu]

On Fri, 16 Mar 2018 12:28:59 -0400 (EDT)
Mathieu Desnoyers  wrote:

> - On Mar 16, 2018, at 11:25 AM, rostedt rost...@goodmis.org wrote:
> 
> > On Fri, 16 Mar 2018 11:18:25 -0400
> > Francis Deslauriers  wrote:
> > 
> >> Hi Steven,
> >> 
> >> I completely forgot about this issue until recently when I encountered it 
> >> again.
> >> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
> >> seems to be causing problems.
> >> 
> >> Placing kretprobes like in the following configuration crashes my
> >> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
> >> 
> >> config 1:
> >> echo "r:event_1 __fdget" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 2:
> >> echo "r:event_1 __fdget_pos" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 3:
> >> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> config 4:
> >> echo 'r:event_1 sys_open' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> Here is my kernel config [1]:
> >> 
> >> In a previous email [2], you mentioned that you would like to add the
> >> ftrace-related symbols to a section to un-blacklist them all at once
> >> on demand but wanted to discuss it at Linux Plumbers. Do you still
> >> think that it's the right approach?
> > 
> > We probably didn't discuss it (as there was a lot to discuss, and this
> > was probably overshadowed by that). But yes, you should not probe
> > ftrace called functions. That is guaranteed to crash and that crash is
> > not a bug, but a feature.
> 
> Are you really arguing that crashing the kernel from an ABI visible from
> userspace (even if it's only root user) is not a bug ? You are joking right ?
> Is there an EXPERIMENTAL config option that people need to select in order to
> make sure those ftrace interfaces don't end up on production systems ?
> 
> > 
> > The ftrace and ring buffer files should be blacklisted from being
> > probed. Perhaps the entire directory.
> 
> All code reachable from a kprobe handler should be blacklisted from
> kprobes, yes.

No, actually that is incorrect, since user can make a module to probe
those functions from outside of ftrace via kprobes.

The problematic functions are the functions called between probe-hit
and set current_kprobe (iow, kprobe_ftrace_handler()). After setting
current_kprobe, all probe hits are just skipped.

The issue that Francis faced is actual bug. It must be blacklisted,
since it is called before calling kprobe_ftrace_handler().

target_func
  -> fentry-code
 -> ftrace-code
  -> kprobe_ftrace_handler
(safe area)
  <- ret
  <- ret (ftrace-code)
  <- ret (fentry-code)

In above model, functions in fentry-code and ftrace-code must be
blacklisted.

Thanks,

> > Anyway, I don't see this as much of an urgent matter, as it's one of
> > those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> > cases. And there's a lot of urgent issues that currently need to be
> > dealt with.
> 
> OK, short-term we'll remove everything related to ftrace functions
> from our CI fuzzer coverage. Arguably, the fact that a root user can
> crash the kernel through tracefs files is not that great security-wise
> though.
> 
> Considering that our current focus is to test the kprobe instrumentation
> layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
> instead, which should take care of removing crashes introduced by ftrace
> from our fuzzing results.
> 
> Thanks,
> 
> Mathieu
> 
> 
> > 
> > -- Steve
> > 
> > 
> >> 
> >> I can easily test any patch regarding this issue.
> >> 
> >> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
> >> [2] https://lkml.org/lkml/2017/7/14/568
> >> 
> >> Thank you,
> >> 
> >> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> >> > On Fri, 14 Jul 2017 10:58:35 -0400
> >> > Francis Deslauriers  wrote:
> >> >  
> >> >> This function is called when a kprobe is hit. Thus it should be
> >> >> blacklisted to prevent kprobe to be triggered by kprobes.
> >> >>
> >> >> Signed-off-by: Francis Deslauriers 
> >> >> ---
> >> >>  kernel/trace/ftrace.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >>
> >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> >> index b308be3..c473d9b 100644
> >> >> --- a/kernel/trace/ftrace.c
> >> >> +++ b/kernel/trace/ftrace.c
> >> >> @@ -36,6 +36,7 @@
> >> >>
> >> >>  #include 
> >> >>
> >> >> +#include 
> >> >>  #include 
> >> >>  #include 
> >> >>
> >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long 
> >> >> ip,
> >> >> unsigned long parent_ip,
> >> >>   preempt_enable_notrace();
> >> >>   trace_clear_recursion(bit);
> >> >>  }
> >> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
> >> >
> >> > Continuing from what I said in the other email, this is fixing a
> >> > symptom and not the problem. The real fix will be 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
Mathieu Desnoyers  wrote:

> Would the general approach you envision be based on emitting all code
> generated by compilation of all objects under kernel/tracing and
> kernel/events into a specific "nokprobes" text section of the kernel ?
> Perhaps we could create a specific linker scripts for those directories,
> or do you have in mind a neater way to do this ?

I was thinking of adding it to the objtool work. I need to consolidate
the recordmcount code and objtool for doing this, and that is on my
agenda (it has to do with some of the current "urgent" needs).

But this will take a bit of effort. In the mean time and not against
just adding the whack-a-mole approach and add the functions that the
original patch selected as nokprobes.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
Mathieu Desnoyers  wrote:

> Would the general approach you envision be based on emitting all code
> generated by compilation of all objects under kernel/tracing and
> kernel/events into a specific "nokprobes" text section of the kernel ?
> Perhaps we could create a specific linker scripts for those directories,
> or do you have in mind a neater way to do this ?

I was thinking of adding it to the objtool work. I need to consolidate
the recordmcount code and objtool for doing this, and that is on my
agenda (it has to do with some of the current "urgent" needs).

But this will take a bit of effort. In the mean time and not against
just adding the whack-a-mole approach and add the functions that the
original patch selected as nokprobes.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Mathieu Desnoyers]

- On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:

> On Fri, 16 Mar 2018 12:41:34 -0400
> Steven Rostedt  wrote:
> 
>> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
>> saying that I don't have time to fix it now, but would be happy to
>> accept patches if someone else does so.
> 
> And looking at what I replied before for the original patch. It would
> probably be a good idea to blacklist directories. Like we do with
> function tracing. We probably should black list both kernel/tracing and
> kernel/events from being probed.
> 
> Did this come up at plumbers? You were there too, I don't remember
> discussing it there.

I don't remember this coming up last Plumbers nor KS neither, given
that we were focused on other topics.

Would the general approach you envision be based on emitting all code
generated by compilation of all objects under kernel/tracing and
kernel/events into a specific "nokprobes" text section of the kernel ?
Perhaps we could create a specific linker scripts for those directories,
or do you have in mind a neater way to do this ?

Thanks,

Mathieu

> 
> -- Steve

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Mathieu Desnoyers]

- On Mar 16, 2018, at 12:48 PM, rostedt rost...@goodmis.org wrote:

> On Fri, 16 Mar 2018 12:41:34 -0400
> Steven Rostedt  wrote:
> 
>> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
>> saying that I don't have time to fix it now, but would be happy to
>> accept patches if someone else does so.
> 
> And looking at what I replied before for the original patch. It would
> probably be a good idea to blacklist directories. Like we do with
> function tracing. We probably should black list both kernel/tracing and
> kernel/events from being probed.
> 
> Did this come up at plumbers? You were there too, I don't remember
> discussing it there.

I don't remember this coming up last Plumbers nor KS neither, given
that we were focused on other topics.

Would the general approach you envision be based on emitting all code
generated by compilation of all objects under kernel/tracing and
kernel/events into a specific "nokprobes" text section of the kernel ?
Perhaps we could create a specific linker scripts for those directories,
or do you have in mind a neater way to do this ?

Thanks,

Mathieu

> 
> -- Steve

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 12:41:34 -0400
Steven Rostedt  wrote:

> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> saying that I don't have time to fix it now, but would be happy to
> accept patches if someone else does so.

And looking at what I replied before for the original patch. It would
probably be a good idea to blacklist directories. Like we do with
function tracing. We probably should black list both kernel/tracing and
kernel/events from being probed.

Did this come up at plumbers? You were there too, I don't remember
discussing it there.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 12:41:34 -0400
Steven Rostedt  wrote:

> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> saying that I don't have time to fix it now, but would be happy to
> accept patches if someone else does so.

And looking at what I replied before for the original patch. It would
probably be a good idea to blacklist directories. Like we do with
function tracing. We probably should black list both kernel/tracing and
kernel/events from being probed.

Did this come up at plumbers? You were there too, I don't remember
discussing it there.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 12:28:59 -0400 (EDT)
Mathieu Desnoyers  wrote:

> > We probably didn't discuss it (as there was a lot to discuss, and this
> > was probably overshadowed by that). But yes, you should not probe
> > ftrace called functions. That is guaranteed to crash and that crash is
> > not a bug, but a feature.  
> 
> Are you really arguing that crashing the kernel from an ABI visible from
> userspace (even if it's only root user) is not a bug ? You are joking right ?
> Is there an EXPERIMENTAL config option that people need to select in order to
> make sure those ftrace interfaces don't end up on production systems ?

No I'm not. And yes there is. Disable kprobes. kprobes is much more
dangerous than ftrace, and its kprobes that is crashing not ftrace.

Heck we have "echo c > /proc/sysrq-trigger"

So yes, you can easily crash the kernel via root. If you can load a
module, you can crash the kernel. There's a thousand ways to crash a
kernel. This is why most of the fuzzer testing is done as non-root,
because doing it as root will do more than just crash the system, it may
corrupt it enough that you can no longer boot it.

I see below you are doing fuzzing testing too as root. Hopefully you
limit those tests because yes, things can get really bad.

> 
> > 
> > The ftrace and ring buffer files should be blacklisted from being
> > probed. Perhaps the entire directory.  
> 
> All code reachable from a kprobe handler should be blacklisted from
> kprobes, yes.

The problem is that that list constantly changes. There's been cases we
try to prevent things called by nmi do not get called, but it ended up
being every helper utility can be called in that context.

> 
> > 
> > Anyway, I don't see this as much of an urgent matter, as it's one of
> > those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> > cases. And there's a lot of urgent issues that currently need to be
> > dealt with.  
> 
> OK, short-term we'll remove everything related to ftrace functions
> from our CI fuzzer coverage. Arguably, the fact that a root user can
> crash the kernel through tracefs files is not that great security-wise
> though.

Note, probes are not a normal API. I test the hell out of the other
ftrace interfaces and if it blows up I fix it. But adding probes into
random parts of the kernel is very dangerous, and not something I care
to test. And sure, if you are worried about root killing the system,
disable kprobes.


> 
> Considering that our current focus is to test the kprobe instrumentation
> layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
> instead, which should take care of removing crashes introduced by ftrace
> from our fuzzing results.

Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
saying that I don't have time to fix it now, but would be happy to
accept patches if someone else does so.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 12:28:59 -0400 (EDT)
Mathieu Desnoyers  wrote:

> > We probably didn't discuss it (as there was a lot to discuss, and this
> > was probably overshadowed by that). But yes, you should not probe
> > ftrace called functions. That is guaranteed to crash and that crash is
> > not a bug, but a feature.  
> 
> Are you really arguing that crashing the kernel from an ABI visible from
> userspace (even if it's only root user) is not a bug ? You are joking right ?
> Is there an EXPERIMENTAL config option that people need to select in order to
> make sure those ftrace interfaces don't end up on production systems ?

No I'm not. And yes there is. Disable kprobes. kprobes is much more
dangerous than ftrace, and its kprobes that is crashing not ftrace.

Heck we have "echo c > /proc/sysrq-trigger"

So yes, you can easily crash the kernel via root. If you can load a
module, you can crash the kernel. There's a thousand ways to crash a
kernel. This is why most of the fuzzer testing is done as non-root,
because doing it as root will do more than just crash the system, it may
corrupt it enough that you can no longer boot it.

I see below you are doing fuzzing testing too as root. Hopefully you
limit those tests because yes, things can get really bad.

> 
> > 
> > The ftrace and ring buffer files should be blacklisted from being
> > probed. Perhaps the entire directory.  
> 
> All code reachable from a kprobe handler should be blacklisted from
> kprobes, yes.

The problem is that that list constantly changes. There's been cases we
try to prevent things called by nmi do not get called, but it ended up
being every helper utility can be called in that context.

> 
> > 
> > Anyway, I don't see this as much of an urgent matter, as it's one of
> > those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> > cases. And there's a lot of urgent issues that currently need to be
> > dealt with.  
> 
> OK, short-term we'll remove everything related to ftrace functions
> from our CI fuzzer coverage. Arguably, the fact that a root user can
> crash the kernel through tracefs files is not that great security-wise
> though.

Note, probes are not a normal API. I test the hell out of the other
ftrace interfaces and if it blows up I fix it. But adding probes into
random parts of the kernel is very dangerous, and not something I care
to test. And sure, if you are worried about root killing the system,
disable kprobes.


> 
> Considering that our current focus is to test the kprobe instrumentation
> layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
> instead, which should take care of removing crashes introduced by ftrace
> from our fuzzing results.

Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
saying that I don't have time to fix it now, but would be happy to
accept patches if someone else does so.

-- Steve

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Mathieu Desnoyers]

- On Mar 16, 2018, at 11:25 AM, rostedt rost...@goodmis.org wrote:

> On Fri, 16 Mar 2018 11:18:25 -0400
> Francis Deslauriers  wrote:
> 
>> Hi Steven,
>> 
>> I completely forgot about this issue until recently when I encountered it 
>> again.
>> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
>> seems to be causing problems.
>> 
>> Placing kretprobes like in the following configuration crashes my
>> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
>> 
>> config 1:
>> echo "r:event_1 __fdget" >> kprobe_events
>> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
>> 
>> config 2:
>> echo "r:event_1 __fdget_pos" >> kprobe_events
>> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
>> 
>> config 3:
>> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
>> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
>> 
>> config 4:
>> echo 'r:event_1 sys_open' >> kprobe_events
>> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
>> 
>> Here is my kernel config [1]:
>> 
>> In a previous email [2], you mentioned that you would like to add the
>> ftrace-related symbols to a section to un-blacklist them all at once
>> on demand but wanted to discuss it at Linux Plumbers. Do you still
>> think that it's the right approach?
> 
> We probably didn't discuss it (as there was a lot to discuss, and this
> was probably overshadowed by that). But yes, you should not probe
> ftrace called functions. That is guaranteed to crash and that crash is
> not a bug, but a feature.

Are you really arguing that crashing the kernel from an ABI visible from
userspace (even if it's only root user) is not a bug ? You are joking right ?
Is there an EXPERIMENTAL config option that people need to select in order to
make sure those ftrace interfaces don't end up on production systems ?

> 
> The ftrace and ring buffer files should be blacklisted from being
> probed. Perhaps the entire directory.

All code reachable from a kprobe handler should be blacklisted from
kprobes, yes.

> 
> Anyway, I don't see this as much of an urgent matter, as it's one of
> those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> cases. And there's a lot of urgent issues that currently need to be
> dealt with.

OK, short-term we'll remove everything related to ftrace functions
from our CI fuzzer coverage. Arguably, the fact that a root user can
crash the kernel through tracefs files is not that great security-wise
though.

Considering that our current focus is to test the kprobe instrumentation
layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
instead, which should take care of removing crashes introduced by ftrace
from our fuzzing results.

Thanks,

Mathieu


> 
> -- Steve
> 
> 
>> 
>> I can easily test any patch regarding this issue.
>> 
>> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
>> [2] https://lkml.org/lkml/2017/7/14/568
>> 
>> Thank you,
>> 
>> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
>> > On Fri, 14 Jul 2017 10:58:35 -0400
>> > Francis Deslauriers  wrote:
>> >  
>> >> This function is called when a kprobe is hit. Thus it should be
>> >> blacklisted to prevent kprobe to be triggered by kprobes.
>> >>
>> >> Signed-off-by: Francis Deslauriers 
>> >> ---
>> >>  kernel/trace/ftrace.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> >> index b308be3..c473d9b 100644
>> >> --- a/kernel/trace/ftrace.c
>> >> +++ b/kernel/trace/ftrace.c
>> >> @@ -36,6 +36,7 @@
>> >>
>> >>  #include 
>> >>
>> >> +#include 
>> >>  #include 
>> >>  #include 
>> >>
>> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip,
>> >> unsigned long parent_ip,
>> >>   preempt_enable_notrace();
>> >>   trace_clear_recursion(bit);
>> >>  }
>> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
>> >
>> > Continuing from what I said in the other email, this is fixing a
>> > symptom and not the problem. The real fix will be much more involved. I
>> > have a good idea on how to accomplish it too.
>> >
>> > -- Steve
>> >
>> >  
>> >>
>> >>  /**
>> >>   * ftrace_ops_get_func - get the function a trampoline should call
>> >  
>> 
>> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Mathieu Desnoyers]

- On Mar 16, 2018, at 11:25 AM, rostedt rost...@goodmis.org wrote:

> On Fri, 16 Mar 2018 11:18:25 -0400
> Francis Deslauriers  wrote:
> 
>> Hi Steven,
>> 
>> I completely forgot about this issue until recently when I encountered it 
>> again.
>> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
>> seems to be causing problems.
>> 
>> Placing kretprobes like in the following configuration crashes my
>> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
>> 
>> config 1:
>> echo "r:event_1 __fdget" >> kprobe_events
>> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
>> 
>> config 2:
>> echo "r:event_1 __fdget_pos" >> kprobe_events
>> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
>> 
>> config 3:
>> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
>> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
>> 
>> config 4:
>> echo 'r:event_1 sys_open' >> kprobe_events
>> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
>> 
>> Here is my kernel config [1]:
>> 
>> In a previous email [2], you mentioned that you would like to add the
>> ftrace-related symbols to a section to un-blacklist them all at once
>> on demand but wanted to discuss it at Linux Plumbers. Do you still
>> think that it's the right approach?
> 
> We probably didn't discuss it (as there was a lot to discuss, and this
> was probably overshadowed by that). But yes, you should not probe
> ftrace called functions. That is guaranteed to crash and that crash is
> not a bug, but a feature.

Are you really arguing that crashing the kernel from an ABI visible from
userspace (even if it's only root user) is not a bug ? You are joking right ?
Is there an EXPERIMENTAL config option that people need to select in order to
make sure those ftrace interfaces don't end up on production systems ?

> 
> The ftrace and ring buffer files should be blacklisted from being
> probed. Perhaps the entire directory.

All code reachable from a kprobe handler should be blacklisted from
kprobes, yes.

> 
> Anyway, I don't see this as much of an urgent matter, as it's one of
> those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> cases. And there's a lot of urgent issues that currently need to be
> dealt with.

OK, short-term we'll remove everything related to ftrace functions
from our CI fuzzer coverage. Arguably, the fact that a root user can
crash the kernel through tracefs files is not that great security-wise
though.

Considering that our current focus is to test the kprobe instrumentation
layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
instead, which should take care of removing crashes introduced by ftrace
from our fuzzing results.

Thanks,

Mathieu


> 
> -- Steve
> 
> 
>> 
>> I can easily test any patch regarding this issue.
>> 
>> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
>> [2] https://lkml.org/lkml/2017/7/14/568
>> 
>> Thank you,
>> 
>> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
>> > On Fri, 14 Jul 2017 10:58:35 -0400
>> > Francis Deslauriers  wrote:
>> >  
>> >> This function is called when a kprobe is hit. Thus it should be
>> >> blacklisted to prevent kprobe to be triggered by kprobes.
>> >>
>> >> Signed-off-by: Francis Deslauriers 
>> >> ---
>> >>  kernel/trace/ftrace.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> >> index b308be3..c473d9b 100644
>> >> --- a/kernel/trace/ftrace.c
>> >> +++ b/kernel/trace/ftrace.c
>> >> @@ -36,6 +36,7 @@
>> >>
>> >>  #include 
>> >>
>> >> +#include 
>> >>  #include 
>> >>  #include 
>> >>
>> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip,
>> >> unsigned long parent_ip,
>> >>   preempt_enable_notrace();
>> >>   trace_clear_recursion(bit);
>> >>  }
>> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
>> >
>> > Continuing from what I said in the other email, this is fixing a
>> > symptom and not the problem. The real fix will be much more involved. I
>> > have a good idea on how to accomplish it too.
>> >
>> > -- Steve
>> >
>> >  
>> >>
>> >>  /**
>> >>   * ftrace_ops_get_func - get the function a trampoline should call
>> >  
>> 
>> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 11:18:25 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> 
> I completely forgot about this issue until recently when I encountered it 
> again.
> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
> seems to be causing problems.
> 
> Placing kretprobes like in the following configuration crashes my
> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
> 
> config 1:
> echo "r:event_1 __fdget" >> kprobe_events
> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> 
> config 2:
> echo "r:event_1 __fdget_pos" >> kprobe_events
> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> 
> config 3:
> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> 
> config 4:
> echo 'r:event_1 sys_open' >> kprobe_events
> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> 
> Here is my kernel config [1]:
> 
> In a previous email [2], you mentioned that you would like to add the
> ftrace-related symbols to a section to un-blacklist them all at once
> on demand but wanted to discuss it at Linux Plumbers. Do you still
> think that it's the right approach?

We probably didn't discuss it (as there was a lot to discuss, and this
was probably overshadowed by that). But yes, you should not probe
ftrace called functions. That is guaranteed to crash and that crash is
not a bug, but a feature.

The ftrace and ring buffer files should be blacklisted from being
probed. Perhaps the entire directory.

Anyway, I don't see this as much of an urgent matter, as it's one of
those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
cases. And there's a lot of urgent issues that currently need to be
dealt with.

-- Steve


> 
> I can easily test any patch regarding this issue.
> 
> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
> [2] https://lkml.org/lkml/2017/7/14/568
> 
> Thank you,
> 
> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> > On Fri, 14 Jul 2017 10:58:35 -0400
> > Francis Deslauriers  wrote:
> >  
> >> This function is called when a kprobe is hit. Thus it should be
> >> blacklisted to prevent kprobe to be triggered by kprobes.
> >>
> >> Signed-off-by: Francis Deslauriers 
> >> ---
> >>  kernel/trace/ftrace.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> index b308be3..c473d9b 100644
> >> --- a/kernel/trace/ftrace.c
> >> +++ b/kernel/trace/ftrace.c
> >> @@ -36,6 +36,7 @@
> >>
> >>  #include 
> >>
> >> +#include 
> >>  #include 
> >>  #include 
> >>
> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
> >> unsigned long parent_ip,
> >>   preempt_enable_notrace();
> >>   trace_clear_recursion(bit);
> >>  }
> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);  
> >
> > Continuing from what I said in the other email, this is fixing a
> > symptom and not the problem. The real fix will be much more involved. I
> > have a good idea on how to accomplish it too.
> >
> > -- Steve
> >
> >  
> >>
> >>  /**
> >>   * ftrace_ops_get_func - get the function a trampoline should call  
> >  
> 
> 
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 16 Mar 2018 11:18:25 -0400
Francis Deslauriers  wrote:

> Hi Steven,
> 
> I completely forgot about this issue until recently when I encountered it 
> again.
> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
> seems to be causing problems.
> 
> Placing kretprobes like in the following configuration crashes my
> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
> 
> config 1:
> echo "r:event_1 __fdget" >> kprobe_events
> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> 
> config 2:
> echo "r:event_1 __fdget_pos" >> kprobe_events
> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> 
> config 3:
> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> 
> config 4:
> echo 'r:event_1 sys_open' >> kprobe_events
> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> 
> Here is my kernel config [1]:
> 
> In a previous email [2], you mentioned that you would like to add the
> ftrace-related symbols to a section to un-blacklist them all at once
> on demand but wanted to discuss it at Linux Plumbers. Do you still
> think that it's the right approach?

We probably didn't discuss it (as there was a lot to discuss, and this
was probably overshadowed by that). But yes, you should not probe
ftrace called functions. That is guaranteed to crash and that crash is
not a bug, but a feature.

The ftrace and ring buffer files should be blacklisted from being
probed. Perhaps the entire directory.

Anyway, I don't see this as much of an urgent matter, as it's one of
those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
cases. And there's a lot of urgent issues that currently need to be
dealt with.

-- Steve


> 
> I can easily test any patch regarding this issue.
> 
> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
> [2] https://lkml.org/lkml/2017/7/14/568
> 
> Thank you,
> 
> 2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> > On Fri, 14 Jul 2017 10:58:35 -0400
> > Francis Deslauriers  wrote:
> >  
> >> This function is called when a kprobe is hit. Thus it should be
> >> blacklisted to prevent kprobe to be triggered by kprobes.
> >>
> >> Signed-off-by: Francis Deslauriers 
> >> ---
> >>  kernel/trace/ftrace.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> index b308be3..c473d9b 100644
> >> --- a/kernel/trace/ftrace.c
> >> +++ b/kernel/trace/ftrace.c
> >> @@ -36,6 +36,7 @@
> >>
> >>  #include 
> >>
> >> +#include 
> >>  #include 
> >>  #include 
> >>
> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
> >> unsigned long parent_ip,
> >>   preempt_enable_notrace();
> >>   trace_clear_recursion(bit);
> >>  }
> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);  
> >
> > Continuing from what I said in the other email, this is fixing a
> > symptom and not the problem. The real fix will be much more involved. I
> > have a good idea on how to accomplish it too.
> >
> > -- Steve
> >
> >  
> >>
> >>  /**
> >>   * ftrace_ops_get_func - get the function a trampoline should call  
> >  
> 
> 
> 

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Hi Steven,

I completely forgot about this issue until recently when I encountered it again.
Instrumenting the ftrace_ops_assist_func symbol and some other symbol
seems to be causing problems.

Placing kretprobes like in the following configuration crashes my
kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:

config 1:
echo "r:event_1 __fdget" >> kprobe_events
echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events

config 2:
echo "r:event_1 __fdget_pos" >> kprobe_events
echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events

config 3:
echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events

config 4:
echo 'r:event_1 sys_open' >> kprobe_events
echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events

Here is my kernel config [1]:

In a previous email [2], you mentioned that you would like to add the
ftrace-related symbols to a section to un-blacklist them all at once
on demand but wanted to discuss it at Linux Plumbers. Do you still
think that it's the right approach?

I can easily test any patch regarding this issue.

[1] http://paste.ubuntu.com/p/BJWvgMnW8z/
[2] https://lkml.org/lkml/2017/7/14/568

Thank you,

2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> On Fri, 14 Jul 2017 10:58:35 -0400
> Francis Deslauriers  wrote:
>
>> This function is called when a kprobe is hit. Thus it should be
>> blacklisted to prevent kprobe to be triggered by kprobes.
>>
>> Signed-off-by: Francis Deslauriers 
>> ---
>>  kernel/trace/ftrace.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index b308be3..c473d9b 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -36,6 +36,7 @@
>>
>>  #include 
>>
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
>> unsigned long parent_ip,
>>   preempt_enable_notrace();
>>   trace_clear_recursion(bit);
>>  }
>> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
>
> Continuing from what I said in the other email, this is fixing a
> symptom and not the problem. The real fix will be much more involved. I
> have a good idea on how to accomplish it too.
>
> -- Steve
>
>
>>
>>  /**
>>   * ftrace_ops_get_func - get the function a trampoline should call
>



-- 
Francis Deslauriers
Software developer
EfficiOS inc.

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

Hi Steven,

I completely forgot about this issue until recently when I encountered it again.
Instrumenting the ftrace_ops_assist_func symbol and some other symbol
seems to be causing problems.

Placing kretprobes like in the following configuration crashes my
kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:

config 1:
echo "r:event_1 __fdget" >> kprobe_events
echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events

config 2:
echo "r:event_1 __fdget_pos" >> kprobe_events
echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events

config 3:
echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events

config 4:
echo 'r:event_1 sys_open' >> kprobe_events
echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events

Here is my kernel config [1]:

In a previous email [2], you mentioned that you would like to add the
ftrace-related symbols to a section to un-blacklist them all at once
on demand but wanted to discuss it at Linux Plumbers. Do you still
think that it's the right approach?

I can easily test any patch regarding this issue.

[1] http://paste.ubuntu.com/p/BJWvgMnW8z/
[2] https://lkml.org/lkml/2017/7/14/568

Thank you,

2017-07-14 14:29 GMT-04:00 Steven Rostedt :
> On Fri, 14 Jul 2017 10:58:35 -0400
> Francis Deslauriers  wrote:
>
>> This function is called when a kprobe is hit. Thus it should be
>> blacklisted to prevent kprobe to be triggered by kprobes.
>>
>> Signed-off-by: Francis Deslauriers 
>> ---
>>  kernel/trace/ftrace.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index b308be3..c473d9b 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -36,6 +36,7 @@
>>
>>  #include 
>>
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
>> unsigned long parent_ip,
>>   preempt_enable_notrace();
>>   trace_clear_recursion(bit);
>>  }
>> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
>
> Continuing from what I said in the other email, this is fixing a
> symptom and not the problem. The real fix will be much more involved. I
> have a good idea on how to accomplish it too.
>
> -- Steve
>
>
>>
>>  /**
>>   * ftrace_ops_get_func - get the function a trampoline should call
>



-- 
Francis Deslauriers
Software developer
EfficiOS inc.

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 14 Jul 2017 10:58:35 -0400
Francis Deslauriers  wrote:

> This function is called when a kprobe is hit. Thus it should be
> blacklisted to prevent kprobe to be triggered by kprobes.
> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/ftrace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b308be3..c473d9b 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -36,6 +36,7 @@
>  
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
> unsigned long parent_ip,
>   preempt_enable_notrace();
>   trace_clear_recursion(bit);
>  }
> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);

Continuing from what I said in the other email, this is fixing a
symptom and not the problem. The real fix will be much more involved. I
have a good idea on how to accomplish it too.

-- Steve


>  
>  /**
>   * ftrace_ops_get_func - get the function a trampoline should call

Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Steven Rostedt]

On Fri, 14 Jul 2017 10:58:35 -0400
Francis Deslauriers  wrote:

> This function is called when a kprobe is hit. Thus it should be
> blacklisted to prevent kprobe to be triggered by kprobes.
> 
> Signed-off-by: Francis Deslauriers 
> ---
>  kernel/trace/ftrace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b308be3..c473d9b 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -36,6 +36,7 @@
>  
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
> unsigned long parent_ip,
>   preempt_enable_notrace();
>   trace_clear_recursion(bit);
>  }
> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);

Continuing from what I said in the other email, this is fixing a
symptom and not the problem. The real fix will be much more involved. I
have a good idea on how to accomplish it too.

-- Steve


>  
>  /**
>   * ftrace_ops_get_func - get the function a trampoline should call

[PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

This function is called when a kprobe is hit. Thus it should be
blacklisted to prevent kprobe to be triggered by kprobes.

Signed-off-by: Francis Deslauriers 
---
 kernel/trace/ftrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b308be3..c473d9b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -36,6 +36,7 @@
 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
unsigned long parent_ip,
preempt_enable_notrace();
trace_clear_recursion(bit);
 }
+NOKPROBE_SYMBOL(ftrace_ops_assist_func);
 
 /**
  * ftrace_ops_get_func - get the function a trampoline should call
-- 
2.7.4

[PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist

[Francis Deslauriers]

This function is called when a kprobe is hit. Thus it should be
blacklisted to prevent kprobe to be triggered by kprobes.

Signed-off-by: Francis Deslauriers 
---
 kernel/trace/ftrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b308be3..c473d9b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -36,6 +36,7 @@
 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, 
unsigned long parent_ip,
preempt_enable_notrace();
trace_clear_recursion(bit);
 }
+NOKPROBE_SYMBOL(ftrace_ops_assist_func);
 
 /**
  * ftrace_ops_get_func - get the function a trampoline should call
-- 
2.7.4