* [PATCH v4 1/2] ftrace: make extra rcu_is_watching() validation check optional
@ 2024-04-18 19:09 Andrii Nakryiko
2024-04-18 19:09 ` [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get() Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2024-04-18 19:09 UTC (permalink / raw)
To: linux-trace-kernel, rostedt, mhiramat
Cc: bpf, jolsa, Andrii Nakryiko, Paul E . McKenney
Introduce CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING config option to
control whether ftrace low-level code performs additional
rcu_is_watching()-based validation logic in an attempt to catch noinstr
violations.
This check is expected to never be true and is mostly useful for
low-level validation of ftrace subsystem invariants. For most users it
should probably be kept disabled to eliminate unnecessary runtime
overhead.
This improves BPF multi-kretprobe (relying on ftrace and rethook
infrastructure) runtime throughput by 2%, according to BPF benchmarks ([0]).
[0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul E. McKenney <paulmck@kernel.org>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/trace_recursion.h | 2 +-
kernel/trace/Kconfig | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
index d48cd92d2364..24ea8ac049b4 100644
--- a/include/linux/trace_recursion.h
+++ b/include/linux/trace_recursion.h
@@ -135,7 +135,7 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip);
# define do_ftrace_record_recursion(ip, pip) do { } while (0)
#endif
-#ifdef CONFIG_ARCH_WANTS_NO_INSTR
+#ifdef CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING
# define trace_warn_on_no_rcu(ip) \
({ \
bool __ret = !rcu_is_watching(); \
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 61c541c36596..7aebd1b8f93e 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -974,6 +974,19 @@ config FTRACE_RECORD_RECURSION_SIZE
This file can be reset, but the limit can not change in
size at runtime.
+config FTRACE_VALIDATE_RCU_IS_WATCHING
+ bool "Validate RCU is on during ftrace execution"
+ depends on FUNCTION_TRACER
+ depends on ARCH_WANTS_NO_INSTR
+ help
+ All callbacks that attach to the function tracing have some sort of
+ protection against recursion. This option is only to verify that
+ ftrace (and other users of ftrace_test_recursion_trylock()) are not
+ called outside of RCU, as if they are, it can cause a race. But it
+ also has a noticeable overhead when enabled.
+
+ If unsure, say N
+
config RING_BUFFER_RECORD_RECURSION
bool "Record functions that recurse in the ring buffer"
depends on FTRACE_RECORD_RECURSION
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()
2024-04-18 19:09 [PATCH v4 1/2] ftrace: make extra rcu_is_watching() validation check optional Andrii Nakryiko
@ 2024-04-18 19:09 ` Andrii Nakryiko
2024-04-18 22:53 ` Paul E. McKenney
2024-04-19 1:00 ` Masami Hiramatsu
0 siblings, 2 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2024-04-18 19:09 UTC (permalink / raw)
To: linux-trace-kernel, rostedt, mhiramat
Cc: bpf, jolsa, Andrii Nakryiko, Paul E . McKenney
Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
that RCU is watching when trying to setup rethooko on a function entry.
One notable exception when we force rcu_is_watching() check is
CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
old-style int3-based workflow instead of relying on ftrace, making RCU
watching check important to validate.
This further (in addition to improvements in the previous patch)
improves BPF multi-kretprobe (which rely on rethook) runtime throughput
by 2.3%, according to BPF benchmarks ([0]).
[0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/trace/rethook.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index fa03094e9e69..a974605ad7a5 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
if (unlikely(!handler))
return NULL;
+#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
/*
* This expects the caller will set up a rethook on a function entry.
* When the function returns, the rethook will eventually be reclaimed
@@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
*/
if (unlikely(!rcu_is_watching()))
return NULL;
+#endif
return (struct rethook_node *)objpool_pop(&rh->pool);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()
2024-04-18 19:09 ` [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get() Andrii Nakryiko
@ 2024-04-18 22:53 ` Paul E. McKenney
2024-04-19 1:00 ` Masami Hiramatsu
1 sibling, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2024-04-18 22:53 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: linux-trace-kernel, rostedt, mhiramat, bpf, jolsa
On Thu, Apr 18, 2024 at 12:09:09PM -0700, Andrii Nakryiko wrote:
> Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
> that RCU is watching when trying to setup rethooko on a function entry.
>
> One notable exception when we force rcu_is_watching() check is
> CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
> old-style int3-based workflow instead of relying on ftrace, making RCU
> watching check important to validate.
>
> This further (in addition to improvements in the previous patch)
> improves BPF multi-kretprobe (which rely on rethook) runtime throughput
> by 2.3%, according to BPF benchmarks ([0]).
>
> [0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
> ---
> kernel/trace/rethook.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index fa03094e9e69..a974605ad7a5 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> if (unlikely(!handler))
> return NULL;
>
> +#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
> /*
> * This expects the caller will set up a rethook on a function entry.
> * When the function returns, the rethook will eventually be reclaimed
> @@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> */
> if (unlikely(!rcu_is_watching()))
> return NULL;
> +#endif
>
> return (struct rethook_node *)objpool_pop(&rh->pool);
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()
2024-04-18 19:09 ` [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get() Andrii Nakryiko
2024-04-18 22:53 ` Paul E. McKenney
@ 2024-04-19 1:00 ` Masami Hiramatsu
2024-04-19 17:59 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2024-04-19 1:00 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: linux-trace-kernel, rostedt, bpf, jolsa, Paul E . McKenney
On Thu, 18 Apr 2024 12:09:09 -0700
Andrii Nakryiko <andrii@kernel.org> wrote:
> Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
> that RCU is watching when trying to setup rethooko on a function entry.
>
> One notable exception when we force rcu_is_watching() check is
> CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
> old-style int3-based workflow instead of relying on ftrace, making RCU
> watching check important to validate.
>
> This further (in addition to improvements in the previous patch)
> improves BPF multi-kretprobe (which rely on rethook) runtime throughput
> by 2.3%, according to BPF benchmarks ([0]).
>
> [0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Thanks for update! This looks good to me.
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thanks,
> ---
> kernel/trace/rethook.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index fa03094e9e69..a974605ad7a5 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> if (unlikely(!handler))
> return NULL;
>
> +#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
> /*
> * This expects the caller will set up a rethook on a function entry.
> * When the function returns, the rethook will eventually be reclaimed
> @@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> */
> if (unlikely(!rcu_is_watching()))
> return NULL;
> +#endif
>
> return (struct rethook_node *)objpool_pop(&rh->pool);
> }
> --
> 2.43.0
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()
2024-04-19 1:00 ` Masami Hiramatsu
@ 2024-04-19 17:59 ` Andrii Nakryiko
2024-04-21 2:40 ` Masami Hiramatsu
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2024-04-19 17:59 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Andrii Nakryiko, linux-trace-kernel, rostedt, bpf, jolsa,
Paul E . McKenney
On Thu, Apr 18, 2024 at 6:00 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Thu, 18 Apr 2024 12:09:09 -0700
> Andrii Nakryiko <andrii@kernel.org> wrote:
>
> > Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
> > that RCU is watching when trying to setup rethooko on a function entry.
> >
> > One notable exception when we force rcu_is_watching() check is
> > CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
> > old-style int3-based workflow instead of relying on ftrace, making RCU
> > watching check important to validate.
> >
> > This further (in addition to improvements in the previous patch)
> > improves BPF multi-kretprobe (which rely on rethook) runtime throughput
> > by 2.3%, according to BPF benchmarks ([0]).
> >
> > [0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
>
> Thanks for update! This looks good to me.
Thanks, Masami! Will you take it through your tree, or you'd like to
route it through bpf-next?
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Thanks,
>
> > ---
> > kernel/trace/rethook.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> > index fa03094e9e69..a974605ad7a5 100644
> > --- a/kernel/trace/rethook.c
> > +++ b/kernel/trace/rethook.c
> > @@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > if (unlikely(!handler))
> > return NULL;
> >
> > +#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
> > /*
> > * This expects the caller will set up a rethook on a function entry.
> > * When the function returns, the rethook will eventually be reclaimed
> > @@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > */
> > if (unlikely(!rcu_is_watching()))
> > return NULL;
> > +#endif
> >
> > return (struct rethook_node *)objpool_pop(&rh->pool);
> > }
> > --
> > 2.43.0
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()
2024-04-19 17:59 ` Andrii Nakryiko
@ 2024-04-21 2:40 ` Masami Hiramatsu
0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2024-04-21 2:40 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, linux-trace-kernel, rostedt, bpf, jolsa,
Paul E . McKenney
On Fri, 19 Apr 2024 10:59:09 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Thu, Apr 18, 2024 at 6:00 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Thu, 18 Apr 2024 12:09:09 -0700
> > Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > > Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
> > > that RCU is watching when trying to setup rethooko on a function entry.
> > >
> > > One notable exception when we force rcu_is_watching() check is
> > > CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
> > > old-style int3-based workflow instead of relying on ftrace, making RCU
> > > watching check important to validate.
> > >
> > > This further (in addition to improvements in the previous patch)
> > > improves BPF multi-kretprobe (which rely on rethook) runtime throughput
> > > by 2.3%, according to BPF benchmarks ([0]).
> > >
> > > [0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >
> >
> > Thanks for update! This looks good to me.
>
> Thanks, Masami! Will you take it through your tree, or you'd like to
> route it through bpf-next?
OK, let me take it through linux-trace tree.
Thank you!
>
> >
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Thanks,
> >
> > > ---
> > > kernel/trace/rethook.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> > > index fa03094e9e69..a974605ad7a5 100644
> > > --- a/kernel/trace/rethook.c
> > > +++ b/kernel/trace/rethook.c
> > > @@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > > if (unlikely(!handler))
> > > return NULL;
> > >
> > > +#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
> > > /*
> > > * This expects the caller will set up a rethook on a function entry.
> > > * When the function returns, the rethook will eventually be reclaimed
> > > @@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > > */
> > > if (unlikely(!rcu_is_watching()))
> > > return NULL;
> > > +#endif
> > >
> > > return (struct rethook_node *)objpool_pop(&rh->pool);
> > > }
> > > --
> > > 2.43.0
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-21 2:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-18 19:09 [PATCH v4 1/2] ftrace: make extra rcu_is_watching() validation check optional Andrii Nakryiko
2024-04-18 19:09 ` [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get() Andrii Nakryiko
2024-04-18 22:53 ` Paul E. McKenney
2024-04-19 1:00 ` Masami Hiramatsu
2024-04-19 17:59 ` Andrii Nakryiko
2024-04-21 2:40 ` Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.