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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.