| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Sep 2021 17:03:26 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Toke Høiland-Jørgensen <toke@...hat.com>,
Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v4 11/11] bpf: selftests: Add selftests for
module kfunc support
On Tue, Sep 21, 2021 at 4:13 PM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> On Wed, Sep 22, 2021 at 04:30:32AM IST, Andrii Nakryiko wrote:
> > On Mon, Sep 20, 2021 at 7:16 AM Kumar Kartikeya Dwivedi
> > <memxor@...il.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.
> > >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> > > ---
> > > include/linux/btf.h | 2 +
> > > kernel/bpf/btf.c | 2 +
> > > net/bpf/test_run.c | 5 +-
> > > tools/testing/selftests/bpf/Makefile | 5 +-
> > > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 26 ++++++-
> > > .../selftests/bpf/prog_tests/ksyms_module.c | 52 ++++++++++----
> > > .../bpf/prog_tests/ksyms_module_libbpf.c | 44 ++++++++++++
> > > .../selftests/bpf/progs/test_ksyms_module.c | 41 ++++++++---
> > > .../bpf/progs/test_ksyms_module_fail.c | 29 ++++++++
> > > .../progs/test_ksyms_module_fail_toomany.c | 19 +++++
> > > .../bpf/progs/test_ksyms_module_libbpf.c | 71 +++++++++++++++++++
> > > .../bpf/progs/test_ksyms_module_util.h | 48 +++++++++++++
> > > 12 files changed, 317 insertions(+), 27 deletions(-)
> > > create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_module_fail.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_module_fail_toomany.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_module_libbpf.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_module_util.h
> > >
> >
> > [...]
> >
> > > @@ -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_check_prog_test_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 326ea75ce99e..d20ff0563120 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) -s ../../../../vmlinux bpf_testmod.ko
> >
> > $(VMLINUX_BTF) instead of "../../../../vmlinux", it will break
> >
> > >
> > > $(OUTPUT)/test_stub.o: test_stub.c $(BPFOBJ)
> > > $(call msg,CC,,$@)
> > > @@ -315,8 +316,8 @@ 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_ksyms_module.c test_ksyms_module_fail.c test_ksyms_module_fail_toomany.c \
> > > + test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c
> > > SKEL_BLACKLIST += $$(LSKELS)
> > >
> >
> > [...]
> >
> > > diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_module_util.h b/tools/testing/selftests/bpf/progs/test_ksyms_module_util.h
> > > new file mode 100644
> > > index 000000000000..3afa74841ae0
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_ksyms_module_util.h
> > > @@ -0,0 +1,48 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#ifndef __KSYMS_MODULE_UTIL_H__
> > > +#define __KSYMS_MODULE_UTIL_H__
> > > +
> > > +#define __KFUNC_NR_EXP(Y) \
> > > +Y(0) Y(1) Y(2) Y(3) Y(4) Y(5) Y(6) Y(7) Y(8) Y(9) Y(10) Y(11) Y(12) \
> > > +Y(13) Y(14) Y(15) Y(16) Y(17) Y(18) Y(19) Y(20) Y(21) Y(22) Y(23) \
> > > +Y(24) Y(25) Y(26) Y(27) Y(28) Y(29) Y(30) Y(31) Y(32) Y(33) Y(34) \
> > > +Y(35) Y(36) Y(37) Y(38) Y(39) Y(40) Y(41) Y(42) Y(43) Y(44) Y(45) \
> > > +Y(46) Y(47) Y(48) Y(49) Y(50) Y(51) Y(52) Y(53) Y(54) Y(55) Y(56) \
> > > +Y(57) Y(58) Y(59) Y(60) Y(61) Y(62) Y(63) Y(64) Y(65) Y(66) Y(67) \
> > > +Y(68) Y(69) Y(70) Y(71) Y(72) Y(73) Y(74) Y(75) Y(76) Y(77) Y(78) \
> > > +Y(79) Y(80) Y(81) Y(82) Y(83) Y(84) Y(85) Y(86) Y(87) Y(88) Y(89) \
> > > +Y(90) Y(91) Y(92) Y(93) Y(94) Y(95) Y(96) Y(97) Y(98) Y(99) Y(100) \
> > > +Y(101) Y(102) Y(103) Y(104) Y(105) Y(106) Y(107) Y(108) Y(109) Y(110) \
> > > +Y(111) Y(112) Y(113) Y(114) Y(115) Y(116) Y(117) Y(118) Y(119) Y(120) \
> > > +Y(121) Y(122) Y(123) Y(124) Y(125) Y(126) Y(127) Y(128) Y(129) Y(130) \
> > > +Y(131) Y(132) Y(133) Y(134) Y(135) Y(136) Y(137) Y(138) Y(139) Y(140) \
> > > +Y(141) Y(142) Y(143) Y(144) Y(145) Y(146) Y(147) Y(148) Y(149) Y(150) \
> > > +Y(151) Y(152) Y(153) Y(154) Y(155) Y(156) Y(157) Y(158) Y(159) Y(160) \
> > > +Y(161) Y(162) Y(163) Y(164) Y(165) Y(166) Y(167) Y(168) Y(169) Y(170) \
> > > +Y(171) Y(172) Y(173) Y(174) Y(175) Y(176) Y(177) Y(178) Y(179) Y(180) \
> > > +Y(181) Y(182) Y(183) Y(184) Y(185) Y(186) Y(187) Y(188) Y(189) Y(190) \
> > > +Y(191) Y(192) Y(193) Y(194) Y(195) Y(196) Y(197) Y(198) Y(199) Y(200) \
> > > +Y(201) Y(202) Y(203) Y(204) Y(205) Y(206) Y(207) Y(208) Y(209) Y(210) \
> > > +Y(211) Y(212) Y(213) Y(214) Y(215) Y(216) Y(217) Y(218) Y(219) Y(220) \
> > > +Y(221) Y(222) Y(223) Y(224) Y(225) Y(226) Y(227) Y(228) Y(229) Y(230) \
> > > +Y(231) Y(232) Y(233) Y(234) Y(235) Y(236) Y(237) Y(238) Y(239) Y(240) \
> > > +Y(241) Y(242) Y(243) Y(244) Y(245) Y(246) Y(247) Y(248) Y(249) Y(250) \
> > > +Y(251) Y(252) Y(253) Y(254) Y(255)
> > > +
> > > +#define __KFUNC_A(nr) bpf_testmod_test_mod_kfunc_##nr();
> > > +#define KFUNC_VALID_DISTINCT_256 __KFUNC_NR_EXP(__KFUNC_A)
> > > +
> > > +#define __KFUNC_B(nr) extern void bpf_testmod_test_mod_kfunc_##nr(void) __ksym;
> > > +#define KFUNC_KSYM_DECLARE_VALID_DISTINCT_256 __KFUNC_NR_EXP(__KFUNC_B)
> > > +
> > > +#define __KFUNC_C(nr) noinline void bpf_testmod_test_mod_kfunc_##nr(void) {};
> > > +#define KFUNC_DEFINE_VALID_DISTINCT_256 __KFUNC_NR_EXP(__KFUNC_C)
> > > +
> > > +#define __KFUNC_D(nr) BTF_ID(func, bpf_testmod_test_mod_kfunc_##nr)
> > > +#define KFUNC_BTF_ID_VALID_DISTINCT_256 __KFUNC_NR_EXP(__KFUNC_D)
> > > +
> > > +#define __KFUNC_E(nr) bpf_testmod_test_mod_kfunc(nr);
> > > +#define KFUNC_VALID_SAME_ONE __KFUNC_E(0)
> > > +#define KFUNC_VALID_SAME_256 __KFUNC_NR_EXP(__KFUNC_E)
> > > +
> >
> > This is pretty horrible... Wouldn't it be better to test limits like
>
> Yeah, I actually thought about this a bit yesterday, we could also do:
> (untested)
>
> #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)
>
> Then, for generating 256 items
>
> X_2(X_10(X_10(foo))) X_5(X_10(foo)) X_6(foo)
>
> ... which looks much better.
>
> > this using the test_verifier approach, where we can craft a *short*
> > sequence of instructions that will test all these limits?...
> >
>
> Hmm, good idea, I'd just need to fill in the BTF id dynamically at runtime,
> but that should be possible.
>
> Though we still need to craft distinct calls (I am trying to test the limit
> where insn->off is different for each case). Since we try to reuse index in both
> gen_loader and libbpf, just generating same call 256 times would not be enough.
You just need to generate one instruction with offset = 257 to test
this. And separately one call with fd_array that has module BTF fd at
fd_array[256] (to check that 256 is ok). Or am I missing something?
>
> Let me know which one of the two you prefer.
>
> >
> > > +#endif
> > > --
> > > 2.33.0
> > >
>
> --
> Kartikeya
Powered by blists - more mailing lists