Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

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

On Tue, Apr 5, 2022 at 10:35 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-03-21 at 21:32 -0700, Andrii Nakryiko wrote:
> > On Mon, Mar 21, 2022 at 6:15 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > I remember I brought this up earlier, but I forgot the outcome.
> > > > What
> > > > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to
> > > > create all
> > > > the same links through more universal BPF_LINK_CREATE command.
> > > > And
> > > > only there we add bpf_cookie? There are few advantages:
> > > >
> > > > 1. We can separate raw_tracepoint and trampoline-based programs
> > > > more
> > > > cleanly in UAPI (it will be two separate structs:
> > > > link_create.raw_tp
> > > > with raw tracepoint name vs link_create.trampoline, or whatever
> > > > the
> > > > name, with cookie and stuff). Remember that raw_tp won't support
> > > > bpf_cookie for now, so it would be another advantage not to
> > > > promise
> > > > cookie in UAPI.
> > >
> > > What would it look like?
> > > Technically link_create has prog_fd and perf_event.bpf_cookie
> > > already.
> > >
> > >         case BPF_PROG_TYPE_TRACING:
> > >                 ret = tracing_bpf_link_attach(attr, uattr, prog);
> > > would just gain a few more checks for prog->expected_attach_type ?
> > >
> > > Then link_create cmd will be equivalent to raw_tp_open.
> > > With and without bpf_cookie.
> > > ?
> >
> > Yes, except I'd leave perf_event for perf_event-based attachments
> > (kprobe, uprobe, tracepoint) and would define a separate substruct
> > for
> > trampoline-based programs. Something like this (I only compile-tested
> > it, of course). I've also simplified prog_type/expected_attach_type
> > logic a bit because it felt like a total maze to me and I was getting
> > lost all the time. Gmail will probably corrupt all the whitespaces,
> > sorry about that in advance.
> >
> > Seems like we could already attach BPF_PROG_TYPE_EXT both through
> > RAW_TRACEPOINT_OPEN and LINK_CREATE, I didn't realize that. The
> > "patch" below leaves raw_tp handling
> > (BPF_PROG_TYPE_TRACING+BPF_TRACE_RAW_TP,
> > BPF_PROG_TYPE_RAW_TRACEPOINT,
> > and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) in RAW_TRACEPOINT_OPEN. If
> > we want to completely unify all the bpf_link creations under
> > LINK_CREATE, see extra small "patch" at the very bottom.
>
> I just implemented and tested a patch of tracing links with
> bpf_link_create, so it can be done with both raw_tp_open and
> bpf_link_create.
>

Nice, please send it as part of your cookie patch set in a separate patch.

> Do we want to remove raw_tp_open() eventually?  Should I remove
> raw_tp_open() supports of cookies?

We can't remove existing Linux UAPI, but we can stop extending them.
So I'd say let's add cookie only through CREATE_LINK and leave
RAW_TRACEPOINT_OPEN as is.

>
>