| Message ID | 20221206145936.922196-2-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5b481acab4ce017fda8166fa9428511da41109e5 |
| Delegated to: | BPF |
| Headers | show |
| Series | HID: bpf: remove the need for ALLOW_ERROR_INJECTION and Kconfig fixes | expand |
On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > The current way of expressing that a non-bpf kernel component is willing > to accept that bpf programs can be attached to it and that they can change > the return value is to abuse ALLOW_ERROR_INJECTION. > This is debated in the link below, and the result is that it is not a > reasonable thing to do. > > Reuse the kfunc declaration structure to also tag the kernel functions > we want to be fmodret. This way we can control from any subsystem which > functions are being modified by bpf without touching the verifier. > > > Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/ > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> BPF CI couldn't do its job because of a merge conflict. CI only tries to apply the whole series. But I tested the patch 1 manually. Everything is green on x86-64 and the patch looks good. Acked-by: Alexei Starovoitov <ast@kernel.org> Please send the set during the merge window. If not we can take just this patch, since the series from Viktor Malik would need this patch too.
On Tue, Dec 6, 2022 at 9:48 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > The current way of expressing that a non-bpf kernel component is willing > > to accept that bpf programs can be attached to it and that they can change > > the return value is to abuse ALLOW_ERROR_INJECTION. > > This is debated in the link below, and the result is that it is not a > > reasonable thing to do. > > > > Reuse the kfunc declaration structure to also tag the kernel functions > > we want to be fmodret. This way we can control from any subsystem which > > functions are being modified by bpf without touching the verifier. > > > > > > Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/ > > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > BPF CI couldn't do its job because of a merge conflict. > CI only tries to apply the whole series. > But I tested the patch 1 manually. > Everything is green on x86-64 and the patch looks good. > > Acked-by: Alexei Starovoitov <ast@kernel.org> > > Please send the set during the merge window. > If not we can take just this patch, > since the series from Viktor Malik would need this patch too. > Thanks a lot for the quick review/tests Alexei. I have now taken this patch and the next into the hid tree. I actually took this patch through a branch attached to our hid.git master branch so compared to Linus, it only has this one patch. I also tagged (and signed) that very same branch with "for-alexei-2022120701" in case you also want to bring this one in through the bpf tree too. Full path is at https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/tree/?h=for-alexei-2022120701 Cheers, Benjamin
On Wed, Dec 7, 2022 at 6:57 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Dec 6, 2022 at 9:48 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > The current way of expressing that a non-bpf kernel component is willing > > > to accept that bpf programs can be attached to it and that they can change > > > the return value is to abuse ALLOW_ERROR_INJECTION. > > > This is debated in the link below, and the result is that it is not a > > > reasonable thing to do. > > > > > > Reuse the kfunc declaration structure to also tag the kernel functions > > > we want to be fmodret. This way we can control from any subsystem which > > > functions are being modified by bpf without touching the verifier. > > > > > > > > > Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/ > > > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > BPF CI couldn't do its job because of a merge conflict. > > CI only tries to apply the whole series. > > But I tested the patch 1 manually. > > Everything is green on x86-64 and the patch looks good. > > > > Acked-by: Alexei Starovoitov <ast@kernel.org> > > > > Please send the set during the merge window. > > If not we can take just this patch, > > since the series from Viktor Malik would need this patch too. > > > > Thanks a lot for the quick review/tests Alexei. > > I have now taken this patch and the next into the hid tree. > > I actually took this patch through a branch attached to our hid.git > master branch so compared to Linus, it only has this one patch. I also > tagged (and signed) that very same branch with "for-alexei-2022120701" > in case you also want to bring this one in through the bpf tree too. I didn't find such a branch in your tree, but found that tag and pulled into bpf-next. Thanks!
diff --git a/include/linux/btf.h b/include/linux/btf.h index f9aababc5d78..6a0808e7845c 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -412,8 +412,10 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog); u32 *btf_kfunc_id_set_contains(const struct btf *btf, enum bpf_prog_type prog_type, u32 kfunc_btf_id); +u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id); int register_btf_kfunc_id_set(enum bpf_prog_type prog_type, const struct btf_kfunc_id_set *s); +int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset); s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id); int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_cnt, struct module *owner); diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 35c07afac924..c1df506293e4 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -204,6 +204,7 @@ enum btf_kfunc_hook { BTF_KFUNC_HOOK_STRUCT_OPS, BTF_KFUNC_HOOK_TRACING, BTF_KFUNC_HOOK_SYSCALL, + BTF_KFUNC_HOOK_FMODRET, BTF_KFUNC_HOOK_MAX, }; @@ -7448,11 +7449,14 @@ u32 *btf_kfunc_id_set_contains(const struct btf *btf, return __btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id); } -/* This function must be invoked only from initcalls/module init functions */ -int register_btf_kfunc_id_set(enum bpf_prog_type prog_type, - const struct btf_kfunc_id_set *kset) +u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id) +{ + return __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_FMODRET, kfunc_btf_id); +} + +static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, + const struct btf_kfunc_id_set *kset) { - enum btf_kfunc_hook hook; struct btf *btf; int ret; @@ -7471,13 +7475,29 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type, if (IS_ERR(btf)) return PTR_ERR(btf); - hook = bpf_prog_type_to_kfunc_hook(prog_type); ret = btf_populate_kfunc_set(btf, hook, kset->set); btf_put(btf); return ret; } + +/* This function must be invoked only from initcalls/module init functions */ +int register_btf_kfunc_id_set(enum bpf_prog_type prog_type, + const struct btf_kfunc_id_set *kset) +{ + enum btf_kfunc_hook hook; + + hook = bpf_prog_type_to_kfunc_hook(prog_type); + return __register_btf_kfunc_id_set(hook, kset); +} EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set); +/* This function must be invoked only from initcalls/module init functions */ +int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset) +{ + return __register_btf_kfunc_id_set(BTF_KFUNC_HOOK_FMODRET, kset); +} +EXPORT_SYMBOL_GPL(register_btf_fmodret_id_set); + s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id) { struct btf_id_dtor_kfunc_tab *tab = btf->dtor_kfunc_tab; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 225666307bba..ec2e6ec7309e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -15021,12 +15021,22 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, ret = -EINVAL; switch (prog->type) { case BPF_PROG_TYPE_TRACING: - /* fentry/fexit/fmod_ret progs can be sleepable only if they are + + /* fentry/fexit/fmod_ret progs can be sleepable if they are * attached to ALLOW_ERROR_INJECTION and are not in denylist. */ if (!check_non_sleepable_error_inject(btf_id) && within_error_injection_list(addr)) ret = 0; + /* fentry/fexit/fmod_ret progs can also be sleepable if they are + * in the fmodret id set with the KF_SLEEPABLE flag. + */ + else { + u32 *flags = btf_kfunc_is_modify_return(btf, btf_id); + + if (flags && (*flags & KF_SLEEPABLE)) + ret = 0; + } break; case BPF_PROG_TYPE_LSM: /* LSM progs check that they are attached to bpf_lsm_*() funcs. @@ -15047,7 +15057,10 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, bpf_log(log, "can't modify return codes of BPF programs\n"); return -EINVAL; } - ret = check_attach_modify_return(addr, tname); + ret = -EINVAL; + if (btf_kfunc_is_modify_return(btf, btf_id) || + !check_attach_modify_return(addr, tname)) + ret = 0; if (ret) { bpf_log(log, "%s() is not modifiable\n", tname); return ret; diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 13d578ce2a09..5079fb089b5f 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -489,7 +489,6 @@ int noinline bpf_fentry_test1(int a) return a + 1; } EXPORT_SYMBOL_GPL(bpf_fentry_test1); -ALLOW_ERROR_INJECTION(bpf_fentry_test1, ERRNO); int noinline bpf_fentry_test2(int a, u64 b) { @@ -733,7 +732,15 @@ noinline void bpf_kfunc_call_test_destructive(void) __diag_pop(); -ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO); +BTF_SET8_START(bpf_test_modify_return_ids) +BTF_ID_FLAGS(func, bpf_modify_return_test) +BTF_ID_FLAGS(func, bpf_fentry_test1, KF_SLEEPABLE) +BTF_SET8_END(bpf_test_modify_return_ids) + +static const struct btf_kfunc_id_set bpf_test_modify_return_set = { + .owner = THIS_MODULE, + .set = &bpf_test_modify_return_ids, +}; BTF_SET8_START(test_sk_check_kfunc_ids) BTF_ID_FLAGS(func, bpf_kfunc_call_test1) @@ -1668,7 +1675,8 @@ static int __init bpf_prog_test_run_init(void) }; int ret; - ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set); + ret = register_btf_fmodret_id_set(&bpf_test_modify_return_set); + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set); ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set); ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_prog_test_kfunc_set); return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
The current way of expressing that a non-bpf kernel component is willing to accept that bpf programs can be attached to it and that they can change the return value is to abuse ALLOW_ERROR_INJECTION. This is debated in the link below, and the result is that it is not a reasonable thing to do. Reuse the kfunc declaration structure to also tag the kernel functions we want to be fmodret. This way we can control from any subsystem which functions are being modified by bpf without touching the verifier. Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/ Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- changes in v3: - renamed btf_kern_func_is_modify_return() into btf_kfunc_is_modify_return() - removed copy/paste in register_btf_fmodret_id_set() changes in v2: - compared to https://lore.kernel.org/all/c9912b24-f611-29b8-28e1-5e8be0d5ad41@redhat.com/ - make use of register_btf_kfunc_id_set() so we don't have to include hid-bpf.h in bpf core. --- include/linux/btf.h | 2 ++ kernel/bpf/btf.c | 30 +++++++++++++++++++++++++----- kernel/bpf/verifier.c | 17 +++++++++++++++-- net/bpf/test_run.c | 14 +++++++++++--- 4 files changed, 53 insertions(+), 10 deletions(-)