Re: [PATCH bpf] selftests/bpf: Do not attach kprobe_multi bench to bpf_dispatcher_xdp_func

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jul 14, 2022 at 1:01 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/14/22 1:23 AM, Jiri Olsa wrote:
> > Alexei reported crash by running test_progs -j on system
> > with 32 cpus.
> >
> > It turned out the kprobe_multi bench test that attaches all
> > ftrace-able functions will race with bpf_dispatcher_update,
> > that calls bpf_arch_text_poke on bpf_dispatcher_xdp_func,
> > which is ftrace-able function.
> >
> > Ftrace is not aware of this update so this will cause
> > ftrace_bug with:
> >
> >    WARNING: CPU: 6 PID: 1985 at
> >    arch/x86/kernel/ftrace.c:94 ftrace_verify_code+0x27/0x50
> >    ...
> >    ftrace_replace_code+0xa3/0x170
> >    ftrace_modify_all_code+0xbd/0x150
> >    ftrace_startup_enable+0x3f/0x50
> >    ftrace_startup+0x98/0xf0
> >    register_ftrace_function+0x20/0x60
> >    register_fprobe_ips+0xbb/0xd0
> >    bpf_kprobe_multi_link_attach+0x179/0x430
> >    __sys_bpf+0x18a1/0x2440
> >    ...
> >    ------------[ ftrace bug ]------------
> >    ftrace failed to modify
> >    [<ffffffff818d9380>] bpf_dispatcher_xdp_func+0x0/0x10
> >     actual:   ffffffe9:7b:ffffff9c:77:1e
> >    Setting ftrace call site to call ftrace function
> >
> > It looks like we need some way to way to hide some functions
>
> need some way to hide some functions ...
>
> > from ftrace, but meanwhile we workaround this by skipping
> > bpf_dispatcher_xdp_func from kprobe_multi bench test.
> >
> > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> I tried with 32cpus on my local qemu/vm but cannot reproduce the crash.
> But look at the code, your should seem okay as bpf_dispatcher_xdp_func
> indeed could be poked and simplified. So with a few nits,
>
> Acked-by: Yonghong Song <yhs@fb.com>
>

Fixed typo and changed strncmp to strcmp, pushed to bpf-next.

> > ---
> >   tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c | 2 ++
> >   1 file changed, 2 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 5b93d5d0bd93..8c442051f312 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > @@ -364,6 +364,8 @@ static int get_syms(char ***symsp, size_t *cntp)
> >                       continue;
> >               if (!strncmp(name, "rcu_", 4))
> >                       continue;
> > +             if (!strncmp(name, "bpf_dispatcher_xdp_func", 23))
>
> ffffffff81b17a90 T bpf_dispatcher_xdp_func
>
> bpf_dispatcher_xdp_func is a full name, you can just use strcmp here.
> Further,
>
> linux/bpf.h:#define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_##name##_func
>
> Currently, bpf_dispatcher_xdp_func is the ONLY BPF_DISPATCHER_FUNC.
> So comparing bpf_dispatcher_xdp_func is enough. It would be good
> to add a comment to explain why not comparing to bpf_dispatcher_*_func.
>
> > +                     continue;
> >               if (!strncmp(name, "__ftrace_invalid_address__",
> >                            sizeof("__ftrace_invalid_address__") - 1))
> >                       continue;