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@xxxxxx> 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@xxxxxx>
> > ---
> > 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
> >
