* [PATCH] ftrace: Allow inline functions not inlined to be traced
@ 2023-05-02 20:41 Steven Rostedt
2023-05-02 22:25 ` Song Liu
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-05-02 20:41 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Andrew Morton, Thomas Gleixner, Peter Zijlstra,
Mark Rutland, Song Liu, live-patching, Kees Cook, Miguel Ojeda,
Nick Desaulniers, Ingo Molnar
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Over 10 years ago there were many bugs that caused function tracing to
crash because some inlined function was not inlined and should not have
been traced. This made it hard to debug because when the developer tried
to reproduce it, if their compiler still inlined the function, the bug
would not trigger. The solution back then was simply to add "notrace" to
"inline" which would make sure all functions that are marked inline are
never traced even when the compiler decides to not inline them.
A lot has changed over the last 10 years.
1) ftrace_test_recursion_trylock() is now used by all ftrace hooks which
will prevent the recursive crashes from happening that was caused by
inlined functions being traced.
2) noinstr is now used to mark pretty much all functions that would also
cause problems if they are traced.
Today, it is no longer a problem if an inlined function is not inlined and
is traced. Removing notrace from inline has been requested several times
over the years. I believe it is now safe to do so.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
include/linux/compiler_types.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 547ea1ff806e..c8f23ba1c339 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -182,9 +182,8 @@ struct ftrace_likely_data {
* externally visible function. This makes extern inline behave as per gnu89
* semantics rather than c99. This prevents multiple symbol definition errors
* of extern inline functions at link time.
- * A lot of inline functions can cause havoc with function tracing.
*/
-#define inline inline __gnu_inline __inline_maybe_unused notrace
+#define inline inline __gnu_inline __inline_maybe_unused
/*
* gcc provides both __inline__ and __inline as alternate spellings of
@@ -230,7 +229,7 @@ struct ftrace_likely_data {
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
* '__maybe_unused' allows us to avoid defined-but-not-used warnings.
*/
-# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
+# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
# define __no_sanitize_or_inline __no_kasan_or_inline
#else
# define __no_kasan_or_inline __always_inline
@@ -247,7 +246,7 @@ struct ftrace_likely_data {
* disable all instrumentation. See Kconfig.kcsan where this is mandatory.
*/
# define __no_kcsan __no_sanitize_thread __disable_sanitizer_instrumentation
-# define __no_sanitize_or_inline __no_kcsan notrace __maybe_unused
+# define __no_sanitize_or_inline __no_kcsan __maybe_unused
#else
# define __no_kcsan
#endif
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ftrace: Allow inline functions not inlined to be traced
2023-05-02 20:41 [PATCH] ftrace: Allow inline functions not inlined to be traced Steven Rostedt
@ 2023-05-02 22:25 ` Song Liu
2023-05-31 21:50 ` Josh Poimboeuf
2023-06-08 9:50 ` Mark Rutland
2 siblings, 0 replies; 5+ messages in thread
From: Song Liu @ 2023-05-02 22:25 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Andrew Morton,
Thomas Gleixner, Peter Zijlstra, Mark Rutland, live-patching,
Kees Cook, Miguel Ojeda, Nick Desaulniers, Ingo Molnar
On Tue, May 2, 2023 at 1:41 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Over 10 years ago there were many bugs that caused function tracing to
> crash because some inlined function was not inlined and should not have
> been traced. This made it hard to debug because when the developer tried
> to reproduce it, if their compiler still inlined the function, the bug
> would not trigger. The solution back then was simply to add "notrace" to
> "inline" which would make sure all functions that are marked inline are
> never traced even when the compiler decides to not inline them.
>
> A lot has changed over the last 10 years.
>
> 1) ftrace_test_recursion_trylock() is now used by all ftrace hooks which
> will prevent the recursive crashes from happening that was caused by
> inlined functions being traced.
>
> 2) noinstr is now used to mark pretty much all functions that would also
> cause problems if they are traced.
>
> Today, it is no longer a problem if an inlined function is not inlined and
> is traced. Removing notrace from inline has been requested several times
> over the years. I believe it is now safe to do so.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Acked-by: Song Liu <song@kernel.org>
Thanks!
Song
> ---
> include/linux/compiler_types.h | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 547ea1ff806e..c8f23ba1c339 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -182,9 +182,8 @@ struct ftrace_likely_data {
> * externally visible function. This makes extern inline behave as per gnu89
> * semantics rather than c99. This prevents multiple symbol definition errors
> * of extern inline functions at link time.
> - * A lot of inline functions can cause havoc with function tracing.
> */
> -#define inline inline __gnu_inline __inline_maybe_unused notrace
> +#define inline inline __gnu_inline __inline_maybe_unused
>
> /*
> * gcc provides both __inline__ and __inline as alternate spellings of
> @@ -230,7 +229,7 @@ struct ftrace_likely_data {
> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
> */
> -# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
> +# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
> # define __no_sanitize_or_inline __no_kasan_or_inline
> #else
> # define __no_kasan_or_inline __always_inline
> @@ -247,7 +246,7 @@ struct ftrace_likely_data {
> * disable all instrumentation. See Kconfig.kcsan where this is mandatory.
> */
> # define __no_kcsan __no_sanitize_thread __disable_sanitizer_instrumentation
> -# define __no_sanitize_or_inline __no_kcsan notrace __maybe_unused
> +# define __no_sanitize_or_inline __no_kcsan __maybe_unused
> #else
> # define __no_kcsan
> #endif
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ftrace: Allow inline functions not inlined to be traced
2023-05-02 20:41 [PATCH] ftrace: Allow inline functions not inlined to be traced Steven Rostedt
2023-05-02 22:25 ` Song Liu
@ 2023-05-31 21:50 ` Josh Poimboeuf
2023-06-08 9:50 ` Mark Rutland
2 siblings, 0 replies; 5+ messages in thread
From: Josh Poimboeuf @ 2023-05-31 21:50 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Andrew Morton,
Thomas Gleixner, Peter Zijlstra, Mark Rutland, Song Liu,
live-patching, Kees Cook, Miguel Ojeda, Nick Desaulniers,
Ingo Molnar
On Tue, May 02, 2023 at 04:41:02PM -0400, Steven Rostedt wrote:
> Today, it is no longer a problem if an inlined function is not inlined and
> is traced. Removing notrace from inline has been requested several times
> over the years. I believe it is now safe to do so.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> include/linux/compiler_types.h | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 547ea1ff806e..c8f23ba1c339 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -182,9 +182,8 @@ struct ftrace_likely_data {
> * externally visible function. This makes extern inline behave as per gnu89
> * semantics rather than c99. This prevents multiple symbol definition errors
> * of extern inline functions at link time.
> - * A lot of inline functions can cause havoc with function tracing.
> */
> -#define inline inline __gnu_inline __inline_maybe_unused notrace
> +#define inline inline __gnu_inline __inline_maybe_unused
Yes!!! I've been wanting to do this for many years. This will help
live patching a lot.
Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>
--
Josh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ftrace: Allow inline functions not inlined to be traced
2023-05-02 20:41 [PATCH] ftrace: Allow inline functions not inlined to be traced Steven Rostedt
2023-05-02 22:25 ` Song Liu
2023-05-31 21:50 ` Josh Poimboeuf
@ 2023-06-08 9:50 ` Mark Rutland
2023-06-09 21:29 ` Steven Rostedt
2 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2023-06-08 9:50 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Andrew Morton,
Thomas Gleixner, Peter Zijlstra, Song Liu, live-patching,
Kees Cook, Miguel Ojeda, Nick Desaulniers, Ingo Molnar
On Tue, May 02, 2023 at 04:41:02PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Over 10 years ago there were many bugs that caused function tracing to
> crash because some inlined function was not inlined and should not have
> been traced. This made it hard to debug because when the developer tried
> to reproduce it, if their compiler still inlined the function, the bug
> would not trigger. The solution back then was simply to add "notrace" to
> "inline" which would make sure all functions that are marked inline are
> never traced even when the compiler decides to not inline them.
>
> A lot has changed over the last 10 years.
>
> 1) ftrace_test_recursion_trylock() is now used by all ftrace hooks which
> will prevent the recursive crashes from happening that was caused by
> inlined functions being traced.
>
> 2) noinstr is now used to mark pretty much all functions that would also
> cause problems if they are traced.
>
> Today, it is no longer a problem if an inlined function is not inlined and
> is traced. Removing notrace from inline has been requested several times
> over the years. I believe it is now safe to do so.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Unfortunately, from a quick build-test of arm64 there are a bunch of places
that are currently inline that need to be __always_inline for this to be safe.
Notably we have a few low-level helpers like is_kernel_in_hyp_mode() that are
only inlines, and those get used in the bowels of our entry code before we've
restored some HW state (e.g. in arch_nmi_enter()).
I'm happy to go audit and fixup arm64, but that will take some work.
Maybe it's worth having something like:
#ifdef ARCH_CAN_TRACE_INLINE
#define __notrace_inline
#else
#define __notrace_inline notrace
#endif
... so that we can opt-in where this is safe, (e.g. on x86)?
Thanks,
Mark.
> ---
> include/linux/compiler_types.h | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 547ea1ff806e..c8f23ba1c339 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -182,9 +182,8 @@ struct ftrace_likely_data {
> * externally visible function. This makes extern inline behave as per gnu89
> * semantics rather than c99. This prevents multiple symbol definition errors
> * of extern inline functions at link time.
> - * A lot of inline functions can cause havoc with function tracing.
> */
> -#define inline inline __gnu_inline __inline_maybe_unused notrace
> +#define inline inline __gnu_inline __inline_maybe_unused
>
> /*
> * gcc provides both __inline__ and __inline as alternate spellings of
> @@ -230,7 +229,7 @@ struct ftrace_likely_data {
> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
> */
> -# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
> +# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
> # define __no_sanitize_or_inline __no_kasan_or_inline
> #else
> # define __no_kasan_or_inline __always_inline
> @@ -247,7 +246,7 @@ struct ftrace_likely_data {
> * disable all instrumentation. See Kconfig.kcsan where this is mandatory.
> */
> # define __no_kcsan __no_sanitize_thread __disable_sanitizer_instrumentation
> -# define __no_sanitize_or_inline __no_kcsan notrace __maybe_unused
> +# define __no_sanitize_or_inline __no_kcsan __maybe_unused
> #else
> # define __no_kcsan
> #endif
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ftrace: Allow inline functions not inlined to be traced
2023-06-08 9:50 ` Mark Rutland
@ 2023-06-09 21:29 ` Steven Rostedt
0 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-06-09 21:29 UTC (permalink / raw)
To: Mark Rutland
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Andrew Morton,
Thomas Gleixner, Peter Zijlstra, Song Liu, live-patching,
Kees Cook, Miguel Ojeda, Nick Desaulniers, Ingo Molnar
On Thu, 8 Jun 2023 10:50:51 +0100
Mark Rutland <mark.rutland@arm.com> wrote:
> Unfortunately, from a quick build-test of arm64 there are a bunch of places
> that are currently inline that need to be __always_inline for this to be safe.
> Notably we have a few low-level helpers like is_kernel_in_hyp_mode() that are
> only inlines, and those get used in the bowels of our entry code before we've
> restored some HW state (e.g. in arch_nmi_enter()).
Sounds like you also need to add noinstr ;-)
>
> I'm happy to go audit and fixup arm64, but that will take some work.
>
> Maybe it's worth having something like:
>
> #ifdef ARCH_CAN_TRACE_INLINE
> #define __notrace_inline
> #else
> #define __notrace_inline notrace
> #endif
>
> ... so that we can opt-in where this is safe, (e.g. on x86)?
I guess I can do that.
-- Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-09 21:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-02 20:41 [PATCH] ftrace: Allow inline functions not inlined to be traced Steven Rostedt
2023-05-02 22:25 ` Song Liu
2023-05-31 21:50 ` Josh Poimboeuf
2023-06-08 9:50 ` Mark Rutland
2023-06-09 21:29 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).