On Mon, Aug 15, 2022 at 08:35:53AM -0700, Alexei Starovoitov wrote:
> On Mon, Aug 15, 2022 at 8:28 AM Peter Zijlstra <[email protected]> wrote:
> >
> > On Mon, Aug 15, 2022 at 08:17:42AM -0700, Alexei Starovoitov wrote:
> > > It's hiding a fake function from ftrace, since it's not a function
> > > and ftrace infra shouldn't show it tracing logs.
> > > In other words it's a _notrace_ function with nop5.
> >
> > Then make it a notrace function with a nop5 in it. That isn't hard.
>
> That's exactly what we're trying to do.
All the while claiming ftrace is broken while it is not.
> Jiri's patch is one way to achieve that.
Fairly horrible way.
> What is your suggestion?
Mailed it already.
> Move it from C to asm ?
Would be much better than proposed IMO.
> Make it naked function with explicit inline asm?
Can be made to work but is iffy because the compiler can do horrible
things with placing the asm().
On Mon, Aug 15, 2022 at 05:48:44PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 15, 2022 at 08:35:53AM -0700, Alexei Starovoitov wrote:
> > On Mon, Aug 15, 2022 at 8:28 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > On Mon, Aug 15, 2022 at 08:17:42AM -0700, Alexei Starovoitov wrote:
> > > > It's hiding a fake function from ftrace, since it's not a function
> > > > and ftrace infra shouldn't show it tracing logs.
> > > > In other words it's a _notrace_ function with nop5.
> > >
> > > Then make it a notrace function with a nop5 in it. That isn't hard.
> >
> > That's exactly what we're trying to do.
>
> All the while claiming ftrace is broken while it is not.
>
> > Jiri's patch is one way to achieve that.
>
> Fairly horrible way.
>
> > What is your suggestion?
>
> Mailed it already.
>
> > Move it from C to asm ?
>
> Would be much better than proposed IMO.
nice, that would be independent of the compiler atributes
and config checking.. will check on this one ;-)
thanks,
jirka
>
> > Make it naked function with explicit inline asm?
>
> Can be made to work but is iffy because the compiler can do horrible
> things with placing the asm().
On Tue, Aug 16, 2022 at 08:56:33AM +0200, Jiri Olsa wrote:
> On Mon, Aug 15, 2022 at 05:48:44PM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 15, 2022 at 08:35:53AM -0700, Alexei Starovoitov wrote:
> > > On Mon, Aug 15, 2022 at 8:28 AM Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > On Mon, Aug 15, 2022 at 08:17:42AM -0700, Alexei Starovoitov wrote:
> > > > > It's hiding a fake function from ftrace, since it's not a function
> > > > > and ftrace infra shouldn't show it tracing logs.
> > > > > In other words it's a _notrace_ function with nop5.
> > > >
> > > > Then make it a notrace function with a nop5 in it. That isn't hard.
> > >
> > > That's exactly what we're trying to do.
> >
> > All the while claiming ftrace is broken while it is not.
> >
> > > Jiri's patch is one way to achieve that.
> >
> > Fairly horrible way.
> >
> > > What is your suggestion?
> >
> > Mailed it already.
> >
> > > Move it from C to asm ?
> >
> > Would be much better than proposed IMO.
>
> nice, that would be independent of the compiler atributes
> and config checking.. will check on this one ;-)
how about something like below?
dispatcher code is generated only for x86_64, so that will be covered
by the assembly version (free of ftrace table) other archs stay same
jirka
----
diff --git a/arch/x86/net/Makefile b/arch/x86/net/Makefile
index 383c87300b0d..94964002eaae 100644
--- a/arch/x86/net/Makefile
+++ b/arch/x86/net/Makefile
@@ -7,4 +7,5 @@ ifeq ($(CONFIG_X86_32),y)
obj-$(CONFIG_BPF_JIT) += bpf_jit_comp32.o
else
obj-$(CONFIG_BPF_JIT) += bpf_jit_comp.o
+ obj-$(CONFIG_BPF_JIT) += bpf_dispatcher.o
endif
diff --git a/arch/x86/net/bpf_dispatcher.S b/arch/x86/net/bpf_dispatcher.S
new file mode 100644
index 000000000000..65790a1286e8
--- /dev/null
+++ b/arch/x86/net/bpf_dispatcher.S
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/linkage.h>
+#include <asm/nops.h>
+#include <asm/nospec-branch.h>
+
+ .text
+SYM_FUNC_START(bpf_dispatcher_xdp_func)
+ ASM_NOP5
+ JMP_NOSPEC rdx
+SYM_FUNC_END(bpf_dispatcher_xdp_func)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a627a02cf8ab..03b54c820b95 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -924,7 +924,7 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
}
#define DEFINE_BPF_DISPATCHER(name) \
- noinline __nocfi unsigned int bpf_dispatcher_##name##_func( \
+ noinline __nocfi unsigned int __weak bpf_dispatcher_##name##_func(\
const void *ctx, \
const struct bpf_insn *insnsi, \
bpf_func_t bpf_func) \
On Wed, Aug 17, 2022 at 2:29 AM Jiri Olsa <[email protected]> wrote:
>
> On Tue, Aug 16, 2022 at 08:56:33AM +0200, Jiri Olsa wrote:
> > On Mon, Aug 15, 2022 at 05:48:44PM +0200, Peter Zijlstra wrote:
> > > On Mon, Aug 15, 2022 at 08:35:53AM -0700, Alexei Starovoitov wrote:
> > > > On Mon, Aug 15, 2022 at 8:28 AM Peter Zijlstra <[email protected]> wrote:
> > > > >
> > > > > On Mon, Aug 15, 2022 at 08:17:42AM -0700, Alexei Starovoitov wrote:
> > > > > > It's hiding a fake function from ftrace, since it's not a function
> > > > > > and ftrace infra shouldn't show it tracing logs.
> > > > > > In other words it's a _notrace_ function with nop5.
> > > > >
> > > > > Then make it a notrace function with a nop5 in it. That isn't hard.
> > > >
> > > > That's exactly what we're trying to do.
> > >
> > > All the while claiming ftrace is broken while it is not.
> > >
> > > > Jiri's patch is one way to achieve that.
> > >
> > > Fairly horrible way.
> > >
> > > > What is your suggestion?
> > >
> > > Mailed it already.
> > >
> > > > Move it from C to asm ?
> > >
> > > Would be much better than proposed IMO.
> >
> > nice, that would be independent of the compiler atributes
> > and config checking.. will check on this one ;-)
>
> how about something like below?
>
> dispatcher code is generated only for x86_64, so that will be covered
> by the assembly version (free of ftrace table) other archs stay same
>
> jirka
>
>
> ----
> diff --git a/arch/x86/net/Makefile b/arch/x86/net/Makefile
> index 383c87300b0d..94964002eaae 100644
> --- a/arch/x86/net/Makefile
> +++ b/arch/x86/net/Makefile
> @@ -7,4 +7,5 @@ ifeq ($(CONFIG_X86_32),y)
> obj-$(CONFIG_BPF_JIT) += bpf_jit_comp32.o
> else
> obj-$(CONFIG_BPF_JIT) += bpf_jit_comp.o
> + obj-$(CONFIG_BPF_JIT) += bpf_dispatcher.o
> endif
> diff --git a/arch/x86/net/bpf_dispatcher.S b/arch/x86/net/bpf_dispatcher.S
> new file mode 100644
> index 000000000000..65790a1286e8
> --- /dev/null
> +++ b/arch/x86/net/bpf_dispatcher.S
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/linkage.h>
> +#include <asm/nops.h>
> +#include <asm/nospec-branch.h>
> +
> + .text
> +SYM_FUNC_START(bpf_dispatcher_xdp_func)
> + ASM_NOP5
> + JMP_NOSPEC rdx
> +SYM_FUNC_END(bpf_dispatcher_xdp_func)
Wait. Why asm ? Did you try Peter's suggestion:
__attribute__((__no_instrument_function__))
__attribute__((patchable_function_entry(5)))
?
On Wed, Aug 17, 2022 at 09:57:45AM -0700, Alexei Starovoitov wrote:
> On Wed, Aug 17, 2022 at 2:29 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Tue, Aug 16, 2022 at 08:56:33AM +0200, Jiri Olsa wrote:
> > > On Mon, Aug 15, 2022 at 05:48:44PM +0200, Peter Zijlstra wrote:
> > > > On Mon, Aug 15, 2022 at 08:35:53AM -0700, Alexei Starovoitov wrote:
> > > > > On Mon, Aug 15, 2022 at 8:28 AM Peter Zijlstra <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Aug 15, 2022 at 08:17:42AM -0700, Alexei Starovoitov wrote:
> > > > > > > It's hiding a fake function from ftrace, since it's not a function
> > > > > > > and ftrace infra shouldn't show it tracing logs.
> > > > > > > In other words it's a _notrace_ function with nop5.
> > > > > >
> > > > > > Then make it a notrace function with a nop5 in it. That isn't hard.
> > > > >
> > > > > That's exactly what we're trying to do.
> > > >
> > > > All the while claiming ftrace is broken while it is not.
> > > >
> > > > > Jiri's patch is one way to achieve that.
> > > >
> > > > Fairly horrible way.
> > > >
> > > > > What is your suggestion?
> > > >
> > > > Mailed it already.
> > > >
> > > > > Move it from C to asm ?
> > > >
> > > > Would be much better than proposed IMO.
> > >
> > > nice, that would be independent of the compiler atributes
> > > and config checking.. will check on this one ;-)
> >
> > how about something like below?
> >
> > dispatcher code is generated only for x86_64, so that will be covered
> > by the assembly version (free of ftrace table) other archs stay same
> >
> > jirka
> >
> >
> > ----
> > diff --git a/arch/x86/net/Makefile b/arch/x86/net/Makefile
> > index 383c87300b0d..94964002eaae 100644
> > --- a/arch/x86/net/Makefile
> > +++ b/arch/x86/net/Makefile
> > @@ -7,4 +7,5 @@ ifeq ($(CONFIG_X86_32),y)
> > obj-$(CONFIG_BPF_JIT) += bpf_jit_comp32.o
> > else
> > obj-$(CONFIG_BPF_JIT) += bpf_jit_comp.o
> > + obj-$(CONFIG_BPF_JIT) += bpf_dispatcher.o
> > endif
> > diff --git a/arch/x86/net/bpf_dispatcher.S b/arch/x86/net/bpf_dispatcher.S
> > new file mode 100644
> > index 000000000000..65790a1286e8
> > --- /dev/null
> > +++ b/arch/x86/net/bpf_dispatcher.S
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <linux/linkage.h>
> > +#include <asm/nops.h>
> > +#include <asm/nospec-branch.h>
> > +
> > + .text
> > +SYM_FUNC_START(bpf_dispatcher_xdp_func)
> > + ASM_NOP5
> > + JMP_NOSPEC rdx
> > +SYM_FUNC_END(bpf_dispatcher_xdp_func)
>
> Wait. Why asm ? Did you try Peter's suggestion:
> __attribute__((__no_instrument_function__))
> __attribute__((patchable_function_entry(5)))
ah so this suggestion came in the other thread after the asm one.. ok, will check
jirka