On 8/16/23 1:44 PM, David Vernet wrote: > On Wed, Aug 16, 2023 at 09:58:13AM -0700, Dave Marchevsky wrote: >> This patch adds selftests that exercise kfunc flavor relocation >> functionality added in the previous patch. The actual kfunc defined in >> kernel/bpf/helpers.c is >> >> struct task_struct *bpf_task_acquire(struct task_struct *p) >> >> The following relocation behaviors are checked: >> >> struct task_struct *bpf_task_acquire___one(struct task_struct *name) >> * Should succeed despite differing param name >> >> struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) >> * Should fail because there is no two-param bpf_task_acquire >> >> struct task_struct *bpf_task_acquire___three(void *ctx) >> * Should fail because, despite vmlinux's bpf_task_acquire having one param, >> the types don't match >> >> Changelog: >> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@xxxxxx/ >> * Change comment on bpf_task_acquire___two to more accurately reflect >> that it fails in same codepath as bpf_task_acquire___three, and to >> not mention dead code elimination as thats an implementation detail >> (Yonghong) >> >> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx> >> --- >> .../selftests/bpf/prog_tests/task_kfunc.c | 1 + >> .../selftests/bpf/progs/task_kfunc_success.c | 37 +++++++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c >> index 740d5f644b40..99abb0350154 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c >> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c >> @@ -79,6 +79,7 @@ static const char * const success_tests[] = { >> "test_task_from_pid_current", >> "test_task_from_pid_invalid", >> "task_kfunc_acquire_trusted_walked", >> + "test_task_kfunc_flavor_relo", >> }; >> >> void test_task_kfunc(void) >> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c >> index b09371bba204..ffbe3ff72639 100644 >> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c >> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c > > Do you think it's worth it to also add a failure case for if there's no > correct bpf_taks_acquire___one(), to verify e.g. that we can't resolve > bpf_task_acquire___three(void *ctx) __ksym __weak? > IIUC you're asking about whether it's possible to fail loading the program entirely if _none_ of the three variants resolve successfully? If so, I sent out a response to another email in this round of your comments that should address it. >> @@ -18,6 +18,13 @@ int err, pid; >> */ >> >> struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; >> + >> +struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak; >> +/* The two-param bpf_task_acquire doesn't exist */ >> +struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak; >> +/* Incorrect type for first param */ >> +struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak; >> + >> void invalid_kfunc(void) __ksym __weak; >> void bpf_testmod_test_mod_kfunc(int i) __ksym __weak; >> >> @@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task) >> return 0; >> } >> >> +SEC("tp_btf/task_newtask") >> +int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags) >> +{ >> + struct task_struct *acquired = NULL; >> + int fake_ctx = 42; >> + >> + if (bpf_ksym_exists(bpf_task_acquire___one)) { >> + acquired = bpf_task_acquire___one(task); >> + } else if (bpf_ksym_exists(bpf_task_acquire___two)) { >> + /* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id >> + * call will find vmlinux's bpf_task_acquire, but subsequent >> + * bpf_core_types_are_compat will fail >> + */ >> + acquired = bpf_task_acquire___two(task, &fake_ctx); >> + err = 3; >> + return 0; >> + } else if (bpf_ksym_exists(bpf_task_acquire___three)) { >> + /* bpf_core_types_are_compat will fail similarly to above case */ >> + acquired = bpf_task_acquire___three(&fake_ctx); >> + err = 4; >> + return 0; >> + } >> + >> + if (acquired) >> + bpf_task_release(acquired); > > Might be slightly simpler to do the release + return immediately in the > bpf_task_acquire___one branch, and then to just do the following here > without the if / else: > > err = 5; > return 0; > > What do you think? > Eh, I like this form more because it's easier to visually distinguish that the bpf_task_acquire___one case above is not a 'failure' case and should successfully resolve, whereas the other two bail out early. >> + else >> + err = 5; >> + return 0; >> +} >> + >> SEC("tp_btf/task_newtask") >> int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags) >> { >> -- >> 2.34.1 >> >>
