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.
