On Wed, Jul 22, 2020 at 5:32 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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
