| Message ID | 20240812234356.2089263-2-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | support nocsr patterns for calls to kfuncs | expand |
On 8/12/24 4:43 PM, Eduard Zingerman wrote: > Recognize nocsr patterns around kfunc calls. > For example, suppose bpf_cast_to_kern_ctx() follows nocsr contract > (which it does, it is rewritten by verifier as "r0 = r1" insn), > in such a case, rewrite BPF program below: > > r2 = 1; > *(u64 *)(r10 - 32) = r2; > call %[bpf_cast_to_kern_ctx]; > r2 = *(u64 *)(r10 - 32); > r0 = r2; > > Removing the spill/fill pair: > > r2 = 1; > call %[bpf_cast_to_kern_ctx]; > r0 = r2; I can see this indeed a good optimization esp. when there is a register pressure for the program, and like above r2 has to be spilled. Using nocsr for bpf_cast_to_kern_ctx() can remove those spill/fill insns. > > Add a KF_NOCSR flag to mark kfuncs that follow nocsr contract. > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > include/linux/btf.h | 1 + > kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index cffb43133c68..59ca37300423 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -75,6 +75,7 @@ > #define KF_ITER_NEXT (1 << 9) /* kfunc implements BPF iter next method */ > #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter destructor */ > #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */ > +#define KF_NOCSR (1 << 12) /* kfunc follows nocsr calling contract */ > > /* > * Tag marking a kernel function as a kfunc. This is meant to minimize the > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index df3be12096cf..c579f74be3f9 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > } > } > > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) > +{ > + const struct btf_param *params; > + u32 vlen, i, mask; In helper_nocsr_clobber_mask, we have u8 mask. To be consistent, can we have 'u8 mask' here? Are you worried that the number of arguments could be more than 7? This seems not the case right now. > + > + params = btf_params(meta->func_proto); > + vlen = btf_type_vlen(meta->func_proto); > + mask = 0; > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) > + mask |= BIT(BPF_REG_0); > + for (i = 0; i < vlen; ++i) > + mask |= BIT(BPF_REG_1 + i); > + return mask; > +} > + > +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ > +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) > +{ > + return false; > +} > + > /* GCC and LLVM define a no_caller_saved_registers function attribute. > * This attribute means that function scratches only some of > * the caller saved registers defined by ABI. > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, > bpf_jit_inlines_helper_call(call->imm)); > } > > + if (bpf_pseudo_kfunc_call(call)) { > + struct bpf_kfunc_call_arg_meta meta; > + int err; > + > + err = fetch_kfunc_meta(env, call, &meta, NULL); > + if (err < 0) > + /* error would be reported later */ > + return; > + > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && > + verifier_inlines_kfunc_call(&meta); I think we do not need both meta.kfunc_flags & KF_NOCSR and verifier_inlines_kfunc_call(&meta). Only one of them is enough since they test very similar thing. You do need to ensure kfuncs with KF_NOCSR in special_kfunc_list though. WDYT? > + } > + > if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) > return; >
On Mon, 2024-08-12 at 22:36 -0700, Yonghong Song wrote: [...] > > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > } > > } > > > > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ > > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) > > +{ > > + const struct btf_param *params; > > + u32 vlen, i, mask; > > In helper_nocsr_clobber_mask, we have u8 mask. To be consistent, can we have 'u8 mask' here? > Are you worried that the number of arguments could be more than 7? This seems not the case > right now. Before the nocsr part for helpers landed there was a change request to make helper_nocsr_clobber_mask() return u32. I modified the function but forgot to change the type for 'mask' local variable. The main point in using u32 is uniformity. I can either change kfunc_nocsr_clobber_mask() to use u8 for mask, or update helper_nocsr_clobber_mask() to use u32 for mask. > > > + > > + params = btf_params(meta->func_proto); > > + vlen = btf_type_vlen(meta->func_proto); > > + mask = 0; > > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) > > + mask |= BIT(BPF_REG_0); > > + for (i = 0; i < vlen; ++i) > > + mask |= BIT(BPF_REG_1 + i); > > + return mask; > > +} > > + > > +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ > > +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) > > +{ > > + return false; > > +} > > + > > /* GCC and LLVM define a no_caller_saved_registers function attribute. > > * This attribute means that function scratches only some of > > * the caller saved registers defined by ABI. > > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, > > bpf_jit_inlines_helper_call(call->imm)); > > } > > > > + if (bpf_pseudo_kfunc_call(call)) { > > + struct bpf_kfunc_call_arg_meta meta; > > + int err; > > + > > + err = fetch_kfunc_meta(env, call, &meta, NULL); > > + if (err < 0) > > + /* error would be reported later */ > > + return; > > + > > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); > > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && > > + verifier_inlines_kfunc_call(&meta); > > I think we do not need both meta.kfunc_flags & KF_NOCSR and > verifier_inlines_kfunc_call(&meta). Only one of them is enough > since they test very similar thing. You do need to ensure > kfuncs with KF_NOCSR in special_kfunc_list though. > WDYT? I can remove the flag in favour of verifier_inlines_kfunc_call(). > > > + } > > + > > if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) > > return; > >
On 8/13/24 12:55 AM, Eduard Zingerman wrote: > On Mon, 2024-08-12 at 22:36 -0700, Yonghong Song wrote: > > [...] > >>> @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >>> } >>> } >>> >>> +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ >>> +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) >>> +{ >>> + const struct btf_param *params; >>> + u32 vlen, i, mask; >> In helper_nocsr_clobber_mask, we have u8 mask. To be consistent, can we have 'u8 mask' here? >> Are you worried that the number of arguments could be more than 7? This seems not the case >> right now. > Before the nocsr part for helpers landed there was a change request to > make helper_nocsr_clobber_mask() return u32. I modified the function > but forgot to change the type for 'mask' local variable. > > The main point in using u32 is uniformity. > I can either change kfunc_nocsr_clobber_mask() to use u8 for mask, > or update helper_nocsr_clobber_mask() to use u32 for mask. Changing to u32 in helper_nocsr_clobber_mask() is okay. I just want to have consistent type for 'mask' in both functions. > >>> + >>> + params = btf_params(meta->func_proto); >>> + vlen = btf_type_vlen(meta->func_proto); >>> + mask = 0; >>> + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) >>> + mask |= BIT(BPF_REG_0); >>> + for (i = 0; i < vlen; ++i) >>> + mask |= BIT(BPF_REG_1 + i); >>> + return mask; >>> +} >>> + >>> +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ >>> +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) >>> +{ >>> + return false; >>> +} >>> + >>> /* GCC and LLVM define a no_caller_saved_registers function attribute. >>> * This attribute means that function scratches only some of >>> * the caller saved registers defined by ABI. >>> @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, >>> bpf_jit_inlines_helper_call(call->imm)); >>> } >>> >>> + if (bpf_pseudo_kfunc_call(call)) { >>> + struct bpf_kfunc_call_arg_meta meta; >>> + int err; >>> + >>> + err = fetch_kfunc_meta(env, call, &meta, NULL); >>> + if (err < 0) >>> + /* error would be reported later */ >>> + return; >>> + >>> + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); >>> + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && >>> + verifier_inlines_kfunc_call(&meta); >> I think we do not need both meta.kfunc_flags & KF_NOCSR and >> verifier_inlines_kfunc_call(&meta). Only one of them is enough >> since they test very similar thing. You do need to ensure >> kfuncs with KF_NOCSR in special_kfunc_list though. >> WDYT? > I can remove the flag in favour of verifier_inlines_kfunc_call(). Sounds good to me. > >>> + } >>> + >>> if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) >>> return; >>> >
On Tue, 2024-08-13 at 08:18 -0700, Yonghong Song wrote: [...] > > > > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, > > > > bpf_jit_inlines_helper_call(call->imm)); > > > > } > > > > > > > > + if (bpf_pseudo_kfunc_call(call)) { > > > > + struct bpf_kfunc_call_arg_meta meta; > > > > + int err; > > > > + > > > > + err = fetch_kfunc_meta(env, call, &meta, NULL); > > > > + if (err < 0) > > > > + /* error would be reported later */ > > > > + return; > > > > + > > > > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); > > > > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && > > > > + verifier_inlines_kfunc_call(&meta); > > > I think we do not need both meta.kfunc_flags & KF_NOCSR and > > > verifier_inlines_kfunc_call(&meta). Only one of them is enough > > > since they test very similar thing. You do need to ensure > > > kfuncs with KF_NOCSR in special_kfunc_list though. > > > WDYT? > > I can remove the flag in favour of verifier_inlines_kfunc_call(). > > Sounds good to me. Just one more point. The reason I added the KF_NOCSR was to keep the code as close to helpers case as possible. For helpers there are two guards: - verifier_inlines_helper_call() function shared between mark_nocsr_pattern_for_call() and do_misc_fixups(); - bpf_func_proto->allow_nocsr flag. The idea is that verifier might inline some functions w/o allowing nocsr. Hence I decided to use KF_NOCSR in place of bpf_func_proto->allow_nocsr. On the other hand, verifier_inlines_kfunc_call() is not used by any other function except mark_nocsr_pattern_for_call() at the moment, so the KF_NOCSR flag might be redundant indeed.
On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > Recognize nocsr patterns around kfunc calls. > For example, suppose bpf_cast_to_kern_ctx() follows nocsr contract > (which it does, it is rewritten by verifier as "r0 = r1" insn), > in such a case, rewrite BPF program below: > > r2 = 1; > *(u64 *)(r10 - 32) = r2; > call %[bpf_cast_to_kern_ctx]; > r2 = *(u64 *)(r10 - 32); > r0 = r2; > > Removing the spill/fill pair: > > r2 = 1; > call %[bpf_cast_to_kern_ctx]; > r0 = r2; > > Add a KF_NOCSR flag to mark kfuncs that follow nocsr contract. > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > include/linux/btf.h | 1 + > kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index cffb43133c68..59ca37300423 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -75,6 +75,7 @@ > #define KF_ITER_NEXT (1 << 9) /* kfunc implements BPF iter next method */ > #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter destructor */ > #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */ > +#define KF_NOCSR (1 << 12) /* kfunc follows nocsr calling contract */ > > /* > * Tag marking a kernel function as a kfunc. This is meant to minimize the > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index df3be12096cf..c579f74be3f9 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > } > } > > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) > +{ > + const struct btf_param *params; > + u32 vlen, i, mask; > + > + params = btf_params(meta->func_proto); > + vlen = btf_type_vlen(meta->func_proto); > + mask = 0; > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) > + mask |= BIT(BPF_REG_0); > + for (i = 0; i < vlen; ++i) > + mask |= BIT(BPF_REG_1 + i); Somewhere deep in btf_dump implementation of libbpf, there is a special handling of `<whatever> func(void)` (no args) function as having vlen == 1 and type being VOID (i.e., zero). I don't know if that still can happen, but I believe at some point we could get this vlen==1 and type=VOID for no-args functions. So I wonder if we should handle that here as well, or is it some compiler atavism we can forget about? > + return mask; > +} > + > +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ > +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) > +{ > + return false; > +} > + > /* GCC and LLVM define a no_caller_saved_registers function attribute. > * This attribute means that function scratches only some of > * the caller saved registers defined by ABI. > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, > bpf_jit_inlines_helper_call(call->imm)); > } > > + if (bpf_pseudo_kfunc_call(call)) { > + struct bpf_kfunc_call_arg_meta meta; > + int err; > + > + err = fetch_kfunc_meta(env, call, &meta, NULL); > + if (err < 0) > + /* error would be reported later */ > + return; > + > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && > + verifier_inlines_kfunc_call(&meta); > + } > + > if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) > return; > > -- > 2.45.2 >
On Thu, 2024-08-15 at 14:24 -0700, Andrii Nakryiko wrote: [...] > > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > } > > } > > > > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ > > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) > > +{ > > + const struct btf_param *params; > > + u32 vlen, i, mask; > > + > > + params = btf_params(meta->func_proto); > > + vlen = btf_type_vlen(meta->func_proto); > > + mask = 0; > > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) > > + mask |= BIT(BPF_REG_0); > > + for (i = 0; i < vlen; ++i) > > + mask |= BIT(BPF_REG_1 + i); > > Somewhere deep in btf_dump implementation of libbpf, there is a > special handling of `<whatever> func(void)` (no args) function as > having vlen == 1 and type being VOID (i.e., zero). I don't know if > that still can happen, but I believe at some point we could get this > vlen==1 and type=VOID for no-args functions. So I wonder if we should > handle that here as well, or is it some compiler atavism we can forget > about? > I just checked BTF generated for 'int filelock_init(void)', for gcc compiled kernel using latest pahole and func proto looks as follows: FUNC_PROTO '(anon)' ret_type_id=12 vlen=0 So I assume this is an atavism. [...]
On 8/15/24 2:24 PM, Andrii Nakryiko wrote: > On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <eddyz87@gmail.com> wrote: >> Recognize nocsr patterns around kfunc calls. >> For example, suppose bpf_cast_to_kern_ctx() follows nocsr contract >> (which it does, it is rewritten by verifier as "r0 = r1" insn), >> in such a case, rewrite BPF program below: >> >> r2 = 1; >> *(u64 *)(r10 - 32) = r2; >> call %[bpf_cast_to_kern_ctx]; >> r2 = *(u64 *)(r10 - 32); >> r0 = r2; >> >> Removing the spill/fill pair: >> >> r2 = 1; >> call %[bpf_cast_to_kern_ctx]; >> r0 = r2; >> >> Add a KF_NOCSR flag to mark kfuncs that follow nocsr contract. >> >> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> >> --- >> include/linux/btf.h | 1 + >> kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/include/linux/btf.h b/include/linux/btf.h >> index cffb43133c68..59ca37300423 100644 >> --- a/include/linux/btf.h >> +++ b/include/linux/btf.h >> @@ -75,6 +75,7 @@ >> #define KF_ITER_NEXT (1 << 9) /* kfunc implements BPF iter next method */ >> #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter destructor */ >> #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */ >> +#define KF_NOCSR (1 << 12) /* kfunc follows nocsr calling contract */ >> >> /* >> * Tag marking a kernel function as a kfunc. This is meant to minimize the >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index df3be12096cf..c579f74be3f9 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >> } >> } >> >> +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ >> +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) >> +{ >> + const struct btf_param *params; >> + u32 vlen, i, mask; >> + >> + params = btf_params(meta->func_proto); >> + vlen = btf_type_vlen(meta->func_proto); >> + mask = 0; >> + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) >> + mask |= BIT(BPF_REG_0); >> + for (i = 0; i < vlen; ++i) >> + mask |= BIT(BPF_REG_1 + i); > Somewhere deep in btf_dump implementation of libbpf, there is a > special handling of `<whatever> func(void)` (no args) function as > having vlen == 1 and type being VOID (i.e., zero). I don't know if > that still can happen, but I believe at some point we could get this > vlen==1 and type=VOID for no-args functions. So I wonder if we should > handle that here as well, or is it some compiler atavism we can forget > about? The case to have vlen=1 and type=VOID only happens for bpf programs with llvm19 and later. For example, $ cat t.c int foo(); // a kfunc or a helper int bar() { return foo(1, 2); } $ clang --target=bpf -O2 -g -c t.c && llvm-dwarfdump t.o t.c:3:13: warning: passing arguments to 'foo' without a prototype is deprecated in all versions of C and is not supported in C23 [-Wdeprecated-non-prototype] 3 | return foo(1, 2); | ^ 1 warning generated. t.o: file format elf64-bpf ... 0x00000039: DW_TAG_subprogram DW_AT_name ("foo") DW_AT_decl_file ("/home/yhs/t.c") DW_AT_decl_line (1) DW_AT_type (0x00000043 "int") DW_AT_declaration (true) DW_AT_external (true) 0x00000041: DW_TAG_unspecified_parameters 0x00000042: NULL ... If we do see a BPF kfunc/helper with vlen=1 and type is VOID, that means the number of arguments is actual UNKNOWN based on dwarf DW_TAG_subprogram tag. Although it is unlikely people to write code like above, it might be still useful to add check with vlen=1 and type=VOID and reject such a case. > >> + return mask; >> +} >> + >> +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ >> +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) >> +{ >> + return false; >> +} >> + >> /* GCC and LLVM define a no_caller_saved_registers function attribute. >> * This attribute means that function scratches only some of >> * the caller saved registers defined by ABI. >> @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, >> bpf_jit_inlines_helper_call(call->imm)); >> } >> >> + if (bpf_pseudo_kfunc_call(call)) { >> + struct bpf_kfunc_call_arg_meta meta; >> + int err; >> + >> + err = fetch_kfunc_meta(env, call, &meta, NULL); >> + if (err < 0) >> + /* error would be reported later */ >> + return; >> + >> + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); >> + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && >> + verifier_inlines_kfunc_call(&meta); >> + } >> + >> if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) >> return; >> >> -- >> 2.45.2 >>
On 8/15/24 3:16 PM, Yonghong Song wrote: > > On 8/15/24 2:24 PM, Andrii Nakryiko wrote: >> On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <eddyz87@gmail.com> >> wrote: >>> Recognize nocsr patterns around kfunc calls. >>> For example, suppose bpf_cast_to_kern_ctx() follows nocsr contract >>> (which it does, it is rewritten by verifier as "r0 = r1" insn), >>> in such a case, rewrite BPF program below: >>> >>> r2 = 1; >>> *(u64 *)(r10 - 32) = r2; >>> call %[bpf_cast_to_kern_ctx]; >>> r2 = *(u64 *)(r10 - 32); >>> r0 = r2; >>> >>> Removing the spill/fill pair: >>> >>> r2 = 1; >>> call %[bpf_cast_to_kern_ctx]; >>> r0 = r2; >>> >>> Add a KF_NOCSR flag to mark kfuncs that follow nocsr contract. >>> >>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> >>> --- >>> include/linux/btf.h | 1 + >>> kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 37 insertions(+) >>> >>> diff --git a/include/linux/btf.h b/include/linux/btf.h >>> index cffb43133c68..59ca37300423 100644 >>> --- a/include/linux/btf.h >>> +++ b/include/linux/btf.h >>> @@ -75,6 +75,7 @@ >>> #define KF_ITER_NEXT (1 << 9) /* kfunc implements BPF iter next >>> method */ >>> #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter >>> destructor */ >>> #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by >>> rcu cs when they are invoked */ >>> +#define KF_NOCSR (1 << 12) /* kfunc follows nocsr calling >>> contract */ >>> >>> /* >>> * Tag marking a kernel function as a kfunc. This is meant to >>> minimize the >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index df3be12096cf..c579f74be3f9 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -16140,6 +16140,28 @@ static bool >>> verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >>> } >>> } >>> >>> +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment >>> above */ >>> +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta >>> *meta) >>> +{ >>> + const struct btf_param *params; >>> + u32 vlen, i, mask; >>> + >>> + params = btf_params(meta->func_proto); >>> + vlen = btf_type_vlen(meta->func_proto); >>> + mask = 0; >>> + if (!btf_type_is_void(btf_type_by_id(meta->btf, >>> meta->func_proto->type))) >>> + mask |= BIT(BPF_REG_0); >>> + for (i = 0; i < vlen; ++i) >>> + mask |= BIT(BPF_REG_1 + i); >> Somewhere deep in btf_dump implementation of libbpf, there is a >> special handling of `<whatever> func(void)` (no args) function as >> having vlen == 1 and type being VOID (i.e., zero). I don't know if >> that still can happen, but I believe at some point we could get this >> vlen==1 and type=VOID for no-args functions. So I wonder if we should >> handle that here as well, or is it some compiler atavism we can forget >> about? > > The case to have vlen=1 and type=VOID only happens for > bpf programs with llvm19 and later. > For example, > > $ cat t.c > int foo(); // a kfunc or a helper > int bar() { > return foo(1, 2); > } > > $ clang --target=bpf -O2 -g -c t.c && llvm-dwarfdump t.o > t.c:3:13: warning: passing arguments to 'foo' without a prototype is > deprecated in all versions of C and is not supported in C23 > [-Wdeprecated-non-prototype] > 3 | return foo(1, 2); > | ^ > 1 warning generated. > t.o: file format elf64-bpf > ... > 0x00000039: DW_TAG_subprogram > DW_AT_name ("foo") > DW_AT_decl_file ("/home/yhs/t.c") > DW_AT_decl_line (1) > DW_AT_type (0x00000043 "int") > DW_AT_declaration (true) > DW_AT_external (true) > > 0x00000041: DW_TAG_unspecified_parameters > > 0x00000042: NULL > ... > > If we do see a BPF kfunc/helper with vlen=1 and type is VOID, > that means the number of arguments is actual UNKNOWN > based on dwarf DW_TAG_subprogram tag. Although it is unlikely > people to write code like above, it might be still useful > to add check with vlen=1 and type=VOID and reject such a case. For vmlinux BTF, this is not possible since eventually all function has a definition which will define the function precisely w.r.t. the number of arguments and their types. > > >> >>> + return mask; >>> +} >>> + >>> +/* Same as verifier_inlines_helper_call() but for kfuncs, see >>> comment above */ >>> +static bool verifier_inlines_kfunc_call(struct >>> bpf_kfunc_call_arg_meta *meta) >>> +{ >>> + return false; >>> +} >>> + >>> /* GCC and LLVM define a no_caller_saved_registers function >>> attribute. >>> * This attribute means that function scratches only some of >>> * the caller saved registers defined by ABI. >>> @@ -16238,6 +16260,20 @@ static void >>> mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, >>> bpf_jit_inlines_helper_call(call->imm)); >>> } >>> >>> + if (bpf_pseudo_kfunc_call(call)) { >>> + struct bpf_kfunc_call_arg_meta meta; >>> + int err; >>> + >>> + err = fetch_kfunc_meta(env, call, &meta, NULL); >>> + if (err < 0) >>> + /* error would be reported later */ >>> + return; >>> + >>> + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); >>> + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && >>> + verifier_inlines_kfunc_call(&meta); >>> + } >>> + >>> if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) >>> return; >>> >>> -- >>> 2.45.2 >>>
On 8/15/24 3:07 PM, Eduard Zingerman wrote: > On Thu, 2024-08-15 at 14:24 -0700, Andrii Nakryiko wrote: > > [...] > >>> @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >>> } >>> } >>> >>> +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ >>> +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) >>> +{ >>> + const struct btf_param *params; >>> + u32 vlen, i, mask; >>> + >>> + params = btf_params(meta->func_proto); >>> + vlen = btf_type_vlen(meta->func_proto); >>> + mask = 0; >>> + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) >>> + mask |= BIT(BPF_REG_0); >>> + for (i = 0; i < vlen; ++i) >>> + mask |= BIT(BPF_REG_1 + i); >> Somewhere deep in btf_dump implementation of libbpf, there is a >> special handling of `<whatever> func(void)` (no args) function as >> having vlen == 1 and type being VOID (i.e., zero). I don't know if >> that still can happen, but I believe at some point we could get this >> vlen==1 and type=VOID for no-args functions. So I wonder if we should >> handle that here as well, or is it some compiler atavism we can forget >> about? >> > I just checked BTF generated for 'int filelock_init(void)', > for gcc compiled kernel using latest pahole and func proto looks as follows: > > FUNC_PROTO '(anon)' ret_type_id=12 vlen=0 > > So I assume this is an atavism. Agree, for kernel vmlinux BTF, we should be fine. > > [...] > >
On Thu, 2024-08-15 at 15:23 -0700, Yonghong Song wrote: > On 8/15/24 3:07 PM, Eduard Zingerman wrote: > > On Thu, 2024-08-15 at 14:24 -0700, Andrii Nakryiko wrote: > > > > [...] > > > > > > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > > > } > > > > } > > > > > > > > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ > > > > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) > > > > +{ > > > > + const struct btf_param *params; > > > > + u32 vlen, i, mask; > > > > + > > > > + params = btf_params(meta->func_proto); > > > > + vlen = btf_type_vlen(meta->func_proto); > > > > + mask = 0; > > > > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) > > > > + mask |= BIT(BPF_REG_0); > > > > + for (i = 0; i < vlen; ++i) > > > > + mask |= BIT(BPF_REG_1 + i); > > > Somewhere deep in btf_dump implementation of libbpf, there is a > > > special handling of `<whatever> func(void)` (no args) function as > > > having vlen == 1 and type being VOID (i.e., zero). I don't know if > > > that still can happen, but I believe at some point we could get this > > > vlen==1 and type=VOID for no-args functions. So I wonder if we should > > > handle that here as well, or is it some compiler atavism we can forget > > > about? > > > > > I just checked BTF generated for 'int filelock_init(void)', > > for gcc compiled kernel using latest pahole and func proto looks as follows: > > > > FUNC_PROTO '(anon)' ret_type_id=12 vlen=0 > > > > So I assume this is an atavism. > > Agree, for kernel vmlinux BTF, we should be fine. Right, since we are dealing only with vmlinux BTF special case is not needed. Please let me know if I misunderstand you or Andrii.
diff --git a/include/linux/btf.h b/include/linux/btf.h index cffb43133c68..59ca37300423 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -75,6 +75,7 @@ #define KF_ITER_NEXT (1 << 9) /* kfunc implements BPF iter next method */ #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter destructor */ #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */ +#define KF_NOCSR (1 << 12) /* kfunc follows nocsr calling contract */ /* * Tag marking a kernel function as a kfunc. This is meant to minimize the diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index df3be12096cf..c579f74be3f9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) } } +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */ +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta) +{ + const struct btf_param *params; + u32 vlen, i, mask; + + params = btf_params(meta->func_proto); + vlen = btf_type_vlen(meta->func_proto); + mask = 0; + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type))) + mask |= BIT(BPF_REG_0); + for (i = 0; i < vlen; ++i) + mask |= BIT(BPF_REG_1 + i); + return mask; +} + +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */ +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta) +{ + return false; +} + /* GCC and LLVM define a no_caller_saved_registers function attribute. * This attribute means that function scratches only some of * the caller saved registers defined by ABI. @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env, bpf_jit_inlines_helper_call(call->imm)); } + if (bpf_pseudo_kfunc_call(call)) { + struct bpf_kfunc_call_arg_meta meta; + int err; + + err = fetch_kfunc_meta(env, call, &meta, NULL); + if (err < 0) + /* error would be reported later */ + return; + + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta); + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) && + verifier_inlines_kfunc_call(&meta); + } + if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS) return;
Recognize nocsr patterns around kfunc calls. For example, suppose bpf_cast_to_kern_ctx() follows nocsr contract (which it does, it is rewritten by verifier as "r0 = r1" insn), in such a case, rewrite BPF program below: r2 = 1; *(u64 *)(r10 - 32) = r2; call %[bpf_cast_to_kern_ctx]; r2 = *(u64 *)(r10 - 32); r0 = r2; Removing the spill/fill pair: r2 = 1; call %[bpf_cast_to_kern_ctx]; r0 = r2; Add a KF_NOCSR flag to mark kfuncs that follow nocsr contract. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- include/linux/btf.h | 1 + kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+)