Re: [PATCH bpf-next v3 12/16] libbpf: Add support for SK_LOOKUP program type

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jakub Sitnicki <jakub@cloudflare.com>
Cc: bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	kernel-team <kernel-team@cloudflare.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH bpf-next v3 12/16] libbpf: Add support for SK_LOOKUP program type
Date: Thu, 9 Jul 2020 16:13:03 -0700	[thread overview]
Message-ID: <CAEf4BzY75c+gARvkmQ8OtbpDbZvBkia4qMyxO7HCoOeu=B1AxQ@mail.gmail.com> (raw)
In-Reply-To: <87h7ugadpt.fsf@cloudflare.com>

On Thu, Jul 9, 2020 at 8:51 AM Jakub Sitnicki <jakub@cloudflare.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@cloudflare.com> wrote:
> >>
> >> Make libbpf aware of the newly added program type, and assign it a
> >> section name.
> >>
> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.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.

>
> >
> >>  };
> >>

[...]