Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[David Vernet <void@manifault.com>]

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@fb.com/
>   * 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@fb.com>
> ---
>  .../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?

> @@ -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?

> +	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
> 
>