Date:   Tue, 30 Nov 2021 21:38:37 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Song Liu <song@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Cong Wang <cong.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [PATCH bpf] libbpf: fix missing section "sk_skb/skb_verdict"

On Tue, Nov 30, 2021 at 9:03 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Tue, Nov 30, 2021 at 8:33 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 8:19 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 3:33 PM Song Liu <song@...nel.org> wrote:
> > > >
> > > > On Mon, Nov 29, 2021 at 12:51 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > > > >
> > > > > On Fri, Nov 26, 2021 at 04:20:34PM -0800, Song Liu wrote:
> > > > > > On Fri, Nov 26, 2021 at 12:45 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > > > > > >
> > > > > > > From: Cong Wang <cong.wang@...edance.com>
> > > > > > >
> > > > > > > When BPF_SK_SKB_VERDICT was introduced, I forgot to add
> > > > > > > a section mapping for it in libbpf.
> > > > > > >
> > > > > > > Fixes: a7ba4558e69a ("sock_map: Introduce BPF_SK_SKB_VERDICT")
> > > > > > > Cc: Daniel Borkmann <daniel@...earbox.net>
> > > > > > > Cc: John Fastabend <john.fastabend@...il.com>
> > > > > > > Cc: Jakub Sitnicki <jakub@...udflare.com>
> > > > > > > Signed-off-by: Cong Wang <cong.wang@...edance.com>
> > > > > >
> > > > > > The patch looks good to me. But seems the selftests are OK without this. So,
> > > > > > do we really need this?
> > > > > >
> > > > >
> > > > > Not sure if I understand this question.
> > > > >
> > > > > At least BPF_SK_SKB_STREAM_PARSER and BPF_SK_SKB_STREAM_VERDICT are already
> > > > > there, so either we should remove all of them or add BPF_SK_SKB_VERDICT for
> > > > > completeness.
> > > > >
> > > > > Or are you suggesting we should change it back in selftests too? Note, it was
> > > > > changed by Andrii in commit 15669e1dcd75fe6d51e495f8479222b5884665b6:
> > > > >
> > > > > -SEC("sk_skb/skb_verdict")
> > > > > +SEC("sk_skb")
> > > >
> > > > Yes, I noticed that Andrii made the change, and it seems to work
> > > > as-is. Therefore,
> > > > I had the question "do we really need it".
> > >
> > > Same question from me: why still keep sk_skb/stream_parser and
> > > sk_skb/stream_verdict? ;) I don't see any reason these two are more
> > > special than sk_skb/skb_verdict, therefore we should either keep all
> > > of them or remove all of them.
> > >
> >
> > "sk_skb/skb_verdict" was treated by libbpf *exactly* the same way as
> > "sk_skb". Which means the attach type was set to BPF_PROG_TYPE_SK_SKB
> > and expected_attach_type was 0 (not BPF_SK_SKB_VERDICT!). So that
> > program is definitely not a BPF_SK_SKB_VERDICT, libbpf pre-1.0 just
> > has a sloppy prefix matching logic.
>
> This is exactly what I meant by "umbrella". ;)

You were asking why keep sk_skb/stream_verdict and
sk_skb/stream_parser and how it's different from sk_skb/skb_verdict.
The first two set expected_attach_type, the latter doesn't. Kernel
currently doesn't enforce extected_attach_type for SK_SKB prog type,
but that might change in the future.

>
> >
> > So Song's point is valid, we currently don't have selftests that tests
> > BPF_SK_SKB_VERDICT expected attach type, so it would be good to add
> > it. Or make sure that existing test that was supposed to test it is
> > actually testing it.
>
> Sure, I just noticed we have section name tests a few minutes ago. Will add
> it in V2.
>
> >
> > > >
> > > > If we do need to differentiate skb_verdict from just sk_skb, could you
> > >
> > > Are you sure sk_skb is a real attach type?? To me, it is an umbrella to
> > > catch all of them:
> > >
> > > SEC_DEF("sk_skb",               SK_SKB, 0, SEC_NONE | SEC_SLOPPY_PFX),
> > >
> > > whose expected_attach_type is 0. The reason why it works is
> > > probably because we don't check BPF_PROG_TYPE_SK_SKB in
> > > bpf_prog_load_check_attach().
> >
> > We don't check expected_attach_type in prog_load, but
>
> I see many checks in bpf_prog_load_check_attach(), for instance:
>
> 2084         switch (prog_type) {
> 2085         case BPF_PROG_TYPE_CGROUP_SOCK:
> 2086                 switch (expected_attach_type) {
> 2087                 case BPF_CGROUP_INET_SOCK_CREATE:
> 2088                 case BPF_CGROUP_INET_SOCK_RELEASE:
> 2089                 case BPF_CGROUP_INET4_POST_BIND:
> 2090                 case BPF_CGROUP_INET6_POST_BIND:
> 2091                         return 0;
> 2092                 default:
> 2093                         return -EINVAL;
> 2094                 }

I meant specifically for BPF_PROG_TYPE_SK_SKB, for which kernel
doesn't check or enforce expected_attach_type, as far as I can see
from the code.

>
>
> > sock_map_prog_update in net/core/sock_map.c is checking expected
> > attach type and should return -EOPNOTSUPP. But given that no test is
> > failing our tests don't even try to attach anything, I assume. Which
> > makes them not so great at actually testing anything. Please see if
> > you can improve that.
>
> sock_map_prog_update() checks for attach_type, not
> expected_attach_type.

Right, but shouldn't it make sure that attach_type ==
expected_attach_type? Otherwise what's even the point of
expected_attach_type?

>
> >
> > >
> > > > please add a
> > > > case selftest for skb_verdict?
> > >
> > > Ah, sure, I didn't know we have sec_name_test.
> > >
> > > >
> > > > Also, maybe we can name it as "sk_skb/verdict" to avoid duplication?
> > >
> > > At least we used to call it sk_skb/skb_verdict before commit 15669e1dcd.
> >
> > As I mentioned above, it could have been called "sk_skb!dontcare" and
>
> So why commit c6f6851b28ae26000352598f01968b3ff7dcf58 if your point
> here is we don't need any name? ;)

If kernel doesn't and *shoulnd't* care about expected_attach_type,
then maybe there is no point in supporting those names. I'm not
familiar with SK_SKB prog type, so I can't really answer. Given what
we do with CGROUP prog types and their expected attach types, I'd say
that probably the right thing is to enforce that in the kernel. But
again, opinions of others are welcome.

>
> > that would still work (and still does if strict mode is not enabled
> > for libbpf). For consistency with UAPI expected_attach_type enum it
> > should be called "sk_skb/verdict" because BPF_SK_SKB_VERDICT vs
> > BPF_SK_SKB_STREAM_VERDICT vs BPF_SK_SKB_STREAM_PARSER.
>
> To me, "verdict" is too broad, it could refer "stream_verdict" or "skb_verdict".

It's not "verdict" in isolation, it's "sk_skb/verdict". You yourself
added BPF_SK_SKB_VERDICT in a7ba4558e69a ("sock_map: Introduce
BPF_SK_SKB_VERDICT"), so I suppose that wasn't too broad at that time.
Now it's part of kernel UAPI, and consistency takes priority.

> And let me quote commit c6f6851b28ae26000352598f01968b3ff7dcf588:
>
>     "stream_parser" and "stream_verdict" are used instead of simple "parser"
>     and "verdict" just to avoid possible confusion in a place where attach
>     type is used alone (e.g. in bpftool's show sub-commands) since there is
>     another attach point that can be named as "verdict": BPF_SK_MSG_VERDICT.
>
> Thanks.

Powered by blists - more mailing lists