On Mon, Jun 12, 2023 at 03:07:22PM -0700, Alexei Starovoitov wrote:
On Mon, Jun 12, 2023 at 6:46 AM Ilya Leoshkevich iii@linux.ibm.com wrote:
On Fri, 2023-06-09 at 11:15 -0700, Alexei Starovoitov wrote:
On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen kjlx@templeofstupid.com wrote:
In certain situations a program with subprograms may have a NULL extable entry. This should not happen, and when it does, it turns a single trap into multiple. Add a test case for further debugging and to prevent regressions. N.b: without any other patches this can panic or oops a kernel.
Signed-off-by: Krister Johansen kjlx@templeofstupid.com
.../bpf/prog_tests/subprogs_extable.c | 31 +++++++++++++ .../bpf/progs/test_subprogs_extable.c | 46 +++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/subprogs_extable.c create mode 100644 tools/testing/selftests/bpf/progs/test_subprogs_extable.c
diff --git a/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c new file mode 100644 index 000000000000..2201988274a4 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h> +#include "test_subprogs_extable.skel.h"
+void test_subprogs_extable(void) +{
const int READ_SZ = 456;struct test_subprogs_extable *skel;int err;skel = test_subprogs_extable__open();if (!ASSERT_OK_PTR(skel, "skel_open"))return;err = test_subprogs_extable__load(skel);if (!ASSERT_OK(err, "skel_load"))goto cleanup;err = test_subprogs_extable__attach(skel);if (!ASSERT_OK(err, "skel_attach"))goto cleanup;/* trigger tracepoint */ASSERT_OK(trigger_module_test_read(READ_SZ),"trigger_read");
test_subprogs_extable__detach(skel);+cleanup:
test_subprogs_extable__destroy(skel);+} diff --git a/tools/testing/selftests/bpf/progs/test_subprogs_extable.c b/tools/testing/selftests/bpf/progs/test_subprogs_extable.c new file mode 100644 index 000000000000..c3ff66bf4cbe --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_subprogs_extable.c @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h" +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h>
+struct {
__uint(type, BPF_MAP_TYPE_ARRAY);__uint(max_entries, 8);__type(key, __u32);__type(value, __u64);+} test_array SEC(".maps");
+static __u64 test_cb(struct bpf_map *map, __u32 *key, __u64 *val, void *data) +{
return 1;+}
+SEC("fexit/bpf_testmod_return_ptr") +int BPF_PROG(handle_fexit_ret_subprogs, int arg, struct file *ret) +{
*(volatile long *)ret;*(volatile int *)&ret->f_mode;bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);return 0;+}
+SEC("fexit/bpf_testmod_return_ptr") +int BPF_PROG(handle_fexit_ret_subprogs2, int arg, struct file *ret) +{
*(volatile long *)ret;*(volatile int *)&ret->f_mode;bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);return 0;+}
+SEC("fexit/bpf_testmod_return_ptr") +int BPF_PROG(handle_fexit_ret_subprogs3, int arg, struct file *ret) +{
*(volatile long *)ret;*(volatile int *)&ret->f_mode;bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);return 0;+}
What is the point of attaching 3 the same progs to the same hook? One would be enough to test it, no?
In other news... Looks like this test is triggering a bug on s390.
Ilya, please take a look: https://github.com/kernel-patches/bpf/actions/runs/5216942096/jobs/941640478...
bpf_prog_78c0d4c618ed2df7_handle_fexit_ret_subprogs3 is crashing the kernel. A bug in extable logic on s390?
I think we also need this:
--- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17664,6 +17664,7 @@ static int jit_subprogs(struct bpf_verifier_env *env) prog->bpf_func = func[0]->bpf_func; prog->jited_len = func[0]->jited_len; prog->aux->extable = func[0]->aux->extable;
prog->aux->num_exentries = func[0]->aux->num_exentries; prog->aux->func = func; prog->aux->func_cnt = env->subprog_cnt; bpf_prog_jit_attempt_done(prog);The reason is that s390 JIT doubles the number of extable entries due to how the hardware works (some exceptions point to the failing insn, some point to the next one).
With that:
Acked-by: Ilya Leoshkevich iii@linux.ibm.com Tested-by: Ilya Leoshkevich iii@linux.ibm.com
for the v4 series.
Great.
Krister, could you please resubmit v5 adding the above change and Ilya's tags to patch 1?
I'd like to see green BPF CI on all platforms before landing.
Thanks Alexei and Ilya, and yes, absolutely. I'm hoping to have a v5 out a little later this afternoon.
-K