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

Commit Message

Dave Marchevsky Aug. 11, 2023, 8:13 p.m. UTC

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

Signed-off-by: Dave Marchevsky <[e-mail]>
---
 .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
 .../selftests/bpf/progs/task_kfunc_success.c  | 41 +++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Yonghong Song Aug. 13, 2023, 5:43 a.m. UTC | #1

On 8/11/23 1:13 PM, 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
> 
> Signed-off-by: Dave Marchevsky <[e-mail]>
> ---
>   .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
>   .../selftests/bpf/progs/task_kfunc_success.c  | 41 +++++++++++++++++++
>   2 files changed, 42 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..33e1eb88874f 100644
> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> @@ -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,40 @@ 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)) {
> +		/* if verifier's dead code elimination doesn't remove this,
> +		 * verification should fail due to return w/o bpf_task_release
> +		 */
> +		acquired = bpf_task_acquire___two(task, &fake_ctx);
> +		err = 3;
> +		return 0;
> +	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
> +		/* 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
> +		 *
> +		 * Should be removed by dead code elimination similar to ___two
> +		 */
> +		acquired = bpf_task_acquire___three(&fake_ctx);
> +		err = 4;
> +		return 0;
> +	}

The comments for the above 'bpf_task_acquire___two' and 
'bpf_task_acquire___three' a little confusing. For example, for
'bpf_task_acquire___two', libbpf incorrectly made
'bpf_ksym_exists(bpf_task_acquire___two)' non-NULL, hence
dead code elimination cannot happen and verification will
fail due to missing bpf_task_release. But if libbpf correctly
made ''bpf_ksym_exists(bpf_task_acquire___two)' NULL, but
verifier didn't remove dead code, we should be fine.

I think both 'bpf_task_acquire___two' and 'bpf_task_acquire___three'
can use the same comment as in 'bpf_task_acquire___three'.
There is no need to mention dead code elimination which is
not important for this patch set.

> +
> +	if (acquired)
> +		bpf_task_release(acquired);
> +	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)
>   {

Patch

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..33e1eb88874f 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
@@ -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,40 @@  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)) {
+		/* if verifier's dead code elimination doesn't remove this,
+		 * verification should fail due to return w/o bpf_task_release
+		 */
+		acquired = bpf_task_acquire___two(task, &fake_ctx);
+		err = 3;
+		return 0;
+	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
+		/* 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
+		 *
+		 * Should be removed by dead code elimination similar to ___two
+		 */
+		acquired = bpf_task_acquire___three(&fake_ctx);
+		err = 4;
+		return 0;
+	}
+
+	if (acquired)
+		bpf_task_release(acquired);
+	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)
 {