On Wed, Dec 13, 2023 at 5:44 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > Hi, > > On 12/14/2023 7:33 AM, Andrii Nakryiko wrote: > > On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > >> From: Hou Tao <houtao1@xxxxxxxxxx> > >> > >> If an abnormally huge cnt is used for multi-kprobes attachment, the > >> following warning will be reported: > >> > >> ------------[ cut here ]------------ > >> WARNING: CPU: 1 PID: 392 at mm/util.c:632 kvmalloc_node+0xd9/0xe0 > >> Modules linked in: bpf_testmod(O) > >> CPU: 1 PID: 392 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32 > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) > >> ...... > >> RIP: 0010:kvmalloc_node+0xd9/0xe0 > >> ? __warn+0x89/0x150 > >> ? kvmalloc_node+0xd9/0xe0 > >> bpf_kprobe_multi_link_attach+0x87/0x670 > >> __sys_bpf+0x2a28/0x2bc0 > >> __x64_sys_bpf+0x1a/0x30 > >> do_syscall_64+0x36/0xb0 > >> entry_SYSCALL_64_after_hwframe+0x6e/0x76 > >> RIP: 0033:0x7fbe067f0e0d > >> ...... > >> </TASK> > >> ---[ end trace 0000000000000000 ]--- > >> > >> So add a test to ensure the warning is fixed. > >> > >> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx> > >> --- > >> .../selftests/bpf/prog_tests/kprobe_multi_test.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > >> index 4041cfa670eb..802554d4ee24 100644 > >> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > >> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > >> @@ -300,6 +300,20 @@ static void test_attach_api_fails(void) > >> if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_5_error")) > >> goto cleanup; > >> > >> + /* fail_6 - abnormal cnt */ > >> + opts.addrs = (const unsigned long *) addrs; > >> + opts.syms = NULL; > >> + opts.cnt = INT_MAX; > >> + opts.cookies = NULL; > >> + > >> + link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual, > >> + NULL, &opts); > >> + if (!ASSERT_ERR_PTR(link, "fail_6")) > >> + goto cleanup; > >> + > >> + if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_6_error")) > > this is unreliable, store errno right after the operation before > > ASSERT_xxx() macros > > I didn't fully follow the reason why it is unreliable. Do you mean > ASSERT_ERR_PTR() macro may overwrite errno, right ? But _CHECK() already > saves and restores errno before invoking fprintf(), so I think it is OK > to use libbpf_get_error() to get the errno here ? We shouldn't be using libbpf_get_error() in new code, it's legacy and not advised to be used. If ASSERT_xxx() macro guarantee to save/restore errno, then it might be actually fine, but it still feels better to save errno explicitly right after the operation, just like you'd do in normal code. > > > >> + goto cleanup; > >> + > >> cleanup: > >> bpf_link__destroy(link); > >> kprobe_multi__destroy(skel); > >> -- > >> 2.29.2 > >> >
