* [PATCH bpf v3 0/2] bpf: fix NULL dereference during extable search
@ 2023-06-09 0:10 Krister Johansen
2023-06-09 0:10 ` [PATCH bpf v3 1/2] bpf: ensure main program has an extable Krister Johansen
2023-06-09 0:11 ` [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables Krister Johansen
0 siblings, 2 replies; 11+ messages in thread
From: Krister Johansen @ 2023-06-09 0:10 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, linux-kernel, linux-kselftest
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
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH bpf v3 1/2] bpf: ensure main program has an extable 2023-06-09 0:10 [PATCH bpf v3 0/2] bpf: fix NULL dereference during extable search Krister Johansen @ 2023-06-09 0:10 ` Krister Johansen 2023-06-09 3:33 ` Yonghong Song 2023-06-09 0:11 ` [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables Krister Johansen 1 sibling, 1 reply; 11+ messages in thread From: Krister Johansen @ 2023-06-09 0:10 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, linux-kernel, linux-kselftest 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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 1/2] bpf: ensure main program has an extable 2023-06-09 0:10 ` [PATCH bpf v3 1/2] bpf: ensure main program has an extable Krister Johansen @ 2023-06-09 3:33 ` Yonghong Song 0 siblings, 0 replies; 11+ messages in thread From: Yonghong Song @ 2023-06-09 3:33 UTC (permalink / raw) To: Krister Johansen, bpf Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, linux-kernel, linux-kselftest 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 0:10 [PATCH bpf v3 0/2] bpf: fix NULL dereference during extable search Krister Johansen 2023-06-09 0:10 ` [PATCH bpf v3 1/2] bpf: ensure main program has an extable Krister Johansen @ 2023-06-09 0:11 ` Krister Johansen 2023-06-09 3:52 ` Yonghong Song 2023-06-09 18:15 ` Alexei Starovoitov 1 sibling, 2 replies; 11+ messages in thread From: Krister Johansen @ 2023-06-09 0:11 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, linux-kernel, linux-kselftest 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"; -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 0:11 ` [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables Krister Johansen @ 2023-06-09 3:52 ` Yonghong Song 2023-06-09 21:34 ` Krister Johansen 2023-06-09 18:15 ` Alexei Starovoitov 1 sibling, 1 reply; 11+ messages in thread From: Yonghong Song @ 2023-06-09 3:52 UTC (permalink / raw) To: Krister Johansen, bpf Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, linux-kernel, linux-kselftest 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"; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 3:52 ` Yonghong Song @ 2023-06-09 21:34 ` Krister Johansen 0 siblings, 0 replies; 11+ messages in thread From: Krister Johansen @ 2023-06-09 21:34 UTC (permalink / raw) To: Yonghong Song Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, linux-kernel, linux-kselftest 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 0:11 ` [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables Krister Johansen 2023-06-09 3:52 ` Yonghong Song @ 2023-06-09 18:15 ` Alexei Starovoitov 2023-06-09 19:08 ` Krister Johansen 2023-06-12 13:46 ` Ilya Leoshkevich 1 sibling, 2 replies; 11+ messages in thread From: Alexei Starovoitov @ 2023-06-09 18:15 UTC (permalink / raw) To: Krister Johansen, Ilya Leoshkevich Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK 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/9416404780 bpf_prog_78c0d4c618ed2df7_handle_fexit_ret_subprogs3 is crashing the kernel. A bug in extable logic on s390? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 18:15 ` Alexei Starovoitov @ 2023-06-09 19:08 ` Krister Johansen 2023-06-12 13:46 ` Ilya Leoshkevich 1 sibling, 0 replies; 11+ messages in thread From: Krister Johansen @ 2023-06-09 19:08 UTC (permalink / raw) To: Alexei Starovoitov Cc: Krister Johansen, Ilya Leoshkevich, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-09 18:15 ` Alexei Starovoitov 2023-06-09 19:08 ` Krister Johansen @ 2023-06-12 13:46 ` Ilya Leoshkevich 2023-06-12 22:07 ` Alexei Starovoitov 1 sibling, 1 reply; 11+ messages in thread From: Ilya Leoshkevich @ 2023-06-12 13:46 UTC (permalink / raw) To: Alexei Starovoitov, Krister Johansen Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK 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/9416404780 > > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-12 13:46 ` Ilya Leoshkevich @ 2023-06-12 22:07 ` Alexei Starovoitov 2023-06-12 22:12 ` Krister Johansen 0 siblings, 1 reply; 11+ messages in thread From: Alexei Starovoitov @ 2023-06-12 22:07 UTC (permalink / raw) To: Ilya Leoshkevich Cc: Krister Johansen, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK 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/9416404780 > > > > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables 2023-06-12 22:07 ` Alexei Starovoitov @ 2023-06-12 22:12 ` Krister Johansen 0 siblings, 0 replies; 11+ messages in thread From: Krister Johansen @ 2023-06-12 22:12 UTC (permalink / raw) To: Alexei Starovoitov Cc: Ilya Leoshkevich, Krister Johansen, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK 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/9416404780 > > > > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-06-12 22:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-09 0:10 [PATCH bpf v3 0/2] bpf: fix NULL dereference during extable search Krister Johansen 2023-06-09 0:10 ` [PATCH bpf v3 1/2] bpf: ensure main program has an extable Krister Johansen 2023-06-09 3:33 ` Yonghong Song 2023-06-09 0:11 ` [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables Krister Johansen 2023-06-09 3:52 ` Yonghong Song 2023-06-09 21:34 ` Krister Johansen 2023-06-09 18:15 ` Alexei Starovoitov 2023-06-09 19:08 ` Krister Johansen 2023-06-12 13:46 ` Ilya Leoshkevich 2023-06-12 22:07 ` Alexei Starovoitov 2023-06-12 22:12 ` Krister Johansen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).