| Message ID | 20240221-hid-bpf-sleepable-v3-8-1fb378ca6301@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sleepable bpf_timer (was: allow HID-BPF to do device IOs) | expand |
On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote: > This is still a WIP, but I think this can be dropped as we never > get to this instruction. So what should we do here? As Alexei replied in a separate sub-thread you probably want this for sleepable timers. Here is full source code block: if (insn->imm == BPF_FUNC_timer_set_callback || is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) { ... struct bpf_insn ld_addrs[2] = { BPF_LD_IMM64(BPF_REG_3, (long)prog->aux), }; insn_buf[0] = ld_addrs[0]; insn_buf[1] = ld_addrs[1]; insn_buf[2] = *insn; cnt = 3; new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); ... } Effectively, it sets up third function call parameter (R3) for timer_set_callback() to be prog->aux. E.g. before bpf_patch_insn_data(): r1 = ... timer ... r2 = ... callback address ... call timer_set_callback After bpf_patch_insn_data(): r1 = ... timer ... r2 = ... callback address ... r3 = prog->aux ll call timer_set_callback This way it won't be necessary to walk stack in search for ctx.aux in bpf_timer_set_sleepable_cb().
On Feb 23 2024, Eduard Zingerman wrote: > On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote: > > This is still a WIP, but I think this can be dropped as we never > > get to this instruction. So what should we do here? > > As Alexei replied in a separate sub-thread you probably want this > for sleepable timers. Here is full source code block: > > if (insn->imm == BPF_FUNC_timer_set_callback || > is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) { > ... > struct bpf_insn ld_addrs[2] = { > BPF_LD_IMM64(BPF_REG_3, (long)prog->aux), > }; > > insn_buf[0] = ld_addrs[0]; > insn_buf[1] = ld_addrs[1]; > insn_buf[2] = *insn; > cnt = 3; > > new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); > ... > } > > Effectively, it sets up third function call parameter (R3) > for timer_set_callback() to be prog->aux. > E.g. before bpf_patch_insn_data(): > > r1 = ... timer ... > r2 = ... callback address ... > call timer_set_callback > > After bpf_patch_insn_data(): > > r1 = ... timer ... > r2 = ... callback address ... > r3 = prog->aux ll > call timer_set_callback > > This way it won't be necessary to walk stack in search for ctx.aux > in bpf_timer_set_sleepable_cb(). Hmm, I must still be missing a piece of the puzzle: if I declare bpf_timer_set_sleepable_cb() to take a third "aux" argument, given that it is declared as kfunc, I also must declare it in my bpf program, or I get the following: # libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151] And if I declare it, then I don't know what to pass, given that this is purely added by the verifier: 43: (85) call bpf_timer_set_sleepable_cb#18152 arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar Maybe I should teach the verifier that this kfunc only takes 2 arguments, and the third one is virtual, but that also means that when the kfunc definitions are to be included in vmlinux.h, they would also have this special case. (I just tried with a blank u64 instead of the struct bpf_prog_aux*, but it crashes with KASAN complaining). Cheers, Benjamin
On Tue, 2024-02-27 at 17:18 +0100, Benjamin Tissoires wrote: [...] > Hmm, I must still be missing a piece of the puzzle: > if I declare bpf_timer_set_sleepable_cb() to take a third "aux" > argument, given that it is declared as kfunc, I also must declare it in > my bpf program, or I get the following: > > # libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151] > > And if I declare it, then I don't know what to pass, given that this is > purely added by the verifier: > > 43: (85) call bpf_timer_set_sleepable_cb#18152 > arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar Right, something has to be done about number of arguments and we don't have a convenient mechanism for this afaik. The simplest way would be to have two kfuncs: - one with 2 arguments, used form bpf program; - another with 3 arguments, used at runtime; - replace former by latter during rewrite. > Maybe I should teach the verifier that this kfunc only takes 2 > arguments, and the third one is virtual, but that also means that when > the kfunc definitions are to be included in vmlinux.h, they would also > have this special case. It might be a somewhat generic mechanism, e.g. btf_decl_tag("hidden") for kfunc parameter. imho, having two kfuncs is less hacky. > (I just tried with a blank u64 instead of the struct bpf_prog_aux*, but > it crashes with KASAN complaining). For my understanding: - you added a 3rd param (void *) to kfunc; - passed it as zero in BPF program; - applied the above rewrite, so that r3 equals to prog->aux; - and now KASAN complains, right? Could you please provide more details on what exactly it complains about?
On Tue, Feb 27, 2024 at 5:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2024-02-27 at 17:18 +0100, Benjamin Tissoires wrote: > [...] > > > Hmm, I must still be missing a piece of the puzzle: > > if I declare bpf_timer_set_sleepable_cb() to take a third "aux" > > argument, given that it is declared as kfunc, I also must declare it in > > my bpf program, or I get the following: > > > > # libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151] > > > > And if I declare it, then I don't know what to pass, given that this is > > purely added by the verifier: > > > > 43: (85) call bpf_timer_set_sleepable_cb#18152 > > arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar > > Right, something has to be done about number of arguments and we don't > have a convenient mechanism for this afaik. > > The simplest way would be to have two kfuncs: > - one with 2 arguments, used form bpf program; > - another with 3 arguments, used at runtime; > - replace former by latter during rewrite. It's hacky but seems interesting enough to be tested :) > > > Maybe I should teach the verifier that this kfunc only takes 2 > > arguments, and the third one is virtual, but that also means that when > > the kfunc definitions are to be included in vmlinux.h, they would also > > have this special case. > > It might be a somewhat generic mechanism, e.g. btf_decl_tag("hidden") > for kfunc parameter. We also could use the suffix (like __uninit, __k, etc...), but it might introduce more headaches than the 2 kfuncs you are proposing. > > imho, having two kfuncs is less hacky. > > > (I just tried with a blank u64 instead of the struct bpf_prog_aux*, but > > it crashes with KASAN complaining). > > For my understanding: > - you added a 3rd param (void *) to kfunc; it was struct bpf_prog_aux *, but yes > - passed it as zero in BPF program; > - applied the above rewrite, so that r3 equals to prog->aux; > - and now KASAN complains, right? yep, but see below > > Could you please provide more details on what exactly it complains about? > Well, there is a simple reason: that code is never reached because, in that function, there is a `if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)` above that unconditionally terminates with a `continue`. So basically this part of the code is never hit. I'll include that new third argument and the dual kfunc call in fixup_kfunc_call() and report if it works from here. Cheers, Benjamin
On Tue, Feb 27, 2024 at 8:51 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Feb 27, 2024 at 5:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Tue, 2024-02-27 at 17:18 +0100, Benjamin Tissoires wrote: > > [...] > > > > > Hmm, I must still be missing a piece of the puzzle: > > > if I declare bpf_timer_set_sleepable_cb() to take a third "aux" > > > argument, given that it is declared as kfunc, I also must declare it in > > > my bpf program, or I get the following: > > > > > > # libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151] > > > > > > And if I declare it, then I don't know what to pass, given that this is > > > purely added by the verifier: > > > > > > 43: (85) call bpf_timer_set_sleepable_cb#18152 > > > arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar > > > > Right, something has to be done about number of arguments and we don't > > have a convenient mechanism for this afaik. > > > > The simplest way would be to have two kfuncs: > > - one with 2 arguments, used form bpf program; > > - another with 3 arguments, used at runtime; > > - replace former by latter during rewrite. > > It's hacky but seems interesting enough to be tested :) Too hacky imo :) Let's follow the existing pattern. See: __bpf_kfunc void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign) __ign suffix tells the verifier to ignore it. Then we do: #define bpf_obj_new(type) \ ((type *)bpf_obj_new_impl(bpf_core_type_id_local(type), NULL)) and later the verifier replaces arg2 with the correct pointer. > We also could use the suffix (like __uninit, __k, etc...), but it > might introduce more headaches than the 2 kfuncs you are proposing. Only one kfunc pls. Let's not make it more complex than necessary. We cannot easily add a suffix to tell libbpf to ignore that arg, since bpf_core_types_are_compat() compares types and there are no argument names in the types. So it will be a significant surgery for libbpf to find the arg name in vmlinux BTF and strcmp the suffix. > > > > > Could you please provide more details on what exactly it complains about? > > > > Well, there is a simple reason: that code is never reached because, in > that function, there is a `if (insn->src_reg == > BPF_PSEUDO_KFUNC_CALL)` above that unconditionally terminates with a > `continue`. So basically this part of the code is never hit. > > I'll include that new third argument and the dual kfunc call in > fixup_kfunc_call() and report if it works from here. Something is wrong. fixup_kfunc_call() can rewrite args with whatever it wants. Are you sure you've added bpf_timer_set_sleepable_cb to special_kfunc_list ?
On Wed, Feb 28, 2024 at 2:49 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Feb 27, 2024 at 8:51 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Tue, Feb 27, 2024 at 5:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Tue, 2024-02-27 at 17:18 +0100, Benjamin Tissoires wrote: > > > [...] > > > > > > > Hmm, I must still be missing a piece of the puzzle: > > > > if I declare bpf_timer_set_sleepable_cb() to take a third "aux" > > > > argument, given that it is declared as kfunc, I also must declare it in > > > > my bpf program, or I get the following: > > > > > > > > # libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151] > > > > > > > > And if I declare it, then I don't know what to pass, given that this is > > > > purely added by the verifier: > > > > > > > > 43: (85) call bpf_timer_set_sleepable_cb#18152 > > > > arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar > > > > > > Right, something has to be done about number of arguments and we don't > > > have a convenient mechanism for this afaik. > > > > > > The simplest way would be to have two kfuncs: > > > - one with 2 arguments, used form bpf program; > > > - another with 3 arguments, used at runtime; > > > - replace former by latter during rewrite. > > > > It's hacky but seems interesting enough to be tested :) > > Too hacky imo :) > > Let's follow the existing pattern. > See: > __bpf_kfunc void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign) > > __ign suffix tells the verifier to ignore it. > > Then we do: > #define bpf_obj_new(type) \ > ((type *)bpf_obj_new_impl(bpf_core_type_id_local(type), NULL)) > > and later the verifier replaces arg2 with the correct pointer. \o/ Thanks, it works :) > > > We also could use the suffix (like __uninit, __k, etc...), but it > > might introduce more headaches than the 2 kfuncs you are proposing. > > Only one kfunc pls. Let's not make it more complex than necessary. > > We cannot easily add a suffix to tell libbpf to ignore that arg, > since bpf_core_types_are_compat() compares types and there are > no argument names in the types. > So it will be a significant surgery for libbpf to find the arg name > in vmlinux BTF and strcmp the suffix. Yeah, I guessed so. Having a single #define is fine, especially given that there are already a lot of them for the same purpose. > > > > > > > > > Could you please provide more details on what exactly it complains about? > > > > > > > Well, there is a simple reason: that code is never reached because, in > > that function, there is a `if (insn->src_reg == > > BPF_PSEUDO_KFUNC_CALL)` above that unconditionally terminates with a > > `continue`. So basically this part of the code is never hit. > > > > I'll include that new third argument and the dual kfunc call in > > fixup_kfunc_call() and report if it works from here. > > Something is wrong. fixup_kfunc_call() can rewrite args with whatever > it wants. > Are you sure you've added bpf_timer_set_sleepable_cb to special_kfunc_list ? > Yeah, but as I mentioned, I wasn't hacking at the correct place. I was not doing the changes in the fixup_kfunc_call() but in the helper processing, so that path was not hit. But with your instructions it works. I have a couple of changes to do and the selftests to add and the series will be ready. Cheers, Benjamin
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 4766c43606c4..8a9f268c4ee2 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19720,7 +19720,8 @@ static int do_misc_fixups(struct bpf_verifier_env *env) continue; } - if (insn->imm == BPF_FUNC_timer_set_callback) { + if (insn->imm == BPF_FUNC_timer_set_callback || + is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) { /* The verifier will process callback_fn as many times as necessary * with different maps and the register states prepared by * set_timer_callback_state will be accurate.
This is still a WIP, but I think this can be dropped as we never get to this instruction. So what should we do here? Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- kernel/bpf/verifier.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)