On Wed, Jun 26, 2019 at 7:25 AM Daniel Borkmann <dan...@iogearbox.net> wrote:
>
> On 06/21/2019 06:55 AM, Andrii Nakryiko wrote:
> > Add ability to attach to kernel and user probes and retprobes.
> > Implementation depends on perf event support for kprobes/uprobes.
> >
> > Signed-off-by: Andrii Nakryiko <andr...@fb.com>
> > ---
<snip>
> > +}
>
> I do like that we facilitate usage by adding these APIs to libbpf, but my
> $0.02
> would be that they should be designed slightly different. See it as a nit, but
> given it's exposed in libbpf.map and therefore immutable in future it's worth
> considering; right now with this set here you have:
>
> int bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> const char *func_name)
> int bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> pid_t pid, const char *binary_path,
> size_t func_offset)
> int bpf_program__attach_tracepoint(struct bpf_program *prog,
> const char *tp_category,
> const char *tp_name)
> int bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> const char *tp_name)
> int bpf_program__attach_perf_event(struct bpf_program *prog, int pfd)
> int libbpf_perf_event_disable_and_close(int pfd)
>
> So the idea is that all the bpf_program__attach_*() APIs return an fd that you
> can later on pass into libbpf_perf_event_disable_and_close(). I think there is
> a bit of a disconnect in that the bpf_program__attach_*() APIs try to do too
> many things at once. For example, the bpf_program__attach_raw_tracepoint() fd
> has nothing to do with perf, so passing to
> libbpf_perf_event_disable_and_close()
> kind of works, but is hacky since there's no PERF_EVENT_IOC_DISABLE for it so
> this
> would always error if a user cares to check the return code. In the kernel, we
Yeah, you are absolutely right, missed that it's not creating perf
event under cover, to be honest.
> use anon inode for this kind of object. Also, if a user tries to add more than
> one program to the same event, we need to recreate a new event fd every time.
>
> What this boils down to is that this should get a proper abstraction, e.g. as
> in struct libbpf_event which holds the event object. There should be helper
> functions like libbpf_event_create_{kprobe,uprobe,tracepoint,raw_tracepoint}
> returning
> such an struct libbpf_event object on success, and a single
> libbpf_event_destroy()
> that does the event specific teardown. bpf_program__attach_event() can then
> take
> care of only attaching the program to it. Having an object for this is also
> more
> extensible than just a fd number. Nice thing is that this can also be
> completely
> internal to libbpf.c as with struct bpf_program and other abstractions where
> we
> don't expose the internals in the public header.
Yeah, I totally agree, I think this is a great idea! I don't
particularly like "event" name, that seems very overloaded term. Do
you mind if I call this "bpf_hook" instead of "libbpf_event"? I've
always thought about these different points in the system to which one
can attach BPF program as hooks exposed from kernel :)
Would it also make sense to do attaching to non-tracing hooks using
the same mechanism (e.g., all the per-cgroup stuff, sysctl, etc)? Not
sure how people do that today, will check to see how it's done, but I
think nothing should conceptually prevent doing that using the same
abstract bpf_hook way, right?
>
> Thanks,
> Daniel
