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

[PATCH 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.

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               |  2 ++
 kernel/trace/Kconfig              |  6 ++++++
 tools/objtool/check.c             |  3 ++-
 5 files changed, 21 insertions(+), 2 deletions(-)

[PATCH 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

[PATCH 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.

The dispatcher code is generated and attached only for x86 so it's safe
to keep bpf_dispatcher func in patchable_function_entry locations for
other archs.

[1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/Kconfig    | 1 +
 include/linux/bpf.h | 2 ++
 2 files changed, 3 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..945d5414bb62 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -925,6 +925,8 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
 }
 
 #define DEFINE_BPF_DISPATCHER(name)					\
+	__attribute__((__no_instrument_function__))			\
+	__attribute__((patchable_function_entry(5)))			\
 	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
 		const void *ctx,					\
 		const struct bpf_insn *insnsi,				\
-- 
2.37.2

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

[KP Singh]

On Fri, Aug 26, 2022 at 8:46 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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.
>
> The dispatcher code is generated and attached only for x86 so it's safe
> to keep bpf_dispatcher func in patchable_function_entry locations for
> other archs.
>
> [1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

I read the whole discussion in
https://lore.kernel.org/bpf/CAADnVQLMY9Gbipx6Tc4dsRN3YSXo2CF8T=KwAvjU7nRDjYxRgQ@mail.gmail.com/

and this LGTM

Acked-by: KP Singh <kpsingh@kernel.org>

> ---
>  arch/x86/Kconfig    | 1 +
>  include/linux/bpf.h | 2 ++
>  2 files changed, 3 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..945d5414bb62 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -925,6 +925,8 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
>  }
>
>  #define DEFINE_BPF_DISPATCHER(name)                                    \
> +       __attribute__((__no_instrument_function__))                     \
> +       __attribute__((patchable_function_entry(5)))                    \
>         noinline __nocfi unsigned int bpf_dispatcher_##name##_func(     \
>                 const void *ctx,                                        \
>                 const struct bpf_insn *insnsi,                          \
> --
> 2.37.2
>

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

[Peter Zijlstra]

On Fri, Aug 26, 2022 at 08:46:08PM +0200, Jiri Olsa wrote:
> 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

This is typically spelled like: 'notrace' in the kernel.

>   - 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.
> 
> The dispatcher code is generated and attached only for x86 so it's safe
> to keep bpf_dispatcher func in patchable_function_entry locations for
> other archs.
> 
> [1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/Kconfig    | 1 +
>  include/linux/bpf.h | 2 ++
>  2 files changed, 3 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..945d5414bb62 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -925,6 +925,8 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
>  }
>  
>  #define DEFINE_BPF_DISPATCHER(name)					\
> +	__attribute__((__no_instrument_function__))			\
> +	__attribute__((patchable_function_entry(5)))			\
>  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
>  		const void *ctx,					\
>  		const struct bpf_insn *insnsi,				\

What makes that whole dispatcher thing x86 only? AFAICT it is only under
BPF_JIT here and could be used by anyone.

ARM64 for instance has BPG_JIT and builds net/core/filter.c. And ARM64
very much does use patchable_function_entry() for its ftrace
implementation.

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

[Jiri Olsa]

On Tue, Aug 30, 2022 at 12:44:13PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 26, 2022 at 08:46:08PM +0200, Jiri Olsa wrote:
> > 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
> 
> This is typically spelled like: 'notrace' in the kernel.
> 
> >   - 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.
> > 
> > The dispatcher code is generated and attached only for x86 so it's safe
> > to keep bpf_dispatcher func in patchable_function_entry locations for
> > other archs.
> > 
> > [1] https://lore.kernel.org/bpf/20220722110811.124515-1-jolsa@kernel.org/
> > Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  arch/x86/Kconfig    | 1 +
> >  include/linux/bpf.h | 2 ++
> >  2 files changed, 3 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..945d5414bb62 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -925,6 +925,8 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
> >  }
> >  
> >  #define DEFINE_BPF_DISPATCHER(name)					\
> > +	__attribute__((__no_instrument_function__))			\
> > +	__attribute__((patchable_function_entry(5)))			\
> >  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
> >  		const void *ctx,					\
> >  		const struct bpf_insn *insnsi,				\
> 
> What makes that whole dispatcher thing x86 only? AFAICT it is only under
> BPF_JIT here and could be used by anyone.

it is just optimalization on x86, that transform default indirect calls to
direct call,  described in changelog in here:

  75ccbef6369e bpf: Introduce BPF dispatcher

other archs just make the call to bpf_func

jirka

> 
> ARM64 for instance has BPG_JIT and builds net/core/filter.c. And ARM64
> very much does use patchable_function_entry() for its ftrace
> implementation.

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

[Daniel Borkmann]

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.

> [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               |  2 ++
>   kernel/trace/Kconfig              |  6 ++++++
>   tools/objtool/check.c             |  3 ++-
>   5 files changed, 21 insertions(+), 2 deletions(-)
> 

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

[Jiri Olsa]

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

> 
> > [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               |  2 ++
> >   kernel/trace/Kconfig              |  6 ++++++
> >   tools/objtool/check.c             |  3 ++-
> >   5 files changed, 21 insertions(+), 2 deletions(-)
> > 
> 

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

[Ilya Leoshkevich]

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

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

[Ilya Leoshkevich]

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

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

Best regards,
Ilya

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

[Jiri Olsa]

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