From 6cb86a0fdece87e126323ec1bb19deb16a52aedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Toke=20H=C3=B8iland-J=C3=B8rgensen?= Date: Thu, 10 Oct 2024 15:27:07 +0200 Subject: [PATCH 1/3] bpf: fix kfunc btf caching for modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The verifier contains a cache for looking up module BTF objects when calling kfuncs defined in modules. This cache uses a 'struct bpf_kfunc_btf_tab', which contains a sorted list of BTF objects that were already seen in the current verifier run, and the BTF objects are looked up by the offset stored in the relocated call instruction using bsearch(). The first time a given offset is seen, the module BTF is loaded from the file descriptor passed in by libbpf, and stored into the cache. However, there's a bug in the code storing the new entry: it stores a pointer to the new cache entry, then calls sort() to keep the cache sorted for the next lookup using bsearch(), and then returns the entry that was just stored through the stored pointer. However, because sort() modifies the list of entries in place *by value*, the stored pointer may no longer point to the right entry, in which case the wrong BTF object will be returned. The end result of this is an intermittent bug where, if a BPF program calls two functions with the same signature in two different modules, the function from the wrong module may sometimes end up being called. Whether this happens depends on the order of the calls in the BPF program (as that affects whether sort() reorders the array of BTF objects), making it especially hard to track down. Simon, credited as reporter below, spent significant effort analysing and creating a reproducer for this issue. The reproducer is added as a selftest in a subsequent patch. The fix is straight forward: simply don't use the stored pointer after calling sort(). Since we already have an on-stack pointer to the BTF object itself at the point where the function return, just use that, and populate it from the cache entry in the branch where the lookup succeeds. Fixes: 2357672c54c3 ("bpf: Introduce BPF support for kernel module function calls") Reported-by: Simon Sundberg Acked-by: Jiri Olsa Acked-by: Kumar Kartikeya Dwivedi Signed-off-by: Toke Høiland-Jørgensen Link: https://lore.kernel.org/r/20241010-fix-kfunc-btf-caching-for-modules-v2-1-745af6c1af98@redhat.com Signed-off-by: Alexei Starovoitov --- kernel/bpf/verifier.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 633fd6da40c2..bf9996ea34fe 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2750,10 +2750,16 @@ static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env, b->module = mod; b->offset = offset; + /* sort() reorders entries by value, so b may no longer point + * to the right entry after this + */ sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]), kfunc_btf_cmp_by_off, NULL); + } else { + btf = b->btf; } - return b->btf; + + return btf; } void bpf_free_kfunc_btf_tab(struct bpf_kfunc_btf_tab *tab) From 4192bb294f80928bc2257c7a2ff6c86a27de6807 Mon Sep 17 00:00:00 2001 From: Simon Sundberg Date: Thu, 10 Oct 2024 15:27:08 +0200 Subject: [PATCH 2/3] selftests/bpf: Provide a generic [un]load_module helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalize the previous [un]load_bpf_testmod() helpers (in testing_helpers.c) to the more generic [un]load_module(), which can load an arbitrary kernel module by name. This allows future selftests to more easily load custom kernel modules other than bpf_testmod.ko. Refactor [un]load_bpf_testmod() to wrap this new helper. Signed-off-by: Simon Sundberg Acked-by: Jiri Olsa Acked-by: Kumar Kartikeya Dwivedi Signed-off-by: Toke Høiland-Jørgensen Link: https://lore.kernel.org/r/20241010-fix-kfunc-btf-caching-for-modules-v2-2-745af6c1af98@redhat.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/testing_helpers.c | 34 ++++++++++++------- tools/testing/selftests/bpf/testing_helpers.h | 2 ++ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index d3c3c3a24150..5e9f16683be5 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -367,7 +367,7 @@ int delete_module(const char *name, int flags) return syscall(__NR_delete_module, name, flags); } -int unload_bpf_testmod(bool verbose) +int unload_module(const char *name, bool verbose) { int ret, cnt = 0; @@ -375,11 +375,11 @@ int unload_bpf_testmod(bool verbose) fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n"); for (;;) { - ret = delete_module("bpf_testmod", 0); + ret = delete_module(name, 0); if (!ret || errno != EAGAIN) break; if (++cnt > 10000) { - fprintf(stdout, "Unload of bpf_testmod timed out\n"); + fprintf(stdout, "Unload of %s timed out\n", name); break; } usleep(100); @@ -388,41 +388,51 @@ int unload_bpf_testmod(bool verbose) if (ret) { if (errno == ENOENT) { if (verbose) - fprintf(stdout, "bpf_testmod.ko is already unloaded.\n"); + fprintf(stdout, "%s.ko is already unloaded.\n", name); return -1; } - fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno); + fprintf(stdout, "Failed to unload %s.ko from kernel: %d\n", name, -errno); return -1; } if (verbose) - fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n"); + fprintf(stdout, "Successfully unloaded %s.ko.\n", name); return 0; } -int load_bpf_testmod(bool verbose) +int load_module(const char *path, bool verbose) { int fd; if (verbose) - fprintf(stdout, "Loading bpf_testmod.ko...\n"); + fprintf(stdout, "Loading %s...\n", path); - fd = open("bpf_testmod.ko", O_RDONLY); + fd = open(path, O_RDONLY); if (fd < 0) { - fprintf(stdout, "Can't find bpf_testmod.ko kernel module: %d\n", -errno); + fprintf(stdout, "Can't find %s kernel module: %d\n", path, -errno); return -ENOENT; } if (finit_module(fd, "", 0)) { - fprintf(stdout, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno); + fprintf(stdout, "Failed to load %s into the kernel: %d\n", path, -errno); close(fd); return -EINVAL; } close(fd); if (verbose) - fprintf(stdout, "Successfully loaded bpf_testmod.ko.\n"); + fprintf(stdout, "Successfully loaded %s.\n", path); return 0; } +int unload_bpf_testmod(bool verbose) +{ + return unload_module("bpf_testmod", verbose); +} + +int load_bpf_testmod(bool verbose) +{ + return load_module("bpf_testmod.ko", verbose); +} + /* * Trigger synchronize_rcu() in kernel. */ diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h index d55f6ab12433..46d7f7089f63 100644 --- a/tools/testing/selftests/bpf/testing_helpers.h +++ b/tools/testing/selftests/bpf/testing_helpers.h @@ -38,6 +38,8 @@ int unload_bpf_testmod(bool verbose); int kern_sync_rcu(void); int finit_module(int fd, const char *param_values, int flags); int delete_module(const char *name, int flags); +int load_module(const char *path, bool verbose); +int unload_module(const char *name, bool verbose); static inline __u64 get_time_ns(void) { From f91b256644ea6f7628580029c5a223573f55d98c Mon Sep 17 00:00:00 2001 From: Simon Sundberg Date: Thu, 10 Oct 2024 15:27:09 +0200 Subject: [PATCH 3/3] selftests/bpf: Add test for kfunc module order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a test case for kfuncs from multiple external modules, checking that the correct kfuncs are called regardless of which order they're called in. Specifically, check that calling the kfuncs in an order different from the one the modules' BTF are loaded in works. Signed-off-by: Simon Sundberg Acked-by: Kumar Kartikeya Dwivedi Signed-off-by: Toke Høiland-Jørgensen Link: https://lore.kernel.org/r/20241010-fix-kfunc-btf-caching-for-modules-v2-3-745af6c1af98@redhat.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/Makefile | 20 ++++++- .../bpf/bpf_test_modorder_x/Makefile | 19 +++++++ .../bpf_test_modorder_x/bpf_test_modorder_x.c | 39 +++++++++++++ .../bpf/bpf_test_modorder_y/Makefile | 19 +++++++ .../bpf_test_modorder_y/bpf_test_modorder_y.c | 39 +++++++++++++ .../bpf/prog_tests/kfunc_module_order.c | 55 +++++++++++++++++++ .../selftests/bpf/progs/kfunc_module_order.c | 30 ++++++++++ 7 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/bpf/bpf_test_modorder_x/Makefile create mode 100644 tools/testing/selftests/bpf/bpf_test_modorder_x/bpf_test_modorder_x.c create mode 100644 tools/testing/selftests/bpf/bpf_test_modorder_y/Makefile create mode 100644 tools/testing/selftests/bpf/bpf_test_modorder_y/bpf_test_modorder_y.c create mode 100644 tools/testing/selftests/bpf/prog_tests/kfunc_module_order.c create mode 100644 tools/testing/selftests/bpf/progs/kfunc_module_order.c diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index f04af11df8eb..6f9838a5dcc9 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -157,7 +157,8 @@ TEST_GEN_PROGS_EXTENDED = \ flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \ test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \ - xdp_features bpf_test_no_cfi.ko + xdp_features bpf_test_no_cfi.ko bpf_test_modorder_x.ko \ + bpf_test_modorder_y.ko TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi @@ -303,6 +304,19 @@ $(OUTPUT)/bpf_test_no_cfi.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_te $(Q)$(MAKE) $(submake_extras) RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) -C bpf_test_no_cfi $(Q)cp bpf_test_no_cfi/bpf_test_no_cfi.ko $@ +$(OUTPUT)/bpf_test_modorder_x.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_test_modorder_x/Makefile bpf_test_modorder_x/*.[ch]) + $(call msg,MOD,,$@) + $(Q)$(RM) bpf_test_modorder_x/bpf_test_modorder_x.ko # force re-compilation + $(Q)$(MAKE) $(submake_extras) RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) -C bpf_test_modorder_x + $(Q)cp bpf_test_modorder_x/bpf_test_modorder_x.ko $@ + +$(OUTPUT)/bpf_test_modorder_y.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_test_modorder_y/Makefile bpf_test_modorder_y/*.[ch]) + $(call msg,MOD,,$@) + $(Q)$(RM) bpf_test_modorder_y/bpf_test_modorder_y.ko # force re-compilation + $(Q)$(MAKE) $(submake_extras) RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) -C bpf_test_modorder_y + $(Q)cp bpf_test_modorder_y/bpf_test_modorder_y.ko $@ + + DEFAULT_BPFTOOL := $(HOST_SCRATCH_DIR)/sbin/bpftool ifneq ($(CROSS_COMPILE),) CROSS_BPFTOOL := $(SCRATCH_DIR)/sbin/bpftool @@ -722,6 +736,8 @@ TRUNNER_EXTRA_SOURCES := test_progs.c \ ip_check_defrag_frags.h TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ $(OUTPUT)/bpf_test_no_cfi.ko \ + $(OUTPUT)/bpf_test_modorder_x.ko \ + $(OUTPUT)/bpf_test_modorder_y.ko \ $(OUTPUT)/liburandom_read.so \ $(OUTPUT)/xdp_synproxy \ $(OUTPUT)/sign-file \ @@ -856,6 +872,8 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \ $(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h \ no_alu32 cpuv4 bpf_gcc bpf_testmod.ko \ bpf_test_no_cfi.ko \ + bpf_test_modorder_x.ko \ + bpf_test_modorder_y.ko \ liburandom_read.so) \ $(OUTPUT)/FEATURE-DUMP.selftests diff --git a/tools/testing/selftests/bpf/bpf_test_modorder_x/Makefile b/tools/testing/selftests/bpf/bpf_test_modorder_x/Makefile new file mode 100644 index 000000000000..40b25b98ad1b --- /dev/null +++ b/tools/testing/selftests/bpf/bpf_test_modorder_x/Makefile @@ -0,0 +1,19 @@ +BPF_TESTMOD_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST))))) +KDIR ?= $(abspath $(BPF_TESTMOD_DIR)/../../../../..) + +ifeq ($(V),1) +Q = +else +Q = @ +endif + +MODULES = bpf_test_modorder_x.ko + +obj-m += bpf_test_modorder_x.o + +all: + +$(Q)make -C $(KDIR) M=$(BPF_TESTMOD_DIR) modules + +clean: + +$(Q)make -C $(KDIR) M=$(BPF_TESTMOD_DIR) clean + diff --git a/tools/testing/selftests/bpf/bpf_test_modorder_x/bpf_test_modorder_x.c b/tools/testing/selftests/bpf/bpf_test_modorder_x/bpf_test_modorder_x.c new file mode 100644 index 000000000000..0cc747fa912f --- /dev/null +++ b/tools/testing/selftests/bpf/bpf_test_modorder_x/bpf_test_modorder_x.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include +#include +#include + +__bpf_kfunc_start_defs(); + +__bpf_kfunc int bpf_test_modorder_retx(void) +{ + return 'x'; +} + +__bpf_kfunc_end_defs(); + +BTF_KFUNCS_START(bpf_test_modorder_kfunc_x_ids) +BTF_ID_FLAGS(func, bpf_test_modorder_retx); +BTF_KFUNCS_END(bpf_test_modorder_kfunc_x_ids) + +static const struct btf_kfunc_id_set bpf_test_modorder_x_set = { + .owner = THIS_MODULE, + .set = &bpf_test_modorder_kfunc_x_ids, +}; + +static int __init bpf_test_modorder_x_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, + &bpf_test_modorder_x_set); +} + +static void __exit bpf_test_modorder_x_exit(void) +{ +} + +module_init(bpf_test_modorder_x_init); +module_exit(bpf_test_modorder_x_exit); + +MODULE_DESCRIPTION("BPF selftest ordertest module X"); +MODULE_LICENSE("GPL"); diff --git a/tools/testing/selftests/bpf/bpf_test_modorder_y/Makefile b/tools/testing/selftests/bpf/bpf_test_modorder_y/Makefile new file mode 100644 index 000000000000..52c3ab9d84e2 --- /dev/null +++ b/tools/testing/selftests/bpf/bpf_test_modorder_y/Makefile @@ -0,0 +1,19 @@ +BPF_TESTMOD_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST))))) +KDIR ?= $(abspath $(BPF_TESTMOD_DIR)/../../../../..) + +ifeq ($(V),1) +Q = +else +Q = @ +endif + +MODULES = bpf_test_modorder_y.ko + +obj-m += bpf_test_modorder_y.o + +all: + +$(Q)make -C $(KDIR) M=$(BPF_TESTMOD_DIR) modules + +clean: + +$(Q)make -C $(KDIR) M=$(BPF_TESTMOD_DIR) clean + diff --git a/tools/testing/selftests/bpf/bpf_test_modorder_y/bpf_test_modorder_y.c b/tools/testing/selftests/bpf/bpf_test_modorder_y/bpf_test_modorder_y.c new file mode 100644 index 000000000000..c627ee085d13 --- /dev/null +++ b/tools/testing/selftests/bpf/bpf_test_modorder_y/bpf_test_modorder_y.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include +#include +#include + +__bpf_kfunc_start_defs(); + +__bpf_kfunc int bpf_test_modorder_rety(void) +{ + return 'y'; +} + +__bpf_kfunc_end_defs(); + +BTF_KFUNCS_START(bpf_test_modorder_kfunc_y_ids) +BTF_ID_FLAGS(func, bpf_test_modorder_rety); +BTF_KFUNCS_END(bpf_test_modorder_kfunc_y_ids) + +static const struct btf_kfunc_id_set bpf_test_modorder_y_set = { + .owner = THIS_MODULE, + .set = &bpf_test_modorder_kfunc_y_ids, +}; + +static int __init bpf_test_modorder_y_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, + &bpf_test_modorder_y_set); +} + +static void __exit bpf_test_modorder_y_exit(void) +{ +} + +module_init(bpf_test_modorder_y_init); +module_exit(bpf_test_modorder_y_exit); + +MODULE_DESCRIPTION("BPF selftest ordertest module Y"); +MODULE_LICENSE("GPL"); diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_module_order.c b/tools/testing/selftests/bpf/prog_tests/kfunc_module_order.c new file mode 100644 index 000000000000..48c0560d398e --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_module_order.c @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include + +#include "kfunc_module_order.skel.h" + +static int test_run_prog(const struct bpf_program *prog, + struct bpf_test_run_opts *opts) +{ + int err; + + err = bpf_prog_test_run_opts(bpf_program__fd(prog), opts); + if (!ASSERT_OK(err, "bpf_prog_test_run_opts")) + return err; + + if (!ASSERT_EQ((int)opts->retval, 0, bpf_program__name(prog))) + return -EINVAL; + + return 0; +} + +void test_kfunc_module_order(void) +{ + struct kfunc_module_order *skel; + char pkt_data[64] = {}; + int err = 0; + + DECLARE_LIBBPF_OPTS(bpf_test_run_opts, test_opts, .data_in = pkt_data, + .data_size_in = sizeof(pkt_data)); + + err = load_module("bpf_test_modorder_x.ko", + env_verbosity > VERBOSE_NONE); + if (!ASSERT_OK(err, "load bpf_test_modorder_x.ko")) + return; + + err = load_module("bpf_test_modorder_y.ko", + env_verbosity > VERBOSE_NONE); + if (!ASSERT_OK(err, "load bpf_test_modorder_y.ko")) + goto exit_modx; + + skel = kfunc_module_order__open_and_load(); + if (!ASSERT_OK_PTR(skel, "kfunc_module_order__open_and_load()")) { + err = -EINVAL; + goto exit_mods; + } + + test_run_prog(skel->progs.call_kfunc_xy, &test_opts); + test_run_prog(skel->progs.call_kfunc_yx, &test_opts); + + kfunc_module_order__destroy(skel); +exit_mods: + unload_module("bpf_test_modorder_y", env_verbosity > VERBOSE_NONE); +exit_modx: + unload_module("bpf_test_modorder_x", env_verbosity > VERBOSE_NONE); +} diff --git a/tools/testing/selftests/bpf/progs/kfunc_module_order.c b/tools/testing/selftests/bpf/progs/kfunc_module_order.c new file mode 100644 index 000000000000..76003d04c95f --- /dev/null +++ b/tools/testing/selftests/bpf/progs/kfunc_module_order.c @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include + +extern int bpf_test_modorder_retx(void) __ksym; +extern int bpf_test_modorder_rety(void) __ksym; + +SEC("classifier") +int call_kfunc_xy(struct __sk_buff *skb) +{ + int ret1, ret2; + + ret1 = bpf_test_modorder_retx(); + ret2 = bpf_test_modorder_rety(); + + return ret1 == 'x' && ret2 == 'y' ? 0 : -1; +} + +SEC("classifier") +int call_kfunc_yx(struct __sk_buff *skb) +{ + int ret1, ret2; + + ret1 = bpf_test_modorder_rety(); + ret2 = bpf_test_modorder_retx(); + + return ret1 == 'y' && ret2 == 'x' ? 0 : -1; +} + +char _license[] SEC("license") = "GPL";