Hi, Enclosed are a pair of patches for an oops that can occur if an exception is generated while a bpf subprogram is running. One of the bpf_prog_aux entries for the subprograms are missing an extable. This can lead to an exception that would otherwise be handled turning into a NULL pointer bug.
When run out of the selftest, the oops looks like this:
BUG: kernel NULL pointer dereference, address: 000000000000000c #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 0 PID: 1132 Comm: test_progs Tainted: G OE 6.4.0-rc3+ #2 RIP: 0010:cmp_ex_search+0xb/0x30 Code: cc cc cc cc e8 36 cb 03 00 66 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 55 48 89 e5 48 8b 07 <48> 63 0e 48 01 f1 31 d2 48 39 c8 19 d2 48 39 c8 b8 01 00 00 00 0f RSP: 0018:ffffb30c4291f998 EFLAGS: 00010006 RAX: ffffffffc00b49da RBX: 0000000000000002 RCX: 000000000000000c RDX: 0000000000000002 RSI: 000000000000000c RDI: ffffb30c4291f9e8 RBP: ffffb30c4291f998 R08: ffffffffab1a42d0 R09: 0000000000000001 R10: 0000000000000000 R11: ffffffffab1a42d0 R12: ffffb30c4291f9e8 R13: 000000000000000c R14: 000000000000000c R15: 0000000000000000 FS: 00007fb5d9e044c0(0000) GS:ffff92e95ee00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000000000000c CR3: 000000010c3a2005 CR4: 00000000007706f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: <TASK> bsearch+0x41/0x90 ? __pfx_cmp_ex_search+0x10/0x10 ? bpf_prog_45a7907e7114d0ff_handle_fexit_ret_subprogs3+0x2a/0x6c search_extable+0x3b/0x60 ? bpf_prog_45a7907e7114d0ff_handle_fexit_ret_subprogs3+0x2a/0x6c search_bpf_extables+0x10d/0x190 ? bpf_prog_45a7907e7114d0ff_handle_fexit_ret_subprogs3+0x2a/0x6c search_exception_tables+0x5d/0x70 fixup_exception+0x3f/0x5b0 ? look_up_lock_class+0x61/0x110 ? __lock_acquire+0x6b8/0x3560 ? __lock_acquire+0x6b8/0x3560 ? __lock_acquire+0x6b8/0x3560 kernelmode_fixup_or_oops+0x46/0x110 __bad_area_nosemaphore+0x68/0x2b0 ? __lock_acquire+0x6b8/0x3560 bad_area_nosemaphore+0x16/0x20 do_kern_addr_fault+0x81/0xa0 exc_page_fault+0xd6/0x210 asm_exc_page_fault+0x2b/0x30 RIP: 0010:bpf_prog_45a7907e7114d0ff_handle_fexit_ret_subprogs3+0x2a/0x6c Code: f3 0f 1e fa 0f 1f 44 00 00 66 90 55 48 89 e5 f3 0f 1e fa 48 8b 7f 08 49 bb 00 00 00 00 00 80 00 00 4c 39 df 73 04 31 f6 eb 04 <48> 8b 77 00 49 bb 00 00 00 00 00 80 00 00 48 81 c7 7c 00 00 00 4c RSP: 0018:ffffb30c4291fcb8 EFLAGS: 00010282 RAX: 0000000000000001 RBX: 0000000000000001 RCX: 0000000000000000 RDX: 00000000cddf1af1 RSI: 000000005315a00d RDI: ffffffffffffffea RBP: ffffb30c4291fcb8 R08: ffff92e644bf38a8 R09: 0000000000000000 R10: 0000000000000000 R11: 0000800000000000 R12: ffff92e663652690 R13: 00000000000001c8 R14: 00000000000001c8 R15: 0000000000000003 bpf_trampoline_251255721842_2+0x63/0x1000 bpf_testmod_return_ptr+0x9/0xb0 [bpf_testmod] ? bpf_testmod_test_read+0x43/0x2d0 [bpf_testmod] sysfs_kf_bin_read+0x60/0x90 kernfs_fop_read_iter+0x143/0x250 vfs_read+0x240/0x2a0 ksys_read+0x70/0xe0 __x64_sys_read+0x1f/0x30 do_syscall_64+0x68/0xa0 ? syscall_exit_to_user_mode+0x77/0x1f0 ? do_syscall_64+0x77/0xa0 ? irqentry_exit+0x35/0xa0 ? sysvec_apic_timer_interrupt+0x4d/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc RIP: 0033:0x7fb5da00a392 Code: ac 00 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb be 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24 RSP: 002b:00007ffc5b3cab68 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 000055bee7b8b100 RCX: 00007fb5da00a392 RDX: 00000000000001c8 RSI: 0000000000000000 RDI: 0000000000000009 RBP: 00007ffc5b3caba0 R08: 0000000000000000 R09: 0000000000000037 R10: 000055bee7b8c2a7 R11: 0000000000000246 R12: 000055bee78f1f60 R13: 00007ffc5b3cae90 R14: 0000000000000000 R15: 0000000000000000 </TASK> Modules linked in: bpf_testmod(OE) nls_iso8859_1 dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua intel_rapl_msr intel_rapl_common intel_uncore_frequency_common ppdev nfit crct10dif_pclmul crc32_pclmul psmouse ghash_clmulni_intel sha512_ssse3 aesni_intel parport_pc crypto_simd cryptd input_leds parport rapl ena i2c_piix4 mac_hid serio_raw ramoops reed_solomon pstore_blk drm pstore_zone efi_pstore autofs4 [last unloaded: bpf_testmod(OE)] CR2: 000000000000000c
These changes were tested via the verifier and progs selftests and no regressions were observed.
Changes from v2: - Insert only the main program's kallsyms (Feedback from Yonghong Song and Alexei Starovoitov) - Selftest should use ASSERT instead of CHECK (Feedback from Yonghong Song) - Selftest needs some cleanup (Feedback from Yonghong Song) - Switch patch order (Feedback from Alexei Starovoitov)
Changes from v1:
- Add a selftest (Feedback From Alexei Starovoitov) - Move to a 1-line verifier change instead of searching multiple extables
Krister Johansen (2): bpf: ensure main program has an extable selftests/bpf: add a test for subprogram extables
kernel/bpf/verifier.c | 6 ++- .../bpf/prog_tests/subprogs_extable.c | 31 +++++++++++++ .../bpf/progs/test_subprogs_extable.c | 46 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/subprogs_extable.c create mode 100644 tools/testing/selftests/bpf/progs/test_subprogs_extable.c
When subprograms are in use, the main program is not jit'd after the subprograms because jit_subprogs sets a value for prog->bpf_func upon success. Subsequent calls to the JIT are bypassed when this value is non-NULL. This leads to a situation where the main program and its func[0] counterpart are both in the bpf kallsyms tree, but only func[0] has an extable. Extables are only created during JIT. Now there are two nearly identical program ksym entries in the tree, but only one has an extable. Depending upon how the entries are placed, there's a chance that a fault will call search_extable on the aux with the NULL entry.
Since jit_subprogs already copies state from func[0] to the main program, include the extable pointer in this state duplication. Additionally, ensure that the copy of the main program in func[0] is not added to the bpf_prog_kallsyms table. Instead, let the main program get added later in bpf_prog_load(). This ensures there is only a single copy of the main program in the kallsyms table, and that its tag matches the tag observed by tooling like bpftool.
Cc: stable@vger.kernel.org Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs") Signed-off-by: Krister Johansen kjlx@templeofstupid.com --- kernel/bpf/verifier.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5871aa78d01a..b62d1fc0f92b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17214,9 +17214,10 @@ static int jit_subprogs(struct bpf_verifier_env *env) }
/* finally lock prog and jit images for all functions and - * populate kallsysm + * populate kallsysm. Begin at the first subprogram, since + * bpf_prog_load will add the kallsyms for the main program. */ - for (i = 0; i < env->subprog_cnt; i++) { + for (i = 1; i < env->subprog_cnt; i++) { bpf_prog_lock_ro(func[i]); bpf_prog_kallsyms_add(func[i]); } @@ -17242,6 +17243,7 @@ static int jit_subprogs(struct bpf_verifier_env *env) prog->jited = 1; prog->bpf_func = func[0]->bpf_func; prog->jited_len = func[0]->jited_len; + prog->aux->extable = func[0]->aux->extable; prog->aux->func = func; prog->aux->func_cnt = env->subprog_cnt; bpf_prog_jit_attempt_done(prog);
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; +} + +char _license[] SEC("license") = "GPL";
On 6/8/23 5:10 PM, Krister Johansen wrote:
When subprograms are in use, the main program is not jit'd after the subprograms because jit_subprogs sets a value for prog->bpf_func upon success. Subsequent calls to the JIT are bypassed when this value is non-NULL. This leads to a situation where the main program and its func[0] counterpart are both in the bpf kallsyms tree, but only func[0] has an extable. Extables are only created during JIT. Now there are two nearly identical program ksym entries in the tree, but only one has an extable. Depending upon how the entries are placed, there's a chance that a fault will call search_extable on the aux with the NULL entry.
Since jit_subprogs already copies state from func[0] to the main program, include the extable pointer in this state duplication. Additionally, ensure that the copy of the main program in func[0] is not added to the bpf_prog_kallsyms table. Instead, let the main program get added later in bpf_prog_load(). This ensures there is only a single copy of the main program in the kallsyms table, and that its tag matches the tag observed by tooling like bpftool.
Cc: stable@vger.kernel.org Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs") Signed-off-by: Krister Johansen kjlx@templeofstupid.com
Acked-by: Yonghong Song yhs@fb.com
On 6/8/23 5:11 PM, Krister Johansen 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.
It would be great if you can add the panic call stack in the commit message.
Please also mention that three identical bpf programs in the test significantly increased the 'oops' chance. Just one program may not be able to trigger the issue.
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;
There is no need to use uppercase for READ_SZ. Just do const int read_sz = 456; is sufficient.
- 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;
You can combine the above open and load with a single one test_subprogs_extable__open_and_load().
- 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");
I think we should at least ensure that the program is triggered. For example, add a global variable 'triggered' in the program and triggered will be set to 1 in the program if the program is running. Here check skel->bss->triggered must be 1.
- 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;
+}
+char _license[] SEC("license") = "GPL";
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?
On Fri, Jun 09, 2023 at 11:15:18AM -0700, Alexei Starovoitov wrote:
On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen kjlx@templeofstupid.com wrote:
+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?
I thought so too, initially. However, when I went to move this from the original test case I submitted to the selftest, I found it was fairly inconsistent about reproducing the problem with a single program. I believe this is because the kallsyms are stored in a binary tree, and the prog and func[0] are identical. Depending on where the item is placed, the func[0] with the extable can sometimes be looked up instead of the prog without.
Yonghong requested that I make note of this in the patch commit message. I'll make sure that's included in the next version I send out.
In other news... Looks like this test is triggering a bug on s390.
Not sure if this is worth mentioning, but when I run with panic_on_oops=0 to capture the stack, I'm seeing some additional warnings that follow the bpf bug. Is any of this of interest?
BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49 in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 1132, name: test_progs preempt_count: 0, expected: 0 RCU nest depth: 2, expected: 0 INFO: lockdep is turned off. CPU: 0 PID: 1132 Comm: test_progs Tainted: G D OE 6.4.0-rc3+ #2 Call Trace: <TASK> dump_stack_lvl+0x63/0x90 dump_stack+0x14/0x20 __might_resched+0x21d/0x230 __might_sleep+0x45/0x70 exit_signals+0x35/0x200 do_exit+0xc6/0x920 ? rewind_stack_and_make_dead+0x17/0x20 ? make_task_dead+0xbe/0x140 ? make_task_dead+0xbe/0x140 make_task_dead+0x88/0x140 rewind_stack_and_make_dead+0x17/0x20 RIP: 0033:0x7fb5da00a392 Code: ac 00 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb be 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24 RSP: 002b:00007ffc5b3cab68 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 000055bee7b8b100 RCX: 00007fb5da00a392 RDX: 00000000000001c8 RSI: 0000000000000000 RDI: 0000000000000009 RBP: 00007ffc5b3caba0 R08: 0000000000000000 R09: 0000000000000037 R10: 000055bee7b8c2a7 R11: 0000000000000246 R12: 000055bee78f1f60 R13: 00007ffc5b3cae90 R14: 0000000000000000 R15: 0000000000000000 </TASK> ------------[ cut here ]------------ Voluntary context switch within RCU read-side critical section! WARNING: CPU: 0 PID: 1132 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x4c0/0x5f0 Modules linked in: bpf_testmod(OE) nls_iso8859_1 dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua intel_rapl_msr intel_rapl_common intel_uncore_frequency_common ppdev nfit crct10dif_pclmul crc32_pclmul psmouse ghash_clmulni_intel sha512_ssse3 aesni_intel parport_pc crypto_simd cryptd input_leds parport rapl ena i2c_piix4 mac_hid serio_raw ramoops reed_solomon pstore_blk drm pstore_zone efi_pstore autofs4 [last unloaded: bpf_testmod(OE)] CPU: 0 PID: 1132 Comm: test_progs Tainted: G D W OE 6.4.0-rc3+ #2 RIP: 0010:rcu_note_context_switch+0x4c0/0x5f0 Code: fb ff ff 0f 0b e9 e0 fb ff ff e8 4b dd e3 ff a8 04 75 b5 0f 0b eb b1 c6 05 e4 03 a4 02 01 48 c7 c7 45 4a a1 ab e8 c0 e2 f1 ff <0f> 0b e9 f7 fb ff ff 0f 0b 45 84 f6 0f 84 d6 fb ff ff e9 e7 fb ff RSP: 0018:ffffb30c4291f9a8 EFLAGS: 00010046 RAX: 4599311900096300 RBX: ffff92e644bf2a40 RCX: 0000000000000027 RDX: 0000000000000000 RSI: ffffb30c4291f830 RDI: ffff92e95ee21948 RBP: ffffb30c4291f9f8 R08: 0000000000000000 R09: ffffb30c4291f7d0 R10: 00000000fffeffff R11: c0000000fffeffff R12: ffff92e95ee36680 R13: ffffb30c4291fc58 R14: 0000000000000000 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff92e95ee00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000000000000c CR3: 000000000d25e001 CR4: 00000000007706f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: <TASK> ? lock_release+0x46/0x330 __schedule+0x173/0x1730 ? trace_hardirqs_on+0x56/0xb0 ? irqentry_exit+0x72/0xa0 ? sysvec_irq_work+0x4a/0x90 ? asm_sysvec_irq_work+0x1f/0x30 schedule+0x6f/0xc0 schedule_timeout+0x35/0x110 ? native_write_msr+0xe/0x40 ? __pfx_schedule_timeout+0x10/0x10 ? trace_hardirqs_on+0x56/0xb0 ? __pfx_schedule_timeout+0x10/0x10 do_wait_for_common+0xe9/0x170 ? __pfx_schedule_timeout+0x10/0x10 ? __pfx_call_rcu+0x10/0x10 wait_for_completion+0x53/0x70 __wait_rcu_gp+0x12f/0x150 synchronize_rcu_tasks_rude+0x67/0xc0 ? __pfx_wakeme_after_rcu+0x10/0x10 ? mutex_unlock+0x16/0x20 ? __pfx_call_rcu_tasks_rude+0x10/0x10 ftrace_shutdown+0x1ea/0x290 ? 0xffffffffc04b8000 unregister_ftrace_function+0x30/0x190 ? 0xffffffffc04b8000 ? 0xffffffffc04b8000 unregister_ftrace_direct+0x51/0xf0 ? __pfx_bpf_testmod_return_ptr+0x10/0x10 [bpf_testmod] ? 0xffffffffc04b8000 bpf_trampoline_update+0x273/0x6d0 bpf_trampoline_unlink_prog+0xb4/0x110 bpf_tracing_link_release+0x1d/0x50 bpf_link_put+0xd0/0x100 bpf_link_release+0x19/0x30 __fput+0x107/0x250 ____fput+0x12/0x20 task_work_run+0x89/0xd0 do_exit+0x263/0x920 ? make_task_dead+0xbe/0x140 ? make_task_dead+0xbe/0x140 make_task_dead+0x88/0x140 rewind_stack_and_make_dead+0x17/0x20 RIP: 0033:0x7fb5da00a392 Code: Unable to access opcode bytes at 0x7fb5da00a368. RSP: 002b:00007ffc5b3cab68 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 000055bee7b8b100 RCX: 00007fb5da00a392 RDX: 00000000000001c8 RSI: 0000000000000000 RDI: 0000000000000009 RBP: 00007ffc5b3caba0 R08: 0000000000000000 R09: 0000000000000037 R10: 000055bee7b8c2a7 R11: 0000000000000246 R12: 000055bee78f1f60 R13: 00007ffc5b3cae90 R14: 0000000000000000 R15: 0000000000000000 </TASK> irq event stamp: 62970 hardirqs last enabled at (62969): [<ffffffffab1e14ba>] syscall_enter_from_user_mode+0x2a/0x1e0 hardirqs last disabled at (62970): [<ffffffffab1e0bb1>] exc_page_fault+0x41/0x210 softirqs last enabled at (62912): [<ffffffffaa2e2ae2>] bpf_link_settle+0x32/0x50 softirqs last disabled at (62910): [<ffffffffaa2e2acd>] bpf_link_settle+0x1d/0x50 ---[ end trace 0000000000000000 ]---
-K
On Thu, Jun 08, 2023 at 08:52:36PM -0700, Yonghong Song wrote:
On 6/8/23 5:11 PM, Krister Johansen 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.
It would be great if you can add the panic call stack in the commit message.
Please also mention that three identical bpf programs in the test significantly increased the 'oops' chance. Just one program may not be able to trigger the issue.
Apologies, I mistakenly put the oops in the cover letter when you did tell me to put it in the commit message for this patch. That is fixed now.
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;
There is no need to use uppercase for READ_SZ. Just do const int read_sz = 456; is sufficient.
- 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;You can combine the above open and load with a single one test_subprogs_extable__open_and_load().
- 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");
I think we should at least ensure that the program is triggered. For example, add a global variable 'triggered' in the program and triggered will be set to 1 in the program if the program is running. Here check skel->bss->triggered must be 1.
Thanks for the additional feedback on cleaning this up. I've incorporated the requested changes and will send out a v4 shortly.
-K
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.
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.
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
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Ilya Leoshkevich -
Krister Johansen -
Yonghong Song