The 6.1-rc3 sources fail to build because bpf.h unconditionally
#define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
for X86_64 builds.
I'm pretty sure that should depend on some other options
since the compiler isn't required to support it.
(The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
The only other reference to that attribute is in the definition
of 'notrace' in compiler.h.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight
> Sent: 31 October 2022 11:15
>
> The 6.1-rc3 sources fail to build because bpf.h unconditionally
> #define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> for X86_64 builds.
>
> I'm pretty sure that should depend on some other options
> since the compiler isn't required to support it.
> (The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
>
> The only other reference to that attribute is in the definition
> of 'notrace' in compiler.h.
I think patchable_function_entry was added in gcc 8.
Documentation/process/changes.rst gives the minimal gcc version for
building the kernel as 5.1.
I doubt a increasing it to 8 is acceptable.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Oct 31, 2022 at 11:14:31AM +0000, David Laight wrote:
> The 6.1-rc3 sources fail to build because bpf.h unconditionally
> #define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> for X86_64 builds.
>
> I'm pretty sure that should depend on some other options
> since the compiler isn't required to support it.
> (The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
>
> The only other reference to that attribute is in the definition
> of 'notrace' in compiler.h.
I guess we need to make some __has_attribute check and make all that conditional
cc-ing Peter
jirka
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
On Mon, Oct 31, 2022 at 01:17:16PM +0100, Jiri Olsa wrote:
> On Mon, Oct 31, 2022 at 11:14:31AM +0000, David Laight wrote:
> > The 6.1-rc3 sources fail to build because bpf.h unconditionally
> > #define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> > for X86_64 builds.
> >
> > I'm pretty sure that should depend on some other options
> > since the compiler isn't required to support it.
> > (The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
> >
> > The only other reference to that attribute is in the definition
> > of 'notrace' in compiler.h.
>
> I guess we need to make some __has_attribute check and make all that conditional
>
> cc-ing Peter
Does something crazy like the below work? It compiles but is otherwise
totally untested.
---
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9e7d46d16032..7d7a00306d19 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -953,6 +953,10 @@ struct bpf_dispatcher {
void *rw_image;
u32 image_off;
struct bpf_ksym ksym;
+#ifdef CONFIG_HAVE_STATIC_CALL
+ struct static_call_key *sc_key;
+ void *sc_tramp;
+#endif
};
static __always_inline __nocfi unsigned int bpf_dispatcher_nop_func(
@@ -970,6 +974,20 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
struct bpf_attach_target_info *tgt_info);
void bpf_trampoline_put(struct bpf_trampoline *tr);
int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_funcs);
+
+
+#ifdef CONFIG_HAVE_STATIC_CALL
+#define BPF_DISPATCH_CALL(name) static_call(bpf_dispatcher_##name##_call)(ctx, insnsi, bpf_func)
+
+#define __BPF_DISPATCHER_SC_INIT(_name) \
+ .sc_key = &STATIC_CALL_KEY(_name), \
+ .sc_tramp = STATIC_CALL_TRAMP_ADDR(_name),
+
+#else
+#define BPF_DISPATCH_CALL(name) bpf_func(ctx, insnsi)
+#define __BPF_DISPATCHER_SC_INIT(name)
+#endif
+
#define BPF_DISPATCHER_INIT(_name) { \
.mutex = __MUTEX_INITIALIZER(_name.mutex), \
.func = &_name##_func, \
@@ -981,32 +999,29 @@ int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_func
.name = #_name, \
.lnode = LIST_HEAD_INIT(_name.ksym.lnode), \
}, \
+ __BPF_DISPATCHER_SC_INIT(_name##_call) \
}
-#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 \
+ DEFINE_STATIC_CALL(bpf_dispatcher_##name##_call, bpf_dispatcher_nop_func); \
noinline __nocfi unsigned int bpf_dispatcher_##name##_func( \
const void *ctx, \
const struct bpf_insn *insnsi, \
bpf_func_t bpf_func) \
{ \
- return bpf_func(ctx, insnsi); \
+ return BPF_DISPATCH_CALL(name); \
} \
EXPORT_SYMBOL(bpf_dispatcher_##name##_func); \
struct bpf_dispatcher bpf_dispatcher_##name = \
BPF_DISPATCHER_INIT(bpf_dispatcher_##name);
+
#define DECLARE_BPF_DISPATCHER(name) \
unsigned int bpf_dispatcher_##name##_func( \
const void *ctx, \
const struct bpf_insn *insnsi, \
bpf_func_t bpf_func); \
extern struct bpf_dispatcher bpf_dispatcher_##name;
+
#define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_##name##_func
#define BPF_DISPATCHER_PTR(name) (&bpf_dispatcher_##name)
void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
index fa64b80b8bca..1ca8bd6da6bb 100644
--- a/kernel/bpf/dispatcher.c
+++ b/kernel/bpf/dispatcher.c
@@ -4,6 +4,7 @@
#include <linux/hash.h>
#include <linux/bpf.h>
#include <linux/filter.h>
+#include <linux/static_call.h>
/* The BPF dispatcher is a multiway branch code generator. The
* dispatcher is a mechanism to avoid the performance penalty of an
@@ -106,7 +107,6 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
{
void *old, *new, *tmp;
u32 noff;
- int err;
if (!prev_num_progs) {
old = NULL;
@@ -128,11 +128,10 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
return;
}
- err = bpf_arch_text_poke(d->func, BPF_MOD_JUMP, old, new);
- if (err || !new)
- return;
+ __static_call_update(d->sc_key, d->sc_tramp, new ?: &bpf_dispatcher_nop_func);
- d->image_off = noff;
+ if (new)
+ d->image_off = noff;
}
void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
On Mon, Oct 31, 2022 at 04:21:42PM +0100, Peter Zijlstra wrote:
> On Mon, Oct 31, 2022 at 01:17:16PM +0100, Jiri Olsa wrote:
> > On Mon, Oct 31, 2022 at 11:14:31AM +0000, David Laight wrote:
> > > The 6.1-rc3 sources fail to build because bpf.h unconditionally
> > > #define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> > > for X86_64 builds.
> > >
> > > I'm pretty sure that should depend on some other options
> > > since the compiler isn't required to support it.
> > > (The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
> > >
> > > The only other reference to that attribute is in the definition
> > > of 'notrace' in compiler.h.
> >
> > I guess we need to make some __has_attribute check and make all that conditional
> >
> > cc-ing Peter
>
> Does something crazy like the below work? It compiles but is otherwise
> totally untested.
looks good
it has now the ftrace nop and the jump to the dispatcher image
or the bpf_dispatcher_nop_func.. great :)
bpf_dispatcher_xdp_func:
ffffffff81cc87b0 <load1+0xcc87b0>:
ffffffff81cc87b0: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
ffffffff81cc87b5: e9 a6 fe 65 ff jmp 0xffffffff81328660
tests work for me.. Toke, Bj?rn, could you please check?
thanks,
jirka
>
> ---
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9e7d46d16032..7d7a00306d19 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -953,6 +953,10 @@ struct bpf_dispatcher {
> void *rw_image;
> u32 image_off;
> struct bpf_ksym ksym;
> +#ifdef CONFIG_HAVE_STATIC_CALL
> + struct static_call_key *sc_key;
> + void *sc_tramp;
> +#endif
> };
>
> static __always_inline __nocfi unsigned int bpf_dispatcher_nop_func(
> @@ -970,6 +974,20 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
> struct bpf_attach_target_info *tgt_info);
> void bpf_trampoline_put(struct bpf_trampoline *tr);
> int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_funcs);
> +
> +
> +#ifdef CONFIG_HAVE_STATIC_CALL
> +#define BPF_DISPATCH_CALL(name) static_call(bpf_dispatcher_##name##_call)(ctx, insnsi, bpf_func)
> +
> +#define __BPF_DISPATCHER_SC_INIT(_name) \
> + .sc_key = &STATIC_CALL_KEY(_name), \
> + .sc_tramp = STATIC_CALL_TRAMP_ADDR(_name),
> +
> +#else
> +#define BPF_DISPATCH_CALL(name) bpf_func(ctx, insnsi)
> +#define __BPF_DISPATCHER_SC_INIT(name)
> +#endif
> +
> #define BPF_DISPATCHER_INIT(_name) { \
> .mutex = __MUTEX_INITIALIZER(_name.mutex), \
> .func = &_name##_func, \
> @@ -981,32 +999,29 @@ int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_func
> .name = #_name, \
> .lnode = LIST_HEAD_INIT(_name.ksym.lnode), \
> }, \
> + __BPF_DISPATCHER_SC_INIT(_name##_call) \
> }
>
> -#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 \
> + DEFINE_STATIC_CALL(bpf_dispatcher_##name##_call, bpf_dispatcher_nop_func); \
> noinline __nocfi unsigned int bpf_dispatcher_##name##_func( \
> const void *ctx, \
> const struct bpf_insn *insnsi, \
> bpf_func_t bpf_func) \
> { \
> - return bpf_func(ctx, insnsi); \
> + return BPF_DISPATCH_CALL(name); \
> } \
> EXPORT_SYMBOL(bpf_dispatcher_##name##_func); \
> struct bpf_dispatcher bpf_dispatcher_##name = \
> BPF_DISPATCHER_INIT(bpf_dispatcher_##name);
> +
> #define DECLARE_BPF_DISPATCHER(name) \
> unsigned int bpf_dispatcher_##name##_func( \
> const void *ctx, \
> const struct bpf_insn *insnsi, \
> bpf_func_t bpf_func); \
> extern struct bpf_dispatcher bpf_dispatcher_##name;
> +
> #define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_##name##_func
> #define BPF_DISPATCHER_PTR(name) (&bpf_dispatcher_##name)
> void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> index fa64b80b8bca..1ca8bd6da6bb 100644
> --- a/kernel/bpf/dispatcher.c
> +++ b/kernel/bpf/dispatcher.c
> @@ -4,6 +4,7 @@
> #include <linux/hash.h>
> #include <linux/bpf.h>
> #include <linux/filter.h>
> +#include <linux/static_call.h>
>
> /* The BPF dispatcher is a multiway branch code generator. The
> * dispatcher is a mechanism to avoid the performance penalty of an
> @@ -106,7 +107,6 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> {
> void *old, *new, *tmp;
> u32 noff;
> - int err;
>
> if (!prev_num_progs) {
> old = NULL;
> @@ -128,11 +128,10 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> return;
> }
>
> - err = bpf_arch_text_poke(d->func, BPF_MOD_JUMP, old, new);
> - if (err || !new)
> - return;
> + __static_call_update(d->sc_key, d->sc_tramp, new ?: &bpf_dispatcher_nop_func);
>
> - d->image_off = noff;
> + if (new)
> + d->image_off = noff;
> }
>
> void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
Jiri Olsa <[email protected]> writes:
> On Mon, Oct 31, 2022 at 04:21:42PM +0100, Peter Zijlstra wrote:
>>
>> Does something crazy like the below work? It compiles but is otherwise
>> totally untested.
>
> looks good
>
> it has now the ftrace nop and the jump to the dispatcher image
> or the bpf_dispatcher_nop_func.. great :)
>
> bpf_dispatcher_xdp_func:
>
> ffffffff81cc87b0 <load1+0xcc87b0>:
> ffffffff81cc87b0: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
> ffffffff81cc87b5: e9 a6 fe 65 ff jmp 0xffffffff81328660
>
> tests work for me.. Toke, Björn, could you please check?
Awesome! Much nicer!
For Peter's patch, feel free to add:
Acked-by: Björn Töpel <[email protected]>
Björn Töpel <[email protected]> writes:
> Jiri Olsa <[email protected]> writes:
>
>> tests work for me.. Toke, Björn, could you please check?
>
> Awesome! Much nicer!
>
> For Peter's patch, feel free to add:
> Acked-by: Björn Töpel <[email protected]>
FWIW, I tested on commit 5aaef24b5c6d with dbe69b299884 ("bpf: Fix
dispatcher patchable function entry to 5 bytes nop") reverted, and
Peter's patch applied.