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]

Message-ID: <a24b3328-2d4a-17f2-3aa3-756af7432e6b@iogearbox.net>
Date:   Fri, 5 Jul 2019 22:46:21 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
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 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!