Re: [PATCH bpf-next v5 3/5] bpf, x86: Attach a cookie to fentry/fexit/fmod_ret.

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

On Tue, 2022-04-12 at 20:03 -0700, Andrii Nakryiko wrote:
> On Tue, Apr 12, 2022 at 9:56 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > Add a bpf_cookie field to struct bpf_tracing_link to attach a
> > cookie
> > to a link of a trace hook.  A cookie of a bpf_tracing_link is
> > available by calling bpf_get_attach_cookie when running the BPF
> > program of the attached link.
> > 
> > The value of a cookie will be set at bpf_tramp_run_ctx by the
> > trampoline of the link.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c    | 11 +++++++++--
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  8 ++++++++
> >  kernel/bpf/syscall.c           | 27 +++++++++++++++++++++++----
> >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  7 +++++++
> >  6 files changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c
> > b/arch/x86/net/bpf_jit_comp.c
> > index 0f521be68f7b..18d02dcd810a 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1764,13 +1764,20 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> >                            struct bpf_tramp_link *l, int
> > stack_size,
> >                            bool save_ret)
> >  {
> > +       u64 cookie = 0;
> >         u8 *prog = *pprog;
> >         u8 *jmp_insn;
> >         int ctx_cookie_off = offsetof(struct bpf_tramp_run_ctx,
> > bpf_cookie);
> >         struct bpf_prog *p = l->link.prog;
> > 
> > -       /* mov rdi, 0 */
> > -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > +       if (l->link.type == BPF_LINK_TYPE_TRACING) {
> > +               struct bpf_tracing_link *tr_link =
> > +                       container_of(l, struct bpf_tracing_link,
> > link);
> 
> empty line between variable declaration block and statement

Got it!

> 
> > +               cookie = tr_link->cookie;
> > +       }
> > +
> > +       /* mov rdi, cookie */
> > +       emit_mov_imm64(&prog, BPF_REG_1, (long) cookie >> 32, (u32)
> > (long) cookie);
> > 
> >         /* Prepare struct bpf_tramp_run_ctx.
> >          *
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d87df049e6b1..49db9c065701 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1062,6 +1062,7 @@ struct bpf_tracing_link {
> >         enum bpf_attach_type attach_type;
> >         struct bpf_trampoline *trampoline;
> >         struct bpf_prog *tgt_prog;
> > +       u64 cookie;
> >  };
> > 
> >  struct bpf_link_primer {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index a4f557338af7..5e901e6d17ea 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1436,6 +1436,7 @@ union bpf_attr {
> >         struct { /* anonymous struct used by
> > BPF_RAW_TRACEPOINT_OPEN command */
> >                 __u64 name;
> >                 __u32 prog_fd;
> > +               __u64 bpf_cookie;
> 
> I thought we are not adding bpf_cookie through RAW_TRACEPOINT_OPEN?

Yes, this line should be removed.

> 
> >         } raw_tracepoint;
> > 
> >         struct { /* anonymous struct for BPF_BTF_LOAD */
> > @@ -1490,6 +1491,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           bpf_cookie;
> > +                       } tracing;
> >                 };
> >         } link_create;
> > 
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 56e69a582b21..53d4da5c76b5 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2695,9 +2695,10 @@ static const struct bpf_link_ops
> > bpf_tracing_link_lops = {
> >         .fill_link_info = bpf_tracing_link_fill_link_info,
> >  };
> > 
> > -static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> > -                                  int tgt_prog_fd,
> > -                                  u32 btf_id)
> > +static int bpf_tracing_prog_attach_cookie(struct bpf_prog *prog,
> > +                                         int tgt_prog_fd,
> > +                                         u32 btf_id,
> > +                                         u64 bpf_cookie)
> 
> let's keep it as bpf_tracing_prog_attach and just update all the call
> sites

Roger

> 
> >  {
> >         struct bpf_link_primer link_primer;
> >         struct bpf_prog *tgt_prog = NULL;
> > @@ -2762,6 +2763,7 @@ static int bpf_tracing_prog_attach(struct
> > bpf_prog *prog,
> >         bpf_link_init(&link->link.link, BPF_LINK_TYPE_TRACING,
> >                       &bpf_tracing_link_lops, prog);
> >         link->attach_type = prog->expected_attach_type;
> > +       link->cookie = bpf_cookie;
> > 
> >         mutex_lock(&prog->aux->dst_mutex);
> > 
> > @@ -2871,6 +2873,13 @@ static int bpf_tracing_prog_attach(struct
> > bpf_prog *prog,
> >         return err;
> >  }
> > 
> > +static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> > +                                  int tgt_prog_fd,
> > +                                  u32 btf_id)
> > +{
> > +       return bpf_tracing_prog_attach_cookie(prog, tgt_prog_fd,
> > btf_id, 0);
> > +}
> > https://docs.google.com/document/d/1FYYdi1Hw4pjulbkvI1VmG-kGqTJRCg7bh1vAF4bwjMc/edit?usp=sharing
> > +
> 
> why this wrapper, just add that 0 for all existing
> bpf_tracing_prog_attach() invocations

Got it!

> 
> 
> >  struct bpf_raw_tp_link {
> >         struct bpf_link link;
> >         struct bpf_raw_event_map *btp;
> > @@ -3023,7 +3032,7 @@ static int bpf_perf_link_attach(const union
> > bpf_attr *attr, struct bpf_prog *pro
> >  }
> >  #endif /* CONFIG_PERF_EVENTS */
> > 
> > -#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
> > +#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD
> > raw_tracepoint.bpf_cookie
> > 
> >  static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >  {
> > @@ -3187,6 +3196,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;
> 
> can you please split the part that adds ability to create
> fentry/fexit/etc links through LINK_CREATE into a separate commit,
> separate from wiring the bpf_cookie? It's two logically distinct
> changes.

No problem.

> 
> >         default:
> >                 return BPF_PROG_TYPE_UNSPEC;
> >         }
> 
> [...]
> 
> > diff --git a/tools/include/uapi/linux/bpf.h
> > b/tools/include/uapi/linux/bpf.h
> > index a4f557338af7..a5ee57f09e04 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           bpf_cookie;
> 
> for kprobe_multi we used "cookies", so let's do "cookie" here? We'll
> have to keep perf_event.bpf_cookie, of course.

Got it!

> 
> > +                       } tracing;
> >                 };
> >         } link_create;
> > 
> > --
> > 2.30.2
> >