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] [day] [month] [year] [list]

Message-ID: <CAEf4Bzaby8iCwEWHPxiJn2PAf6iOyhgcm_-pfR75z9UvXqk2GA@mail.gmail.com>
Date:   Wed, 22 Jul 2020 17:56:47 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...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 Wed, Jul 22, 2020 at 5:32 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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.

ok, sounds good to me