netdev - Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

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:   Wed, 22 Jul 2020 17:32:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs
 to multiple attach points

On Tue, Jul 21, 2020 at 11:02:04PM -0700, Andrii Nakryiko wrote:
> 
> Just one technical moment, let me double-check my understanding again.
> You seem to be favoring pre-creating bpf_tracing_link because there is
> both tgt_prog (that we refcnt on EXT prog load) and we also lookup and
> initialize trampoline in check_attach_btf_id(). Of course there is
> also expected_attach_type, but that's a trivial known enum, so I'm
> ignoring it. So because we have those two entities which on attach are
> supposed to be owned by bpf_tracing_link, you just want to pre-create
> a "shell" of bpf_tracing_link, and then on attach complete its
> initialization, is that right? That certainly simplifies attach logic
> a bit and I think it's fine.

Right. It just feels cleaner to group objects for the same purpose.

> But also it seems like we'll be creating and initializing a
> **different** trampoline on re-attach to prog Y. Now attach will do
> different things depending on whether tgt_prog_fd is provided or not.

Right, but it can be a common helper instead that is creating a 'shell'
of bpf_tracing_link.
Calling it from prog_load and from raw_tp_open is imo clean enough.
No copy paste of code.
If that was the concern.

> So I wonder why not just unify this trampoline initialization and do
> it at attach time? For all valid EXT use cases today the result is the
> same: everything still works the same. For cases where we for some
> reason can't initialize bpf_trampoline, that failure will happen at
> attach time, not on a load time. But that seems fine, because that's
> going to be the case for re-attach (with tgt_prog_fd) anyways. Looking
> through the verifier code, it doesn't seem like it does anything much
> with prog->aux->trampoline, unless I missed something, so it must be
> ok to do it after load? It also seems to avoid this double BTF
> validation concern you have, no? Thoughts?

bpf_trampoline_link_prog() is attach time call.
but bpf_trampoline_lookup() is one to one with the target.
When load_prog holds the target it's a right time to prep all things
about the target. Notice that key into trampoline_lookup() is
key = ((u64)aux->id) << 32 | btf_id;
of the target prog.
Can it be done at raw_tp_open time?
I guess so, but feels kinda weird to me to split the target preparation
job into several places.