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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jul 10, 2020 at 1:37 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> 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.
>

I think we should just solve this for cgroup the same way you did it
for netns. We'll keep adding new attach types regardless, so better
solve the problem, rather than artificially avoid it.


> >
> > 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")

oh, right, which reminds me: how about adding / to sk_lookup in that
libbpf table, so that it's always sk_lookup/<something> for section
name? We did similar change to xdp_devmap recently, and it seems like
a good trend overall to have / separation between program type and
whatever extra name user wants to give?