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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> ---
>
> 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
>



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux