Re: [PATCH bpf-next v6 9/9] bpf: selftests: Add selftests for module kfunc support

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, "Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	Networking <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next v6 9/9] bpf: selftests: Add selftests for module kfunc support
Date: Fri, 1 Oct 2021 15:13:05 -0700	[thread overview]
Message-ID: <CAEf4BzYXFU+o-AKj_JP3_2VzAYHRtkyzO5Wu0BD7W=n9UHxe6w@mail.gmail.com> (raw)
In-Reply-To: <20210930062948.1843919-10-memxor@gmail.com>

On Wed, Sep 29, 2021 at 11:30 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> This adds selftests that tests the success and failure path for modules
> kfuncs (in presence of invalid kfunc calls) for both libbpf and
> gen_loader. It also adds a prog_test kfunc_btf_id_list so that we can
> add module BTF ID set from bpf_testmod.
>
> This also introduces  a couple of test cases to verifier selftests for
> validating whether we get an error or not depending on if invalid kfunc
> call remains after elimination of unreachable instructions.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/btf.h                           |  2 +
>  kernel/bpf/btf.c                              |  2 +
>  net/bpf/test_run.c                            |  5 +-
>  tools/testing/selftests/bpf/Makefile          |  8 ++--
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 23 +++++++++-
>  .../selftests/bpf/prog_tests/ksyms_module.c   | 29 ++++++------
>  .../bpf/prog_tests/ksyms_module_libbpf.c      | 28 +++++++++++
>  .../selftests/bpf/progs/test_ksyms_module.c   | 46 ++++++++++++++-----
>  tools/testing/selftests/bpf/verifier/calls.c  | 23 ++++++++++
>  9 files changed, 135 insertions(+), 31 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
>

[...]

> @@ -243,7 +244,9 @@ BTF_SET_END(test_sk_kfunc_ids)
>
>  bool bpf_prog_test_check_kfunc_call(u32 kfunc_id, struct module *owner)
>  {
> -       return btf_id_set_contains(&test_sk_kfunc_ids, kfunc_id);
> +       if (btf_id_set_contains(&test_sk_kfunc_ids, kfunc_id))
> +               return true;
> +       return __bpf_prog_test_check_kfunc_call(kfunc_id, owner);
>  }
>
>  static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e1ce73be7a5b..df461699932d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -174,6 +174,7 @@ $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_tes
>         $(Q)$(RM) bpf_testmod/bpf_testmod.ko # force re-compilation
>         $(Q)$(MAKE) $(submake_extras) -C bpf_testmod
>         $(Q)cp bpf_testmod/bpf_testmod.ko $@
> +       $(Q)$(RESOLVE_BTFIDS) -b $(VMLINUX_BTF) bpf_testmod.ko

This should be done by kernel Makefiles, which are used to build
bpf_testmod.ko. If this is not happening, something is wrong and let's
try to figure out what.

>
>  $(OUTPUT)/test_stub.o: test_stub.c $(BPFOBJ)
>         $(call msg,CC,,$@)
> @@ -315,8 +316,9 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h               \
>                 linked_vars.skel.h linked_maps.skel.h
>
>  LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
> -       test_ksyms_module.c test_ringbuf.c atomics.c trace_printk.c \
> -       trace_vprintk.c
> +       test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c
> +# Generate both light skeleton and libbpf skeleton for these
> +LSKELS_EXTRA := test_ksyms_module.c
>  SKEL_BLACKLIST += $$(LSKELS)
>

[...]

> +#define X_0(x)
> +#define X_1(x) x X_0(x)
> +#define X_2(x) x X_1(x)
> +#define X_3(x) x X_2(x)
> +#define X_4(x) x X_3(x)
> +#define X_5(x) x X_4(x)
> +#define X_6(x) x X_5(x)
> +#define X_7(x) x X_6(x)
> +#define X_8(x) x X_7(x)
> +#define X_9(x) x X_8(x)
> +#define X_10(x) x X_9(x)
> +#define REPEAT_256(Y) X_2(X_10(X_10(Y))) X_5(X_10(Y)) X_6(Y)

this is impressive, I can even sort of read it :)

> +
>  extern const int bpf_testmod_ksym_percpu __ksym;
> +extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +extern void bpf_testmod_invalid_mod_kfunc(void) __ksym __weak;
>
> -int out_mod_ksym_global = 0;
> -bool triggered = false;
> +int out_bpf_testmod_ksym = 0;
> +const volatile int x = 0;
>
> -SEC("raw_tp/sys_enter")
> -int handler(const void *ctx)
> +SEC("tc")

Did you switch to tc because kfuncs are not allowed from raw_tp
programs? Or is there some other reason?

> +int load(struct __sk_buff *skb)
>  {
> -       int *val;
> -       __u32 cpu;
> -
> -       val = (int *)bpf_this_cpu_ptr(&bpf_testmod_ksym_percpu);
> -       out_mod_ksym_global = *val;
> -       triggered = true;
> +       /* This will be kept by clang, but removed by verifier. Since it is
> +        * marked as __weak, libbpf and gen_loader don't error out if BTF ID
> +        * is not found for it, instead imm and off is set to 0 for it.
> +        */
> +       if (x)
> +               bpf_testmod_invalid_mod_kfunc();
> +       bpf_testmod_test_mod_kfunc(42);
> +       out_bpf_testmod_ksym = *(int *)bpf_this_cpu_ptr(&bpf_testmod_ksym_percpu);
> +       return 0;
> +}
>

[...]