[v2] ftrace: Allow inline functions not inlined to be traced

Commit Message

Steven Rostedt June 9, 2023, 9:44 p.m. UTC

From: "Steven Rostedt (Google)" <[e-mail]>

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, at least on x86. Removing notrace from inline has been requested
several times over the years. I believe it is now safe to do so.

Currently only x86 uses this.

Acked-by: Song Liu <[e-mail]>
Acked-by: Josh Poimboeuf <[e-mail]>
Signed-off-by: Steven Rostedt (Google) <[e-mail]>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/[e-mail]

 - have it opted in by architecture. Currently only x86 adds it. (Mark Rutland)

 arch/x86/Kconfig               |  1 +
 include/linux/compiler_types.h | 16 +++++++++++++---
 kernel/trace/Kconfig           |  7 +++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

Comments

Mark Rutland June 12, 2023, 9:56 a.m. UTC | #1

On Fri, Jun 09, 2023 at 05:44:22PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[e-mail]>
> 
> 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, at least on x86. Removing notrace from inline has been requested
> several times over the years. I believe it is now safe to do so.
> 
> Currently only x86 uses this.
> 
> Acked-by: Song Liu <[e-mail]>
> Acked-by: Josh Poimboeuf <[e-mail]>
> Signed-off-by: Steven Rostedt (Google) <[e-mail]>

Acked-by: Mark Rutland <[e-mail]>

> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/[e-mail]
> 
>  - have it opted in by architecture. Currently only x86 adds it. (Mark Rutland)

I'll add auditing/fixing arm64 on my queue of things to do; thanks for adding
the config option in the mean time!

Mark.

> 
>  arch/x86/Kconfig               |  1 +
>  include/linux/compiler_types.h | 16 +++++++++++++---
>  kernel/trace/Kconfig           |  7 +++++++
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index da5c081d64a5..1ddebf832534 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -61,6 +61,7 @@ config X86
>  	select ACPI_LEGACY_TABLES_LOOKUP	if ACPI
>  	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
>  	select ARCH_32BIT_OFF_T			if X86_32
> +	select ARCH_CAN_TRACE_INLINE
>  	select ARCH_CLOCKSOURCE_INIT
>  	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
>  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 547ea1ff806e..f827e2a98500 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -169,6 +169,16 @@ struct ftrace_likely_data {
>  #define notrace			__attribute__((__no_instrument_function__))
>  #endif
>  
> +/*
> + * If all inline code not marked as __always_inline is safe to trace,
> + * then allow the architecture to do so.
> + */
> +#ifdef CONFIG_ARCH_CAN_TRACE_INLINE
> +#define __notrace_inline
> +#else
> +#define __notrace_inline	notrace
> +#endif
> +
>  /*
>   * it doesn't make sense on ARM (currently the only user of __naked)
>   * to trace naked functions because then mcount is called without
> @@ -184,7 +194,7 @@ struct ftrace_likely_data {
>   * 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 __notrace_inline
>  
>  /*
>   * gcc provides both __inline__ and __inline as alternate spellings of
> @@ -230,7 +240,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 __notrace_inline __maybe_unused
>  # define __no_sanitize_or_inline __no_kasan_or_inline
>  #else
>  # define __no_kasan_or_inline __always_inline
> @@ -247,7 +257,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 __notrace_inline __maybe_unused
>  #else
>  # define __no_kcsan
>  #endif
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index abe5c583bd59..b66ab0e6ce19 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -106,6 +106,13 @@ config HAVE_BUILDTIME_MCOUNT_SORT
>           An architecture selects this if it sorts the mcount_loc section
>  	 at build time.
>  
> +config ARCH_CAN_TRACE_INLINE
> +       bool
> +       help
> +         It is safe for an architecture to trace any function marked
> +	 as inline (not __always_inline) that the compiler decides to
> +	 not inline.
> +
>  config BUILDTIME_MCOUNT_SORT
>         bool
>         default y
> -- 
> 2.39.2
>

Masami Hiramatsu (Google) June 12, 2023, 1:54 p.m. UTC | #2

On Fri, 9 Jun 2023 17:44:22 -0400
Steven Rostedt <[e-mail]> wrote:

> From: "Steven Rostedt (Google)" <[e-mail]>
> 
> 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, at least on x86. Removing notrace from inline has been requested
> several times over the years. I believe it is now safe to do so.
> 
> Currently only x86 uses this.
> 
> Acked-by: Song Liu <[e-mail]>
> Acked-by: Josh Poimboeuf <[e-mail]>
> Signed-off-by: Steven Rostedt (Google) <[e-mail]>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/[e-mail]
> 
>  - have it opted in by architecture. Currently only x86 adds it. (Mark Rutland)
> 
>  arch/x86/Kconfig               |  1 +
>  include/linux/compiler_types.h | 16 +++++++++++++---
>  kernel/trace/Kconfig           |  7 +++++++
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index da5c081d64a5..1ddebf832534 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -61,6 +61,7 @@ config X86
>  	select ACPI_LEGACY_TABLES_LOOKUP	if ACPI
>  	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
>  	select ARCH_32BIT_OFF_T			if X86_32
> +	select ARCH_CAN_TRACE_INLINE
>  	select ARCH_CLOCKSOURCE_INIT
>  	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
>  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 547ea1ff806e..f827e2a98500 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -169,6 +169,16 @@ struct ftrace_likely_data {
>  #define notrace			__attribute__((__no_instrument_function__))
>  #endif
>  
> +/*
> + * If all inline code not marked as __always_inline is safe to trace,
> + * then allow the architecture to do so.
> + */
> +#ifdef CONFIG_ARCH_CAN_TRACE_INLINE
> +#define __notrace_inline
> +#else
> +#define __notrace_inline	notrace
> +#endif

I think I understand what the purpose of this patch. But I'm confusing the
above change, it looks like a literal contradiction :)
I mean, __notrace_inline functions shouldn't be notrace?

What about call it __may_notrace_inline or __maybe_notrace?

Others looks good to me.

Thank you,

> +
>  /*
>   * it doesn't make sense on ARM (currently the only user of __naked)
>   * to trace naked functions because then mcount is called without
> @@ -184,7 +194,7 @@ struct ftrace_likely_data {
>   * 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 __notrace_inline
>  
>  /*
>   * gcc provides both __inline__ and __inline as alternate spellings of
> @@ -230,7 +240,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 __notrace_inline __maybe_unused
>  # define __no_sanitize_or_inline __no_kasan_or_inline
>  #else
>  # define __no_kasan_or_inline __always_inline
> @@ -247,7 +257,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 __notrace_inline __maybe_unused
>  #else
>  # define __no_kcsan
>  #endif
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index abe5c583bd59..b66ab0e6ce19 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -106,6 +106,13 @@ config HAVE_BUILDTIME_MCOUNT_SORT
>           An architecture selects this if it sorts the mcount_loc section
>  	 at build time.
>  
> +config ARCH_CAN_TRACE_INLINE
> +       bool
> +       help
> +         It is safe for an architecture to trace any function marked
> +	 as inline (not __always_inline) that the compiler decides to
> +	 not inline.
> +
>  config BUILDTIME_MCOUNT_SORT
>         bool
>         default y
> -- 
> 2.39.2
>

Thomas Gleixner June 12, 2023, 3:09 p.m. UTC | #3

On Fri, Jun 09 2023 at 17:44, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[e-mail]>
>
> 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, at least on x86. Removing notrace from inline has been requested
> several times over the years. I believe it is now safe to do so.
>
> Currently only x86 uses this.

I assume this passes the objtool noinstr validation. If so, if would be
helpful to document that.
>  /*
>   * gcc provides both __inline__ and __inline as alternate spellings of
> @@ -230,7 +240,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 __notrace_inline __maybe_unused

I'm not convinced that this is correct

>  # define __no_sanitize_or_inline __no_kasan_or_inline
>  #else

given that the !__SANITIZE_ADDRESS__ variant is:

>  # define __no_kasan_or_inline __always_inline

which cannot be traced.

> @@ -247,7 +257,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 __notrace_inline  __maybe_unused

Ditto. 

>  #else
>  # define __no_kcsan
>  #endif
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index abe5c583bd59..b66ab0e6ce19 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -106,6 +106,13 @@ config HAVE_BUILDTIME_MCOUNT_SORT
>           An architecture selects this if it sorts the mcount_loc section
>  	 at build time.
>  
> +config ARCH_CAN_TRACE_INLINE
> +       bool
> +       help
> +         It is safe for an architecture to trace any function marked

Spaces instead of tab.

> +	 as inline (not __always_inline) that the compiler decides to

and this one has a tab.

> +	 not inline.
> +
>  config BUILDTIME_MCOUNT_SORT
>         bool
>         default y

Steven Rostedt June 13, 2023, 3:40 p.m. UTC | #4

On Mon, 12 Jun 2023 17:09:31 +0200
Thomas Gleixner <[e-mail]> wrote:

> >
> > Currently only x86 uses this.  
> 
> I assume this passes the objtool noinstr validation. If so, if would be
> helpful to document that.

I haven't run this through the full test suite. But I will check.

> >  /*
> >   * gcc provides both __inline__ and __inline as alternate spellings of
> > @@ -230,7 +240,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 __notrace_inline __maybe_unused  
> 
> I'm not convinced that this is correct
> 
> >  # define __no_sanitize_or_inline __no_kasan_or_inline
> >  #else  
> 
> given that the !__SANITIZE_ADDRESS__ variant is:
> 
> >  # define __no_kasan_or_inline __always_inline  
> 
> which cannot be traced.
> 
> > @@ -247,7 +257,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 __notrace_inline  __maybe_unused  
> 
> Ditto. 

I'll just keep the notrace on these.

> 
> >  #else
> >  # define __no_kcsan
> >  #endif
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index abe5c583bd59..b66ab0e6ce19 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -106,6 +106,13 @@ config HAVE_BUILDTIME_MCOUNT_SORT
> >           An architecture selects this if it sorts the mcount_loc section
> >  	 at build time.
> >  
> > +config ARCH_CAN_TRACE_INLINE
> > +       bool
> > +       help
> > +         It is safe for an architecture to trace any function marked  
> 
> Spaces instead of tab.

Bah, I noticed that my emacs is doing this on other configs I just added.
It adds spaces for the first entry, then tabs for the rest.

> 
> > +	 as inline (not __always_inline) that the compiler decides to  
> 
> and this one has a tab.
> 
> > +	 not inline.
> > +
> >  config BUILDTIME_MCOUNT_SORT
> >         bool
> >         default y  

Thanks for the review!

-- Steve

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index da5c081d64a5..1ddebf832534 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -61,6 +61,7 @@  config X86
 	select ACPI_LEGACY_TABLES_LOOKUP	if ACPI
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
 	select ARCH_32BIT_OFF_T			if X86_32
+	select ARCH_CAN_TRACE_INLINE
 	select ARCH_CLOCKSOURCE_INIT
 	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 547ea1ff806e..f827e2a98500 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -169,6 +169,16 @@  struct ftrace_likely_data {
 #define notrace			__attribute__((__no_instrument_function__))
 #endif
 
+/*
+ * If all inline code not marked as __always_inline is safe to trace,
+ * then allow the architecture to do so.
+ */
+#ifdef CONFIG_ARCH_CAN_TRACE_INLINE
+#define __notrace_inline
+#else
+#define __notrace_inline	notrace
+#endif
+
 /*
  * it doesn't make sense on ARM (currently the only user of __naked)
  * to trace naked functions because then mcount is called without
@@ -184,7 +194,7 @@  struct ftrace_likely_data {
  * 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 __notrace_inline
 
 /*
  * gcc provides both __inline__ and __inline as alternate spellings of
@@ -230,7 +240,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 __notrace_inline __maybe_unused
 # define __no_sanitize_or_inline __no_kasan_or_inline
 #else
 # define __no_kasan_or_inline __always_inline
@@ -247,7 +257,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 __notrace_inline __maybe_unused
 #else
 # define __no_kcsan
 #endif
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index abe5c583bd59..b66ab0e6ce19 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -106,6 +106,13 @@  config HAVE_BUILDTIME_MCOUNT_SORT
          An architecture selects this if it sorts the mcount_loc section
 	 at build time.
 
+config ARCH_CAN_TRACE_INLINE
+       bool
+       help
+         It is safe for an architecture to trace any function marked
+	 as inline (not __always_inline) that the compiler decides to
+	 not inline.
+
 config BUILDTIME_MCOUNT_SORT
        bool
        default y