netdev - Re: [PATCH bpf-next v3 12/16] libbpf: Add support for SK

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAEf4BzY75c+gARvkmQ8OtbpDbZvBkia4qMyxO7HCoOeu=B1AxQ@mail.gmail.com>
Date:   Thu, 9 Jul 2020 16:13:03 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...udflare.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH bpf-next v3 12/16] libbpf: Add support for SK_LOOKUP
 program type

On Thu, Jul 9, 2020 at 8:51 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> On Thu, Jul 09, 2020 at 06:23 AM CEST, Andrii Nakryiko wrote:
> > On Thu, Jul 2, 2020 at 2:25 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
> >>
> >> Make libbpf aware of the newly added program type, and assign it a
> >> section name.
> >>
> >> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> >> ---
> >>
> >> Notes:
> >>     v3:
> >>     - Move new libbpf symbols to version 0.1.0.
> >>     - Set expected_attach_type in probe_load for new prog type.
> >>
> >>     v2:
> >>     - Add new libbpf symbols to version 0.0.9. (Andrii)
> >>
> >>  tools/lib/bpf/libbpf.c        | 3 +++
> >>  tools/lib/bpf/libbpf.h        | 2 ++
> >>  tools/lib/bpf/libbpf.map      | 2 ++
> >>  tools/lib/bpf/libbpf_probes.c | 3 +++
> >>  4 files changed, 10 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 4ea7f4f1a691..ddcbb5dd78df 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -6793,6 +6793,7 @@ BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
> >>  BPF_PROG_TYPE_FNS(tracing, BPF_PROG_TYPE_TRACING);
> >>  BPF_PROG_TYPE_FNS(struct_ops, BPF_PROG_TYPE_STRUCT_OPS);
> >>  BPF_PROG_TYPE_FNS(extension, BPF_PROG_TYPE_EXT);
> >> +BPF_PROG_TYPE_FNS(sk_lookup, BPF_PROG_TYPE_SK_LOOKUP);
> >>
> >>  enum bpf_attach_type
> >>  bpf_program__get_expected_attach_type(struct bpf_program *prog)
> >> @@ -6969,6 +6970,8 @@ static const struct bpf_sec_def section_defs[] = {
> >>         BPF_EAPROG_SEC("cgroup/setsockopt",     BPF_PROG_TYPE_CGROUP_SOCKOPT,
> >>                                                 BPF_CGROUP_SETSOCKOPT),
> >>         BPF_PROG_SEC("struct_ops",              BPF_PROG_TYPE_STRUCT_OPS),
> >> +       BPF_EAPROG_SEC("sk_lookup",             BPF_PROG_TYPE_SK_LOOKUP,
> >> +                                               BPF_SK_LOOKUP),
> >
> > So it's a BPF_PROG_TYPE_SK_LOOKUP with attach type BPF_SK_LOOKUP. What
> > other potential attach types could there be for
> > BPF_PROG_TYPE_SK_LOOKUP? How the section name will look like in that
> > case?
>
> BPF_PROG_TYPE_SK_LOOKUP won't have any other attach types that I can
> forsee. There is a single attach type shared by tcp4, tcp6, udp4, and
> udp6 hook points. If we hook it up in the future say to sctp, I expect
> the same attach point will be reused.

So you needed to add to bpf_attach_type just to fit into link_create
model of attach_type -> prog_type, right? As I mentioned extending
bpf_attach_type has a real cost on each cgroup, so we either need to
solve that problem (and I think that would be the best) or we can
change link_create logic to not require attach_type for programs like
SK_LOOKUP, where it's clear without attach type.

Second order question was if we have another attach type, having
SEC("sk_lookup/just_kidding_something_else") would be a bit weird :)
But it seems like that's not a concern.

>
> >
> >>  };
> >>

[...]