[PATCHv2 bpf-next 0/2] bpf,ftrace: bpf dispatcher function fix

[PATCHv2 bpf-next 0/2] bpf,ftrace: bpf dispatcher function fix

[Jiri Olsa]

hi,
as discussed [1] sending fix that moves bpf dispatcher function of out
ftrace locations together with Peter's HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
dependency change.

v2 changes:
  - fixing s390x CI build failure by enabling attributes
    only for x86_64 [Ilya Leoshkevic]

thanks,
jirka


[1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
---
Jiri Olsa (1):
      bpf: Move bpf_dispatcher function out of ftrace locations

Peter Zijlstra (Intel) (1):
      ftrace: Add HAVE_DYNAMIC_FTRACE_NO_PATCHABLE

 arch/x86/Kconfig                  |  1 +
 include/asm-generic/vmlinux.lds.h | 11 ++++++++++-
 include/linux/bpf.h               |  8 ++++++++
 kernel/trace/Kconfig              |  6 ++++++
 tools/objtool/check.c             |  3 ++-
 5 files changed, 27 insertions(+), 2 deletions(-)

[PATCHv2 bpf-next 1/2] ftrace: Add HAVE_DYNAMIC_FTRACE_NO_PATCHABLE

[Jiri Olsa]

From: "Peter Zijlstra (Intel)" <peterz@infradead.org>

x86 will shortly start using -fpatchable-function-entry for purposes
other than ftrace, make sure the __patchable_function_entry section
isn't merged in the mcount_loc section.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/asm-generic/vmlinux.lds.h | 11 ++++++++++-
 kernel/trace/Kconfig              |  6 ++++++
 tools/objtool/check.c             |  3 ++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 7515a465ec03..13b197ef0d63 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -154,6 +154,14 @@
 #define MEM_DISCARD(sec) *(.mem##sec)
 #endif
 
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
+#define KEEP_PATCHABLE		KEEP(*(__patchable_function_entries))
+#define PATCHABLE_DISCARDS
+#else
+#define KEEP_PATCHABLE
+#define PATCHABLE_DISCARDS	*(__patchable_function_entries)
+#endif
+
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 /*
  * The ftrace call sites are logged to a section whose name depends on the
@@ -172,7 +180,7 @@
 #define MCOUNT_REC()	. = ALIGN(8);				\
 			__start_mcount_loc = .;			\
 			KEEP(*(__mcount_loc))			\
-			KEEP(*(__patchable_function_entries))	\
+			KEEP_PATCHABLE				\
 			__stop_mcount_loc = .;			\
 			ftrace_stub_graph = ftrace_stub;	\
 			ftrace_ops_list_func = arch_ftrace_ops_list_func;
@@ -1024,6 +1032,7 @@
 
 #define COMMON_DISCARDS							\
 	SANITIZER_DISCARDS						\
+	PATCHABLE_DISCARDS						\
 	*(.discard)							\
 	*(.discard.*)							\
 	*(.modinfo)							\
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 1052126bdca2..e9e95c790b8e 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -51,6 +51,12 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	 This allows for use of regs_get_kernel_argument() and
 	 kernel_stack_pointer().
 
+config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
+	bool
+	help
+	  If the architecture generates __patchable_function_entries sections
+	  but does not want them included in the ftrace locations.
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec74da7ffe..f23e8d11f6d4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4110,7 +4110,8 @@ static int validate_ibt(struct objtool_file *file)
 		    !strcmp(sec->name, "__bug_table")			||
 		    !strcmp(sec->name, "__ex_table")			||
 		    !strcmp(sec->name, "__jump_table")			||
-		    !strcmp(sec->name, "__mcount_loc"))
+		    !strcmp(sec->name, "__mcount_loc")			||
+		    strstr(sec->name, "__patchable_function_entries"))
 			continue;
 
 		list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
-- 
2.37.2

[PATCHv2 bpf-next 2/2] bpf: Move bpf_dispatcher function out of ftrace locations

[Jiri Olsa]

The dispatcher function is attached/detached to trampoline by
dispatcher update function. At the same time it's available as
ftrace attachable function.

After discussion [1] the proposed solution is to use compiler
attributes to alter bpf_dispatcher_##name##_func function:

  - remove it from being instrumented with __no_instrument_function__
    attribute, so ftrace has no track of it

  - but still generate 5 nop instructions with patchable_function_entry(5)
    attribute, which are expected by bpf_arch_text_poke used by
    dispatcher update function

Enabling HAVE_DYNAMIC_FTRACE_NO_PATCHABLE option for x86, so
__patchable_function_entries functions are not part of ftrace/mcount
locations.

Adding attributes to bpf_dispatcher_XXX function on x86_64 so it's
kept out of ftrace locations and has 5 byte nop generated at entry.

These attributes need to be arch specific as pointer out by Ilya
Leoshkevic in here [2].

The dispatcher image is generated only for x86_64 arch, so the
code can stay as is for other archs.

[1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
[2] https://lore.kernel.org/bpf/969a14281a7791c334d476825863ee449964dd0c.camel@linux.ibm.com/
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/Kconfig    | 1 +
 include/linux/bpf.h | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..089c20cefd2b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -284,6 +284,7 @@ config X86
 	select PROC_PID_ARCH_STATUS		if PROC_FS
 	select HAVE_ARCH_NODE_DEV_GROUP		if X86_SGX
 	imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
+	select HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9c1674973e03..4ab4b0a1beb8 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -924,7 +924,15 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
 	},							\
 }
 
+#ifdef CONFIG_X86_64
+#define BPF_DISPATCHER_ATTRIBUTES __attribute__((__no_instrument_function__)) \
+				   __attribute__((patchable_function_entry(5)))
+#else
+#define BPF_DISPATCHER_ATTRIBUTES
+#endif
+
 #define DEFINE_BPF_DISPATCHER(name)					\
+	BPF_DISPATCHER_ATTRIBUTES					\
 	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
 		const void *ctx,					\
 		const struct bpf_insn *insnsi,				\
-- 
2.37.2

Re: [PATCHv2 bpf-next 2/2] bpf: Move bpf_dispatcher function out of ftrace locations

[Peter Zijlstra]

On Thu, Sep 01, 2022 at 03:41:50PM +0200, Jiri Olsa wrote:

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9c1674973e03..4ab4b0a1beb8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -924,7 +924,15 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
>  	},							\
>  }
>  
> +#ifdef CONFIG_X86_64
> +#define BPF_DISPATCHER_ATTRIBUTES __attribute__((__no_instrument_function__)) \
> +				   __attribute__((patchable_function_entry(5)))
> +#else
> +#define BPF_DISPATCHER_ATTRIBUTES
> +#endif
> +
>  #define DEFINE_BPF_DISPATCHER(name)					\
> +	BPF_DISPATCHER_ATTRIBUTES					\
>  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
>  		const void *ctx,					\
>  		const struct bpf_insn *insnsi,				\

Are you sure you want the notrace x86_64 only?

That is, perhaps something like this...

+#ifdef CONFIG_X86_64
+#define BPF_DISPATCHER_ATTRIBUTES	   __attribute__((patchable_function_entry(5)))
+#else
+#define BPF_DISPATCHER_ATTRIBUTES
+#endif
+
 #define DEFINE_BPF_DISPATCHER(name)					\
+	notrace BPF_DISPATCHER_ATTRIBUTES				\
 	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\

Re: [PATCHv2 bpf-next 2/2] bpf: Move bpf_dispatcher function out of ftrace locations

[Jiri Olsa]

On Fri, Sep 02, 2022 at 01:14:19PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 01, 2022 at 03:41:50PM +0200, Jiri Olsa wrote:
> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 9c1674973e03..4ab4b0a1beb8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -924,7 +924,15 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
> >  	},							\
> >  }
> >  
> > +#ifdef CONFIG_X86_64
> > +#define BPF_DISPATCHER_ATTRIBUTES __attribute__((__no_instrument_function__)) \
> > +				   __attribute__((patchable_function_entry(5)))
> > +#else
> > +#define BPF_DISPATCHER_ATTRIBUTES
> > +#endif
> > +
> >  #define DEFINE_BPF_DISPATCHER(name)					\
> > +	BPF_DISPATCHER_ATTRIBUTES					\
> >  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
> >  		const void *ctx,					\
> >  		const struct bpf_insn *insnsi,				\
> 
> Are you sure you want the notrace x86_64 only?
> 
> That is, perhaps something like this...
> 
> +#ifdef CONFIG_X86_64
> +#define BPF_DISPATCHER_ATTRIBUTES	   __attribute__((patchable_function_entry(5)))
> +#else
> +#define BPF_DISPATCHER_ATTRIBUTES
> +#endif
> +
>  #define DEFINE_BPF_DISPATCHER(name)					\
> +	notrace BPF_DISPATCHER_ATTRIBUTES				\
>  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
> 

that's also an option.. but I don't this it's big deal that the function
is traceable on other arches, because the dispatcher image is generated
only on x86, so no other arch is touching that function entry, so it's
safe for ftrace to attach

jirka

Re: [PATCHv2 bpf-next 2/2] bpf: Move bpf_dispatcher function out of ftrace locations

[Peter Zijlstra]

On Fri, Sep 02, 2022 at 06:53:38PM +0200, Jiri Olsa wrote:
> > Are you sure you want the notrace x86_64 only?
> > 
> > That is, perhaps something like this...
> > 
> > +#ifdef CONFIG_X86_64
> > +#define BPF_DISPATCHER_ATTRIBUTES	   __attribute__((patchable_function_entry(5)))
> > +#else
> > +#define BPF_DISPATCHER_ATTRIBUTES
> > +#endif
> > +
> >  #define DEFINE_BPF_DISPATCHER(name)					\
> > +	notrace BPF_DISPATCHER_ATTRIBUTES				\
> >  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
> > 
> 
> that's also an option.. but I don't this it's big deal that the function
> is traceable on other arches, because the dispatcher image is generated
> only on x86, so no other arch is touching that function entry, so it's
> safe for ftrace to attach

It just seems like a pointless difference.

From a code-gen POV you don't strictly need the notrace; without it
it'll generate:

bpf_dispatcher_name_func:
	nop
	nop
	nop
	nop
	nop
	call __fentry__
	RET

It'll just function 'weird', but it'll 'work'.

Re: [PATCHv2 bpf-next 2/2] bpf: Move bpf_dispatcher function out of ftrace locations

[Jiri Olsa]

On Fri, Sep 02, 2022 at 07:20:40PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 02, 2022 at 06:53:38PM +0200, Jiri Olsa wrote:
> > > Are you sure you want the notrace x86_64 only?
> > > 
> > > That is, perhaps something like this...
> > > 
> > > +#ifdef CONFIG_X86_64
> > > +#define BPF_DISPATCHER_ATTRIBUTES	   __attribute__((patchable_function_entry(5)))
> > > +#else
> > > +#define BPF_DISPATCHER_ATTRIBUTES
> > > +#endif
> > > +
> > >  #define DEFINE_BPF_DISPATCHER(name)					\
> > > +	notrace BPF_DISPATCHER_ATTRIBUTES				\
> > >  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
> > > 
> > 
> > that's also an option.. but I don't this it's big deal that the function
> > is traceable on other arches, because the dispatcher image is generated
> > only on x86, so no other arch is touching that function entry, so it's
> > safe for ftrace to attach
> 
> It just seems like a pointless difference.

ok, I'll use notrace as default

> 
> From a code-gen POV you don't strictly need the notrace; without it
> it'll generate:
> 
> bpf_dispatcher_name_func:
> 	nop
> 	nop
> 	nop
> 	nop
> 	nop
> 	call __fentry__
> 	RET
> 
> It'll just function 'weird', but it'll 'work'.

I'll use notrace for all archs as you suggests, so there's less
surprise in future if some other arch implements dispatcher image

thanks,
jirka