On Wed, Sep 16, 2020 at 2:13 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > > [ will fix all your comments above ] > > >> @@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog * > >> prog->expected_attach_type == BPF_TRACE_ITER) > >> return bpf_iter_link_attach(attr, prog); > >> > >> + if (attr->link_create.attach_type == BPF_TRACE_FREPLACE && > >> + !prog->expected_attach_type) > >> + return bpf_tracing_prog_attach(prog, > >> + attr->link_create.target_fd, > >> + attr->link_create.target_btf_id); > > > > Hm.. so you added a "fake" BPF_TRACE_FREPLACE attach_type, which is > > not really set with BPF_PROG_TYPE_EXT and is only specified for the > > LINK_CREATE command. Are you just trying to satisfy the link_create > > flow of going from attach_type to program type? If that's the only > > reason, I think we can adjust link_create code to handle this more > > flexibly. > > > > I need to think a bit more whether we want BPF_TRACE_FREPLACE at all, > > but if we do, whether we should make it an expected_attach_type for > > BPF_PROG_TYPE_EXT then... > > Yeah, wasn't too sure about this. But attach_type seemed to be the only > way to disambiguate between the different link types in the LINK_CREATE > command, so went with that. Didn't think too much about it, TBH :) having extra attach types has real costs in terms of memory (in cgroup land), which no one ever got to fixing yet. And then prog->expected_attach_type != link's expected_attach_type looks weird and wrong and who knows which bugs we'll get later because of this. > > I guess an alternative could be to just enforce attach_type==0 and look > at prog->type? Or if you have any other ideas, I'm all ears! Right, we have prog fd, so can get it (regardless of type), then do switch by type, then translate expected attach type to prog type and see if it matches, but only for program types that care (which right now is all but tracing, where it's obvious from prog_type alone, I think). > > -Toke >
