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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Aug 31, 2022 at 01:46:09AM +0200, Ilya Leoshkevich wrote:
> On Tue, 2022-08-30 at 18:46 +0200, Ilya Leoshkevich wrote:
> > On Tue, 2022-08-30 at 15:48 +0200, Jiri Olsa wrote:
> > > On Tue, Aug 30, 2022 at 12:25:25AM +0200, Daniel Borkmann wrote:
> > > > On 8/26/22 8:46 PM, Jiri Olsa wrote:
> > > > > 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.
> > > > 
> > > > Looks like the series breaks s390x builds; BPF CI link:
> > > > 
> > > > https://github.com/kernel-patches/bpf/runs/8079411784?check_suite_focus=true
> > > > 
> > > >   [...]
> > > >     CC      net/xfrm/xfrm_state.o
> > > >     CC      net/packet/af_packet.o
> > > >   {standard input}: Assembler messages:
> > > >   {standard input}:16055: Error: bad expression
> > > >   {standard input}:16056: Error: bad expression
> > > >   {standard input}:16057: Error: bad expression
> > > >   {standard input}:16058: Error: bad expression
> > > >   {standard input}:16059: Error: bad expression
> > > >     CC      drivers/s390/char/raw3270.o
> > > >     CC      net/ipv6/ip6_output.o
> > > >   [...]
> > > >     CC      net/xfrm/xfrm_output.o
> > > >     CC      net/ipv6/ip6_input.o
> > > >   {standard input}:16055: Error: invalid operands (*ABS* and
> > > > *UND*
> > > > sections) for `%'
> > > >   {standard input}:16056: Error: invalid operands (*ABS* and
> > > > *UND*
> > > > sections) for `%'
> > > >   {standard input}:16057: Error: invalid operands (*ABS* and
> > > > *UND*
> > > > sections) for `%'
> > > >   {standard input}:16058: Error: invalid operands (*ABS* and
> > > > *UND*
> > > > sections) for `%'
> > > >   {standard input}:16059: Error: invalid operands (*ABS* and
> > > > *UND*
> > > > sections) for `%'
> > > >   make[3]: *** [scripts/Makefile.build:249: net/core/filter.o]
> > > > Error 1
> > > >   make[2]: *** [scripts/Makefile.build:465: net/core] Error 2
> > > >   make[2]: *** Waiting for unfinished jobs....
> > > >     CC      net/ipv4/tcp_fastopen.o
> > > >   [...]
> > > >     CC      lib/percpu-refcount.o
> > > >   make[1]: *** [Makefile:1855: net] Error 2
> > > >     CC      lib/rhashtable.o
> > > >   make[1]: *** Waiting for unfinished jobs....
> > > >     CC      lib/base64.o
> > > >   [...]
> > > >     AR      lib/built-in.a
> > > >     CC      kernel/kheaders.o
> > > >     AR      kernel/built-in.a
> > > >   make: *** [Makefile:353: __build_one_by_one] Error 2
> > > >   Error: Process completed with exit code 2.
> > > 
> > > 
> > > it does not break on my cross build with gcc 12, but I can
> > > reproduce with gcc 8 (CI seems to be on gcc 9)
> > > 
> > > the problem seems to be wrong assembler code with extra '%'
> > > that's generated for patchable_function_entry(5)
> > > 
> > > gcc 8 generates:
> > > 
> > > .LPFE1:
> > >         nopr    %%r0
> > >         nopr    %%r0
> > >         nopr    %%r0
> > >         nopr    %%r0
> > >         nopr    %%r0
> > > 
> > > and gcc 12 generates:
> > > 
> > > .LPFE1:
> > >         nopr    %r0
> > >         nopr    %r0
> > >         nopr    %r0
> > >         nopr    %r0
> > >         nopr    %r0
> > > 
> > > perhaps we need to upgrade gcc in CI? cc-ing Ilya, any idea?
> > > 
> > > thanks,
> > > jirka
> > 
> > It's not obvious to me which gcc commit fixed this; I will bisect and
> > find out. This will take some time.
> > 
> > However, officially, the kernel must be buildable by gcc 5.1+.
> > Whatever I find, it's unlikely that we'll be able to backport it
> > that far.
> > 
> > Therefore I think we need to find a way to conditionally
> > do something else when using broken gccs. Or maybe just keep this
> > x86-only after all.
> > 
> > Best regards,
> > Ilya
> 
> FWIW, bisect points to
> 
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=45d06a4045bebc3dbaaf0b1c676f4e22b7c6aca1

great, thanks for doing that

> 
> which makes perfect sense. Still, as I mentioned above, it's probably
> worth tolerating brokens gccs instead of spending time backporting this
> everywhere. And upgrading the CI machine will only paper over the
> issue.
> 
> At a closer look, it looks weird to me that we have
> patchable_function_entry(5) in a common header. If this optimization
> is ever implemented for another architecture, a different number will
> be required.
> 
> For simplicity, would it make sense to hide this under an #ifdef?
> Something like this (untested):
> 
> #ifdef CONFIG_X86
> #define BPF_DISPATCHER_ATTRIBUTES
> __attribute__((patchable_function_entry(5)))
> #else
> #define BPF_DISPATCHER_ATTRIBUTES
> #endif

right, I think we can limit it directly to x86_64 like below

jirka


---
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,				\