Re: [PATCH bpf-next v9 05/23] selftests/bpf: Add tests for kfunc returning a memory pointer

On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

We add 2 new kfuncs that are following the RET_PTR_TO_MEM capability from the previous commit. Then we test them in selftests: the first tests are testing valid case, and are not failing, and the later ones are actually preventing the program to be loaded because they are wrong.

To work around that, we mark the failing ones as not autoloaded (with SEC("?tc")), and we manually enable them one by one, ensuring the verifier rejects them.

To be able to use bpf_program__set_autoload() from libbpf, we need to use a plain skeleton, not a light-skeleton, and this is why we also change the Makefile to generate both for kfunc_call_test.c

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

changes in v9:

• updated to match upstream (net/bpf/test_run.c id sets is now using flags)

no changes in v8

changes in v7:

• removed stray include/linux/btf.h change

new in v6

net/bpf/test_run.c | 20 +++++ tools/testing/selftests/bpf/Makefile | 5 +- .../selftests/bpf/prog_tests/kfunc_call.c | 48 ++++++++++ .../selftests/bpf/progs/kfunc_call_test.c | 89 +++++++++++++++++++ 4 files changed, 160 insertions(+), 2 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index f16baf977a21..6accd57d4ded 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -606,6 +606,24 @@ noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p) WARN_ON_ONCE(1); }

+static int *__bpf_kfunc_call_test_get_mem(struct prog_test_ref_kfunc *p, const int size) +{

•   if (size > 2 * sizeof(int))
  

•           return NULL;
  

•   return (int *)p;
  

+}

+noinline int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) +{

•   return __bpf_kfunc_call_test_get_mem(p, rdwr_buf_size);
  

+}

+noinline int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) +{

•   return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
  

+}

noinline struct prog_test_ref_kfunc * bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b) { @@ -712,6 +730,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_memb_acquire, KF_ACQUIRE | KF_RET_NULL) BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE) BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE) BTF_ID_FLAGS(func, bpf_kfunc_call_memb1_release, KF_RELEASE) +BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdwr_mem, KF_RET_NULL) +BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdonly_mem, KF_RET_NULL) BTF_ID_FLAGS(func, bpf_kfunc_call_test_kptr_get, KF_ACQUIRE | KF_RET_NULL | KF_KPTR_GET) BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass_ctx) BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass1) diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 8d59ec7f4c2d..0905315ff86d 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -350,11 +350,12 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \ test_subskeleton.skel.h test_subskeleton_lib.skel.h \ test_usdt.skel.h

-LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \ +LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \ test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \ map_ptr_kern.c core_kern.c core_kern_overflow.c # Generate both light skeleton and libbpf skeleton for these -LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c +LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \

•           kfunc_call_test_subprog.c
  

SKEL_BLACKLIST += $$(LSKELS)

test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c index 1edad012fe01..590417d48962 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c @@ -2,6 +2,7 @@ /* Copyright (c) 2021 Facebook */ #include <test_progs.h> #include <network_helpers.h> +#include "kfunc_call_test.skel.h" #include "kfunc_call_test.lskel.h" #include "kfunc_call_test_subprog.skel.h" #include "kfunc_call_test_subprog.lskel.h" @@ -53,10 +54,12 @@ static void test_main(void) prog_fd = skel->progs.kfunc_syscall_test.prog_fd; err = bpf_prog_test_run_opts(prog_fd, &syscall_topts); ASSERT_OK(err, "bpf_prog_test_run(syscall_test)");

•   ASSERT_EQ(syscall_topts.retval, 0, "syscall_test-retval");
  
    prog_fd = skel->progs.kfunc_syscall_test_fail.prog_fd;
    err = bpf_prog_test_run_opts(prog_fd, &syscall_topts);
    ASSERT_ERR(err, "bpf_prog_test_run(syscall_test_fail)");
  

•   ASSERT_EQ(syscall_topts.retval, 0, "syscall_test_fail-retval");
  
    syscall_topts.ctx_in = NULL;
    syscall_topts.ctx_size_in = 0;
  

@@ -147,6 +150,48 @@ static void test_destructive(void) cap_enable_effective(save_caps, NULL); }

+static void test_get_mem(void) +{

•   struct kfunc_call_test *skel;
  

•   int prog_fd, err;
  

•   LIBBPF_OPTS(bpf_test_run_opts, topts,
  

•           .data_in = &pkt_v4,
  

•           .data_size_in = sizeof(pkt_v4),
  

•           .repeat = 1,
  

•   );
  

•   skel = kfunc_call_test__open_and_load();
  

•   if (!ASSERT_OK_PTR(skel, "skel"))
  

•           return;
  

•   prog_fd = bpf_program__fd(skel->progs.kfunc_call_test_get_mem);
  

•   err = bpf_prog_test_run_opts(prog_fd, &topts);
  

•   ASSERT_OK(err, "bpf_prog_test_run(test_get_mem)");
  

•   ASSERT_EQ(topts.retval, 42, "test_get_mem-retval");
  

•   kfunc_call_test__destroy(skel);
  

•   /* start the various failing tests */
  

•   skel = kfunc_call_test__open();
  

•   if (!ASSERT_OK_PTR(skel, "skel"))
  

•           return;
  

•   bpf_program__set_autoload(skel->progs.kfunc_call_test_get_mem_fail1, true);
  

•   err = kfunc_call_test__load(skel);
  

•   ASSERT_ERR(err, "load(kfunc_call_test_get_mem_fail1)");
  

•   kfunc_call_test__destroy(skel);
  

•   skel = kfunc_call_test__open();
  

•   if (!ASSERT_OK_PTR(skel, "skel"))
  

•           return;
  

•   bpf_program__set_autoload(skel->progs.kfunc_call_test_get_mem_fail2, true);
  

•   err = kfunc_call_test__load(skel);
  

•   ASSERT_ERR(err, "load(kfunc_call_test_get_mem_fail2)");
  

We should match the verifier error string. See e.g. how dynptr tests work. Also it would be better to split failure and success tests into separate objects.

•   kfunc_call_test__destroy(skel);
  

+}

void test_kfunc_call(void) { if (test__start_subtest("main")) @@ -160,4 +205,7 @@ void test_kfunc_call(void)

    if (test__start_subtest("destructive"))
            test_destructive();

•   if (test__start_subtest("get_mem"))
  

•           test_get_mem();
  

} diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c index da7ae0ef9100..b4a98d17c2b6 100644 --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c @@ -14,6 +14,8 @@ extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym; extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym; extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym; extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym; +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym; +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;

SEC("tc") int kfunc_call_test2(struct __sk_buff *skb) @@ -128,4 +130,91 @@ int kfunc_syscall_test_fail(struct syscall_test_args *args) return 0; }

+SEC("tc") +int kfunc_call_test_get_mem(struct __sk_buff *skb) +{

•   struct prog_test_ref_kfunc *pt;
  

•   unsigned long s = 0;
  

•   int *p = NULL;
  

•   int ret = 0;
  

•   pt = bpf_kfunc_call_test_acquire(&s);
  

•   if (pt) {
  

•           if (pt->a != 42 || pt->b != 108)
  

•                   ret = -1;
  

No need to test this I think.

•           p = bpf_kfunc_call_test_get_rdwr_mem(pt, 2 * sizeof(int));
  

•           if (p) {
  

•                   p[0] = 42;
  

•                   ret = p[1]; /* 108 */
  

•           } else {
  

•                   ret = -1;
  

•           }
  

•           if (ret >= 0) {
  

•                   p = bpf_kfunc_call_test_get_rdonly_mem(pt, 2 * sizeof(int));
  

•                   if (p)
  

•                           ret = p[0]; /* 42 */
  

•                   else
  

•                           ret = -1;
  

•           }
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   return ret;
  

+}

+SEC("?tc") +int kfunc_call_test_get_mem_fail1(struct __sk_buff *skb) +{

•   struct prog_test_ref_kfunc *pt;
  

•   unsigned long s = 0;
  

•   int *p = NULL;
  

•   int ret = 0;
  

•   pt = bpf_kfunc_call_test_acquire(&s);
  

•   if (pt) {
  

•           if (pt->a != 42 || pt->b != 108)
  

•                   ret = -1;
  

•           p = bpf_kfunc_call_test_get_rdonly_mem(pt, 2 * sizeof(int));
  

•           if (p)
  

•                   p[0] = 42; /* this is a read-only buffer, so -EACCES */
  

•           else
  

•                   ret = -1;
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   return ret;
  

+}

+SEC("?tc") +int kfunc_call_test_get_mem_fail2(struct __sk_buff *skb) +{

•   struct prog_test_ref_kfunc *pt;
  

•   unsigned long s = 0;
  

•   int *p = NULL;
  

•   int ret = 0;
  

•   pt = bpf_kfunc_call_test_acquire(&s);
  

•   if (pt) {
  

•           if (pt->a != 42 || pt->b != 108)
  

•                   ret = -1;
  

•           p = bpf_kfunc_call_test_get_rdwr_mem(pt, 2 * sizeof(int));
  

•           if (p) {
  

•                   p[0] = 42;
  

•                   ret = p[1]; /* 108 */
  

•           } else {
  

•                   ret = -1;
  

•           }
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   if (p)
  

•           ret = p[0]; /* p is not valid anymore */
  

Great that this ref_obj_id transfer is tested. A few more small test cases that come to mind: . oob access to returned ptr_to_mem to ensure size is set correctly. . failure when size is not 'const', since this is not going through check_mem_size_reg. . incorrect acq kfunc type inside kernel so that on use its ret type is not struct ptr, so verifier complains about it.

•   return ret;
  

+}

char _license[] SEC("license") = "GPL";

2.36.1