From: Cong Wang <xiyou.wangcong@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Song Liu <song@kernel.org>, Andrii Nakryiko <andrii@kernel.org>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Cong Wang <cong.wang@bytedance.com>,
Daniel Borkmann <daniel@iogearbox.net>,
John Fastabend <john.fastabend@gmail.com>,
Jakub Sitnicki <jakub@cloudflare.com>
Subject: Re: [PATCH bpf] libbpf: fix missing section "sk_skb/skb_verdict"
Date: Tue, 30 Nov 2021 21:03:46 -0800 [thread overview]
Message-ID: <CAM_iQpV3+bYqw4n494-hc=3DcDkBpM8k9gd=iPwqR4X_Vw6LhQ@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzZUdE+gsgiLRRissh1Vskf2Ea4WT3gAseV1b9cvNnaBVQ@mail.gmail.com>
On Tue, Nov 30, 2021 at 8:33 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 8:19 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 3:33 PM Song Liu <song@kernel.org> wrote:
> > >
> > > On Mon, Nov 29, 2021 at 12:51 PM Cong Wang <xiyou.wangcong@gmail.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@gmail.com> wrote:
> > > > > >
> > > > > > From: Cong Wang <cong.wang@bytedance.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@iogearbox.net>
> > > > > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > > > > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > > > > > Signed-off-by: Cong Wang <cong.wang@bytedance.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". ;)
>
> 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 }
> 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.
>
> >
> > > 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? ;)
> 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".
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.
next prev parent reply other threads:[~2021-12-01 5:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-26 20:41 [PATCH bpf] libbpf: fix missing section "sk_skb/skb_verdict" Cong Wang
2021-11-27 0:20 ` Song Liu
2021-11-29 20:50 ` Cong Wang
2021-11-30 23:32 ` Song Liu
2021-12-01 4:19 ` Cong Wang
2021-12-01 4:33 ` Andrii Nakryiko
2021-12-01 5:03 ` Cong Wang [this message]
2021-12-01 5:38 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM_iQpV3+bYqw4n494-hc=3DcDkBpM8k9gd=iPwqR4X_Vw6LhQ@mail.gmail.com' \
--to=xiyou.wangcong@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=daniel@iogearbox.net \
--cc=jakub@cloudflare.com \
--cc=john.fastabend@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.