Re: [PATCH dwarves v6 4/6] bpf: Create fentry/fexit/fmod_ret links through BPF_LINK_CREATE

[Kui-Feng Lee <kuifeng@fb.com>]

On Wed, 2022-04-20 at 10:49 -0700, Andrii Nakryiko wrote:
> On Fri, Apr 15, 2022 at 9:30 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > Make fentry/fexit/fmod_ret as valid attach-types for
> > BPF_LINK_CREATE.
> > Pass a cookie along with BPF_LINK_CREATE requests.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> 
> I think logically this patch should be #3 and current patch #3 adding
> cookie to UAPI should go after this. So it would make sense to swap
> them.

This patch modifies UAPI.  The current patch #3 set a cookie for
fentry/fexit/fmod_ret, but the value is always 0 (empty).

This patch provides a way to set cookies from the userland.

> 
> >  include/uapi/linux/bpf.h       | 7 +++++++
> >  kernel/bpf/syscall.c           | 9 +++++++++
> >  tools/include/uapi/linux/bpf.h | 7 +++++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index a4f557338af7..780be5a8ae39 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1490,6 +1490,13 @@ union bpf_attr {
> >                                 __aligned_u64   addrs;
> >                                 __aligned_u64   cookies;
> >                         } kprobe_multi;
> > +                       struct {
> > +                               /* black box user-provided value
> > passed through
> > +                                * to BPF program at the execution
> > time and
> > +                                * accessible through
> > bpf_get_attach_cookie() BPF helper
> > +                                */
> > +                               __u64           cookie;
> > +                       } tracing;
> >                 };
> >         } link_create;
> > 
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 966f2d40ae55..ca14b0a2e222 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3189,6 +3189,10 @@ attach_type_to_prog_type(enum
> > bpf_attach_type attach_type)
> >                 return BPF_PROG_TYPE_SK_LOOKUP;
> >         case BPF_XDP:
> >                 return BPF_PROG_TYPE_XDP;
> > +       case BPF_TRACE_FENTRY:
> > +       case BPF_TRACE_FEXIT:
> > +       case BPF_MODIFY_RETURN:
> > +               return BPF_PROG_TYPE_TRACING;
> 
> seems like
> 
>        case BPF_LSM_MAC:
>                return BPF_PROG_TYPE_LSM;
> 
> is missing?

Should I add cases for all attach types?
I thought it is intentionally to have cases only for supported attach
types.  For example, link_create() & bpf_prog_attach() returns earlier
if the returned type of this function is BPF_PROG_TYPE_UNSPEC. 

> 
> 
> Looking at my experiment for cleaning up RAW_TRACEPOINT_OPEN and
> LINK_CREATE, I think I also got rid of tracing_bpf_link_attach()
> altogether and there was extra case for BPF_PROG_TYPE_EXT.
> 
> How about this. Given I have an almost ready kernel code and I'd like
> libbpf to use LINK_CREATE if possible in all cases, let me add the
> feature-probing on libbpf side and post it as a separate small patch
> set that you can base your cookie-specific changes on top. That will
> let you concentrate on BPF cookie side and I'll handle the libbpf
> intricacies that are not directly related to your changes?
> 
> I'll try to post patches today or tomorrow, so it should not delay
> you much.

Sure! I will send you my branch.

> 
> 
> >         default:
> >                 return BPF_PROG_TYPE_UNSPEC;
> >         }
> > @@ -4254,6 +4258,11 @@ static int tracing_bpf_link_attach(const
> > union bpf_attr *attr, bpfptr_t uattr,
> >                                                attr-
> > >link_create.target_fd,
> >                                                attr-
> > >link_create.target_btf_id,
> >                                                0);
> > +       else if (prog->type == BPF_PROG_TYPE_TRACING)
> > +               return bpf_tracing_prog_attach(prog,
> > +                                              0,
> > +                                              0,
> > +                                              attr-
> > >link_create.tracing.cookie);
> > 
> >         return -EINVAL;
> >  }
> > diff --git a/tools/include/uapi/linux/bpf.h
> > b/tools/include/uapi/linux/bpf.h
> > index a4f557338af7..780be5a8ae39 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -1490,6 +1490,13 @@ union bpf_attr {
> >                                 __aligned_u64   addrs;
> >                                 __aligned_u64   cookies;
> >                         } kprobe_multi;
> > +                       struct {
> > +                               /* black box user-provided value
> > passed through
> > +                                * to BPF program at the execution
> > time and
> > +                                * accessible through
> > bpf_get_attach_cookie() BPF helper
> > +                                */
> > +                               __u64           cookie;
> > +                       } tracing;
> >                 };
> >         } link_create;
> > 
> > --
> > 2.30.2
> >