Re: [PATCH bpf] bpf,perf: Fix perf_event_detach_bpf_prog error handling

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Dec 09, 2024 at 09:49:01AM -0800, Andrii Nakryiko wrote:

SNIP

> > > > ---
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index fe57dfbf2a86..d4b45543ebc2 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -2251,6 +2251,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
> > > >                 goto unlock;
> > > >
> > > >         old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
> > > > +       if (!old_array)
> > > > +               goto put;
> > >
> > > How does this inherited event stuff work? You can have two separate
> > > events sharing the same prog_array? What if we attach different
> > > programs to each of those events, will both of them be called for
> > > either of two events? That sounds broken, if that's true.
> >
> > so perf event with attr.inherit=1 attached on task will get inherited
> > by child process.. the new child event shares the parent's bpf program
> > and tp_event (hence prog_array) which is global for tracepoint
> >
> > AFAICS when child process exits the inherited event is destroyed and it
> > removes related tp_event->prog_array, so the parent event won't trigger
> > ever again, the test below shows that
> >
> 
> Doesn't this sound broken? Either event inheritance has to copy
> prog_array and make them completely independent. Or inherited event
> shouldn't remove the parent's program. Or something else, but the way
> it is right now seems wrong, no?
> 
> I'm not sure what's the most appropriate behavior that would match
> overall perf_event inheritance, but we should probably think about
> this and fix it, instead of patching up the symptom with that NULL
> check, no?
> 
> >   test_tp_attach:FAIL:executed unexpected executed: actual 1 != expected 2
> >
> > I'm not sure this is problem in practise, because nobody complained
> > about that ;-)
> 
> That's... not really a distinction of what is a problem or not ;)
> 
> >
> > libbpf does not set attr.inherit=1 and creates system wide perf event,
> > so no problem there
> 
> you can use all this outside of libbpf and lead to wrong behavior, so
> worth thinking about this and fixing, IMO

sure, let's fix that.. I like the solution where we let only the parent
to remove the program from prog_array looks good to me and is probably
simple enough.. but need to check what happens when parent dies first

I'll check on that, but perhaps we could go with the simple fix first
to fix the crash (it was the prior behaviour) and I'll send the fix on
top of that

jirka

> 
> >
> > jirka
> >
> >
> > ---
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 66173ddb5a2d..2e96241b5030 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -12430,8 +12430,9 @@ static int perf_event_open_tracepoint(const char *tp_category,
> >         attr.type = PERF_TYPE_TRACEPOINT;
> >         attr.size = attr_sz;
> >         attr.config = tp_id;
> > +       attr.inherit = 1;
> >
> > -       pfd = syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> > +       pfd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, 0 /* cpu */,
> >                       -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> >         if (pfd < 0) {
> >                 err = -errno;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach.c b/tools/testing/selftests/bpf/prog_tests/tp_attach.c
> > new file mode 100644
> > index 000000000000..01bbf1d1ab52
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/tp_attach.c
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +#include "tp_attach.skel.h"
> > +
> > +void test_tp_attach(void)
> > +{
> > +       struct tp_attach *skel;
> > +       int pid;
> > +
> > +       skel = tp_attach__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "tp_attach__open_and_load"))
> > +               return;
> > +
> > +       skel->bss->pid = getpid();
> > +
> > +       if (!ASSERT_OK(tp_attach__attach(skel), "tp_attach__attach"))
> > +               goto out;
> > +
> > +       getpid();
> > +
> > +       pid = fork();
> > +       if (!ASSERT_GE(pid, 0, "fork"))
> > +               goto out;
> > +       if (pid == 0)
> > +               _exit(0);
> > +       waitpid(pid, NULL, 0);
> > +
> > +       getpid();
> > +
> > +       ASSERT_EQ(skel->bss->executed, 2, "executed");
> > +
> > +out:
> > +       tp_attach__destroy(skel);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/tp_attach.c b/tools/testing/selftests/bpf/progs/tp_attach.c
> > new file mode 100644
> > index 000000000000..d9450d2eac17
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/tp_attach.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +int pid;
> > +int executed;
> > +
> > +SEC("tp/syscalls/sys_enter_getpid")
> > +int test(void *ctx)
> > +{
> > +       if (pid == (bpf_get_current_pid_tgid() >> 32))
> > +               executed++;
> > +       return 0;
> > +}