netdev - Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzY+nMbye8wkQjiUra7wHtWZ14aWO5kNwkQFQaj=6-qp9w@mail.gmail.com>
Date:   Wed, 16 Sep 2020 14:17:23 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs
 to multiple attach points

On Wed, Sep 16, 2020 at 2:13 PM Toke Høiland-Jørgensen <toke@...hat.com> 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
>