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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Fri, Jul 10, 2020 at 01:13 AM CEST, Andrii Nakryiko wrote:
> 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.

Right. I was thinking about that a bit. For prog types map 1:1 to an
attach type, like flow_dissector or proposed sk_lookup, we don't really
to know the attach type to attach the program.

PROG_QUERY is more problematic though. But I imagine we could introduce
a flag like BPF_QUERY_F_BY_PROG_TYPE that would make the kernel
interpret attr->query.attach_type as prog type.

PROG_DETACH is yet another story but sk_lookup uses only link-based
attachment, so I'm ignoring it here.

What also might get in the way is the fact that there is no
bpf_attach_type value reserved for unspecified attach type at the
moment. We would have to ensure that the first enum,
BPF_CGROUP_INET_INGRESS, is not treated as an exact 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.

Yes. Sorry, I didn't mean to leave it unanswered. Just assumed that it
was obvious that it's not the case.

I've been happily using the part of section name following "sk_lookup"
prefix to name the programs just to make section names in ELF object
unique:

  SEC("sk_lookup/lookup_pass")
  SEC("sk_lookup/lookup_drop")
  SEC("sk_lookup/redir_port")