Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes: > 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). Right, makes sense; will do that in the next version! -Toke
