Global subprogs are currently not allowed to return void. Adjust verifier logic to allow global functions with a void return type. Signed-off-by: Emil Tsalapatis --- kernel/bpf/btf.c | 7 ++- kernel/bpf/verifier.c | 59 +++++++++++++++++-- .../selftests/bpf/progs/exceptions_fail.c | 2 +- .../selftests/bpf/progs/test_global_func7.c | 2 +- 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7708958e3fb8..51f8d3dece5d 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -7836,15 +7836,16 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog) tname, nargs, MAX_BPF_FUNC_REG_ARGS); return -EINVAL; } - /* check that function returns int, exception cb also requires this */ + /* check that function is void or returns int, exception cb also requires this */ t = btf_type_by_id(btf, t->type); while (btf_type_is_modifier(t)) t = btf_type_by_id(btf, t->type); - if (!btf_type_is_int(t) && !btf_is_any_enum(t)) { + if (!btf_type_is_void(t) && !btf_type_is_int(t) && !btf_is_any_enum(t)) { if (!is_global) return -EINVAL; bpf_log(log, - "Global function %s() doesn't return scalar. Only those are supported.\n", + "Global function %s() return value not void or scalar. " + "Only those are supported.\n", tname); return -EINVAL; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0162f946032f..e997c3776fa7 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -444,6 +444,29 @@ static bool subprog_is_global(const struct bpf_verifier_env *env, int subprog) return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL; } +static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog) +{ + const struct btf_type *type, *func, *func_proto; + const struct btf *btf = env->prog->aux->btf; + u32 btf_id; + + btf_id = env->prog->aux->func_info[subprog].type_id; + + func = btf_type_by_id(btf, btf_id); + if (verifier_bug_if(!func, env, "btf_id %u not found", btf_id)) + return false; + + func_proto = btf_type_by_id(btf, func->type); + if (verifier_bug_if(!func_proto, env, "btf_id %u not found", func->type)) + return false; + + type = btf_type_skip_modifiers(btf, func_proto->type, NULL); + if (verifier_bug_if(!type, env, "btf_id %u not found", func_proto->type)) + return false; + + return btf_type_is_void(type); +} + static const char *subprog_name(const struct bpf_verifier_env *env, int subprog) { struct bpf_func_info *info; @@ -10840,9 +10863,11 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, subprog_aux(env, subprog)->called = true; clear_caller_saved_regs(env, caller->regs); - /* All global functions return a 64-bit SCALAR_VALUE */ - mark_reg_unknown(env, caller->regs, BPF_REG_0); - caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; + /* All non-void global functions return a 64-bit SCALAR_VALUE. */ + if (!subprog_returns_void(env, subprog)) { + mark_reg_unknown(env, caller->regs, BPF_REG_0); + caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; + } /* continue with next insn after call */ return 0; @@ -17812,6 +17837,16 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char /* LSM and struct_ops func-ptr's return type could be "void" */ if (!is_subprog || frame->in_exception_callback_fn) { + + /* + * If the actual program is an extension, let it + * return void - attaching will succeed only if the + * program being replaced also returns void, and since + * it has passed verification its actual type doesn't matter. + */ + if (env->prog->type == BPF_PROG_TYPE_EXT && subprog_returns_void(env, frame->subprogno)) + return 0; + switch (prog_type) { case BPF_PROG_TYPE_LSM: if (prog->expected_attach_type == BPF_LSM_CGROUP) @@ -17841,6 +17876,10 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char default: break; } + } else { + /* If this is a void global subprog, there is no return value. */ + if (subprog_is_global(env, frame->subprogno) && subprog_returns_void(env, frame->subprogno)) + return 0; } /* eBPF calling convention is such that R0 is used @@ -24452,10 +24491,18 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog) if (subprog_is_exc_cb(env, subprog)) { state->frame[0]->in_exception_callback_fn = true; - /* We have already ensured that the callback returns an integer, just - * like all global subprogs. We need to determine it only has a single - * scalar argument. + + /* + * Global functions are scalar or void, make sure + * we return a scalar. */ + if (subprog_returns_void(env, subprog)) { + verbose(env, "exception cb cannot return void\n"); + ret = -EINVAL; + goto out; + } + + /* Also ensure the callback only has a single scalar argument. */ if (sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_ANYTHING) { verbose(env, "exception cb only supports single integer argument\n"); ret = -EINVAL; diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c index 8a0fdff89927..d28ecc4ee2d0 100644 --- a/tools/testing/selftests/bpf/progs/exceptions_fail.c +++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c @@ -51,7 +51,7 @@ __noinline int exception_cb_ok_arg_small(int a) SEC("?tc") __exception_cb(exception_cb_bad_ret_type) -__failure __msg("Global function exception_cb_bad_ret_type() doesn't return scalar.") +__failure __msg("Global function exception_cb_bad_ret_type() return value not void or scalar.") int reject_exception_cb_type_1(struct __sk_buff *ctx) { bpf_throw(0); diff --git a/tools/testing/selftests/bpf/progs/test_global_func7.c b/tools/testing/selftests/bpf/progs/test_global_func7.c index f182febfde3c..9e59625c1c92 100644 --- a/tools/testing/selftests/bpf/progs/test_global_func7.c +++ b/tools/testing/selftests/bpf/progs/test_global_func7.c @@ -12,7 +12,7 @@ void foo(struct __sk_buff *skb) } SEC("tc") -__failure __msg("foo() doesn't return scalar") +__success int global_func7(struct __sk_buff *skb) { foo(skb); -- 2.49.0 Add additional testing for void global functions. The tests ensure that calls to void global functions properly keep R0 invalid. Also make sure that exception callbacks still require a return value. Signed-off-by: Emil Tsalapatis --- .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 24 +++++++++++++++++++ .../bpf/prog_tests/test_global_funcs.c | 2 ++ .../selftests/bpf/progs/exceptions_fail.c | 19 ++++++++++++--- .../bpf/progs/freplace_int_with_void.c | 14 +++++++++++ .../selftests/bpf/progs/freplace_void.c | 14 +++++++++++ .../selftests/bpf/progs/test_global_func18.c | 23 ++++++++++++++++++ 6 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/freplace_int_with_void.c create mode 100644 tools/testing/selftests/bpf/progs/freplace_void.c create mode 100644 tools/testing/selftests/bpf/progs/test_global_func18.c diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c index f29fc789c14b..23d933f1aec6 100644 --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c @@ -347,6 +347,17 @@ static void test_func_sockmap_update(void) prog_name, false, NULL); } +static void test_func_replace_void(void) +{ + const char *prog_name[] = { + "freplace/foo", + }; + test_fexit_bpf2bpf_common("./freplace_void.bpf.o", + "./test_global_func7.bpf.o", + ARRAY_SIZE(prog_name), + prog_name, false, NULL); +} + static void test_obj_load_failure_common(const char *obj_file, const char *target_obj_file, const char *exp_msg) @@ -432,6 +443,15 @@ static void test_func_replace_global_func(void) prog_name, false, NULL); } +static void test_func_replace_int_with_void(void) +{ + /* Make sure we can't freplace with the wrong type */ + test_obj_load_failure_common("freplace_int_with_void.bpf.o", + "./test_global_func2.bpf.o", + "Return type UNKNOWN of test_freplace_int_with_void()" + " doesn't match type INT of global_func2()"); +} + static int find_prog_btf_id(const char *name, __u32 attach_prog_fd) { struct bpf_prog_info info = {}; @@ -597,4 +617,8 @@ void serial_test_fexit_bpf2bpf(void) test_fentry_to_cgroup_bpf(); if (test__start_subtest("func_replace_progmap")) test_func_replace_progmap(); + if (test__start_subtest("freplace_int_with_void")) + test_func_replace_int_with_void(); + if (test__start_subtest("freplace_void")) + test_func_replace_void(); } diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c index e905cbaf6b3d..45854cbcbfcf 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c @@ -18,6 +18,7 @@ #include "test_global_func15.skel.h" #include "test_global_func16.skel.h" #include "test_global_func17.skel.h" +#include "test_global_func18.skel.h" #include "test_global_func_ctx_args.skel.h" #include "bpf/libbpf_internal.h" @@ -155,6 +156,7 @@ void test_test_global_funcs(void) RUN_TESTS(test_global_func15); RUN_TESTS(test_global_func16); RUN_TESTS(test_global_func17); + RUN_TESTS(test_global_func18); RUN_TESTS(test_global_func_ctx_args); if (test__start_subtest("ctx_arg_rewrite")) diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c index d28ecc4ee2d0..691c16001952 100644 --- a/tools/testing/selftests/bpf/progs/exceptions_fail.c +++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c @@ -29,11 +29,15 @@ struct { private(A) struct bpf_spin_lock lock; private(A) struct bpf_rb_root rbtree __contains(foo, node); -__noinline void *exception_cb_bad_ret_type(u64 cookie) +__noinline void *exception_cb_bad_ret_type1(u64 cookie) { return NULL; } +__noinline void exception_cb_bad_ret_type2(u64 cookie) +{ +} + __noinline int exception_cb_bad_arg_0(void) { return 0; @@ -50,8 +54,8 @@ __noinline int exception_cb_ok_arg_small(int a) } SEC("?tc") -__exception_cb(exception_cb_bad_ret_type) -__failure __msg("Global function exception_cb_bad_ret_type() return value not void or scalar.") +__exception_cb(exception_cb_bad_ret_type1) +__failure __msg("Global function exception_cb_bad_ret_type1() return value not void or scalar.") int reject_exception_cb_type_1(struct __sk_buff *ctx) { bpf_throw(0); @@ -85,6 +89,15 @@ int reject_exception_cb_type_4(struct __sk_buff *ctx) return 0; } +SEC("?tc") +__exception_cb(exception_cb_bad_ret_type2) +__failure __msg("exception cb cannot return void") +int reject_exception_cb_type_5(struct __sk_buff *ctx) +{ + bpf_throw(0); + return 0; +} + __noinline static int timer_cb(void *map, int *key, struct bpf_timer *timer) { diff --git a/tools/testing/selftests/bpf/progs/freplace_int_with_void.c b/tools/testing/selftests/bpf/progs/freplace_int_with_void.c new file mode 100644 index 000000000000..8b6f5776cf0f --- /dev/null +++ b/tools/testing/selftests/bpf/progs/freplace_int_with_void.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include +#include + +volatile int data; + +SEC("freplace/global_func2") +void test_freplace_int_with_void(struct __sk_buff *skb) +{ + data = 1; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/freplace_void.c b/tools/testing/selftests/bpf/progs/freplace_void.c new file mode 100644 index 000000000000..76707b29dc6a --- /dev/null +++ b/tools/testing/selftests/bpf/progs/freplace_void.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include + +volatile int data; + +SEC("freplace/foo") +__weak +void test_freplace_void(struct __sk_buff *skb) +{ + data = 1; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/test_global_func18.c b/tools/testing/selftests/bpf/progs/test_global_func18.c new file mode 100644 index 000000000000..3dafb0dc2342 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_global_func18.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include +#include +#include "bpf_misc.h" + +__weak +void foo(void) +{ +} + +SEC("tc") +__failure __msg("!read_ok") +int global_func18(struct __sk_buff *skb) +{ + foo(); + + asm volatile( + "r1 = r0;" + ::: + ); + + return 0; +} -- 2.49.0