netdev - Re: [PATCH v5 bpf-next 4/9] libbpf: add kprobe/uprobe attach API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Jul 2019 20:48:05 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH v5 bpf-next 4/9] libbpf: add kprobe/uprobe attach API

On Fri, Jul 5, 2019 at 1:46 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 07/04/2019 02:57 AM, Andrii Nakryiko wrote:
> > On Wed, Jul 3, 2019 at 9:47 AM Andrii Nakryiko
> > <andrii.nakryiko@...il.com> wrote:
> [...]
> >>   [1] https://lore.kernel.org/bpf/20190621045555.4152743-4-andriin@fb.com/T/#m6cfc141e7b57970bc948134bf671a46972b95134
> >>
> >>> with bpf_link with destructor looks good to me, but my feedback from back then was
> >>> that all the kprobe/uprobe/tracepoint/raw_tracepoint should be split API-wise, so
> >>> you'll end up with something like the below, that is, 1) a set of functions that
> >>> only /create/ the bpf_link handle /once/, and 2) a helper that allows /attaching/
> >>> progs to one or multiple bpf_links. The set of APIs would look like:
> >>>
> >>> struct bpf_link *bpf_link__create_kprobe(bool retprobe, const char *func_name);
> >>> struct bpf_link *bpf_link__create_uprobe(bool retprobe, pid_t pid,
> >>>                                          const char *binary_path,
> >>>                                          size_t func_offset);
> >>> int bpf_program__attach_to_link(struct bpf_link *link, struct bpf_program *prog);
> >>> int bpf_link__destroy(struct bpf_link *link);
> >>>
> >>> This seems much more natural to me. Right now you sort of do both in one single API.
> >>
> >> It felt that way for me as well, until I implemented it and used it in
> >> selftests. And then it felt unnecessarily verbose without giving any
> >> benefit. I still have a local patchset with that change, I can post it
> >> as RFC, if you don't trust my judgement. Please let me know.
> >>
> >>> Detangling the bpf_program__attach_{uprobe,kprobe}() would also avoid that you have
> >>> to redo all the perf_event_open_probe() work over and over in order to get the pfd
> >
> > So re-reading this again, I wonder if you meant that with separate
> > bpf_link (or rather bpf_hook in that case) creation and attachment
> > operations, one would be able to create single bpf_hook for same
> > kprobe and then attach multiple BPF programs to that single pfd
> > representing that specific probe.
> >
> > If that's how I should have read it, I agree that it probably would be
> > possible for some types of hooks, but not for every type of hook. But
> > furthermore, how often in practice same application attaches many
> > different BPF programs to the same hook? And it's also hard to imagine
> > that hook creation (i.e., creating such FD for BPF hook), would ever
> > be a bottleneck.
> >
> > So I still think it's not a strong reason to go with API that's harder
> > to use for typical use cases just because of hypothetical benefits in
> > some extreme cases.
>
> Was thinking along that lines, yes, as we run over an array of BPF progs,
> but I just double checked the kernel code again and the relationship of
> a BPF prog to perf_event is really just 1:1, just that the backing tp_event
> (trace_event_call) contains the shared array. Given that, all makes sense
> and there is no point in splitting. Therefore, applied, thanks!

Great, thanks a lot!