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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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 <[email protected]> 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 <[email protected]>
> ---
> .../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?
On Fri, Jun 09, 2023 at 11:15:18AM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen <[email protected]> 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 <[email protected]>
> > ---
> > .../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
> <[email protected]> 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 <[email protected]>
> > ---
> > .../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 <[email protected]>
Tested-by: Ilya Leoshkevich <[email protected]>
for the v4 series.
On Mon, Jun 12, 2023 at 6:46 AM Ilya Leoshkevich <[email protected]> wrote:
>
> On Fri, 2023-06-09 at 11:15 -0700, Alexei Starovoitov wrote:
> > On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > .../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 <[email protected]>
> Tested-by: Ilya Leoshkevich <[email protected]>
>
> 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 <[email protected]> wrote:
> >
> > On Fri, 2023-06-09 at 11:15 -0700, Alexei Starovoitov wrote:
> > > On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > .../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 <[email protected]>
> > Tested-by: Ilya Leoshkevich <[email protected]>
> >
> > 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