From: sdf@google.com
To: Yonghong Song <yhs@meta.com>
Cc: Jiri Olsa <olsajiri@gmail.com>,
bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
haoluo@google.com
Subject: Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable
Date: Wed, 23 Nov 2022 09:17:33 -0800 [thread overview]
Message-ID: <Y35VrXvKBFg2RJ7y@google.com> (raw)
In-Reply-To: <34cb2b2f-ac3b-65c4-c479-0c4ed3dda096@meta.com>
On 11/23, Yonghong Song wrote:
> On 11/23/22 4:23 AM, Jiri Olsa wrote:
> > On Mon, Nov 21, 2022 at 10:03:40AM -0800, Stanislav Fomichev wrote:
> > > LWT_XMIT to test L3 case, TC to test L2 case.
> > >
> > > v2:
> > > - s/veth_ifindex/ipip_ifindex/ in two places (Martin)
> > > - add comment about which condition triggers the rejection (Martin)
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >
> > hi,
> > I'm getting selftest fails and it looks like it's because of this test:
> >
> > [root@qemu bpf]# ./test_progs -n 62,98
> > #62 empty_skb:OK
> > execute_one_variant:PASS:skel_open 0 nsec
> > execute_one_variant:PASS:my_pid_map_update 0 nsec
> > libbpf: failed to determine tracepoint 'raw_syscalls/sys_enter' perf
> event ID: No such file or directory
> > libbpf: prog 'handle_legacy': failed to create
> tracepoint 'raw_syscalls/sys_enter' perf event: No such file or directory
> > libbpf: prog 'handle_legacy': failed to auto-attach: -2
> > execute_one_variant:FAIL:skel_attach unexpected error: -2 (errno 2)
> > test_legacy_printk:FAIL:legacy_case unexpected error: -2 (errno 2)
> > execute_one_variant:PASS:skel_open 0 nsec
> > libbpf: failed to determine tracepoint 'raw_syscalls/sys_enter' perf
> event ID: No such file or directory
> > libbpf: prog 'handle_modern': failed to create
> tracepoint 'raw_syscalls/sys_enter' perf event: No such file or directory
> > libbpf: prog 'handle_modern': failed to auto-attach: -2
> > execute_one_variant:FAIL:skel_attach unexpected error: -2 (errno 2)
> > #98 legacy_printk:FAIL
> >
> > All error logs:
> > execute_one_variant:PASS:skel_open 0 nsec
> > execute_one_variant:PASS:my_pid_map_update 0 nsec
> > libbpf: failed to determine tracepoint 'raw_syscalls/sys_enter' perf
> event ID: No such file or directory
> > libbpf: prog 'handle_legacy': failed to create
> tracepoint 'raw_syscalls/sys_enter' perf event: No such file or directory
> > libbpf: prog 'handle_legacy': failed to auto-attach: -2
> > execute_one_variant:FAIL:skel_attach unexpected error: -2 (errno 2)
> > test_legacy_printk:FAIL:legacy_case unexpected error: -2 (errno 2)
> > execute_one_variant:PASS:skel_open 0 nsec
> > libbpf: failed to determine tracepoint 'raw_syscalls/sys_enter' perf
> event ID: No such file or directory
> > libbpf: prog 'handle_modern': failed to create
> tracepoint 'raw_syscalls/sys_enter' perf event: No such file or directory
> > libbpf: prog 'handle_modern': failed to auto-attach: -2
> > execute_one_variant:FAIL:skel_attach unexpected error: -2 (errno 2)
> > #98 legacy_printk:FAIL
> > Summary: 1/0 PASSED, 0 SKIPPED, 1 FAILED
> >
> > when I run separately it passes:
> >
> > [root@qemu bpf]# ./test_progs -n 98
> > #98 legacy_printk:OK
> > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >
> >
> > it seems that the open_netns/close_netns does not work properly,
> > and screw up access to tracefs for following tests
> >
> > if I comment out all the umounts in setns_by_fd, it does not fail
> Agreed with the above observations.
> With the current bpf-next, I can easily hit the above perf event ID issue.
> But if I backout the following two patches:
> 68f8e3d4b916531ea3bb8b83e35138cf78f2fce5 selftests/bpf: Make sure zero-len
> skbs aren't redirectable
> 114039b342014680911c35bd6b72624180fd669a bpf: Move skb->len == 0 checks
> into
> __bpf_redirect
> and run a few times with './test_progs -j' and I didn't hit any issues.
My guess would be that we need to remount debugfs in setns_by_fd?
diff --git a/tools/testing/selftests/bpf/network_helpers.c
b/tools/testing/selftests/bpf/network_helpers.c
index bec15558fd93..1f37adff7632 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -426,6 +426,10 @@ static int setns_by_fd(int nsfd)
if (!ASSERT_OK(err, "mount /sys/fs/bpf"))
return err;
+ err = mount("debugfs", "/sys/kernel/debug", "debugfs", 0, NULL);
+ if (!ASSERT_OK(err, "mount /sys/kernel/debug"))
+ return err;
+
return 0;
}
> >
> > jirka
> >
> >
> > > ---
> > > .../selftests/bpf/prog_tests/empty_skb.c | 146
> ++++++++++++++++++
> > > tools/testing/selftests/bpf/progs/empty_skb.c | 37 +++++
> > > 2 files changed, 183 insertions(+)
> > > create mode 100644
> tools/testing/selftests/bpf/prog_tests/empty_skb.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/empty_skb.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> > > new file mode 100644
> > > index 000000000000..32dd731e9070
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> > > @@ -0,0 +1,146 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <test_progs.h>
> > > +#include <network_helpers.h>
> > > +#include <net/if.h>
> > > +#include "empty_skb.skel.h"
> > > +
> > > +#define SYS(cmd) ({ \
> > > + if (!ASSERT_OK(system(cmd), (cmd))) \
> > > + goto out; \
> > > +})
> > > +
> > > +void test_empty_skb(void)
> > > +{
> > > + LIBBPF_OPTS(bpf_test_run_opts, tattr);
> > > + struct empty_skb *bpf_obj = NULL;
> > > + struct nstoken *tok = NULL;
> > > + struct bpf_program *prog;
> > > + char eth_hlen_pp[15];
> > > + char eth_hlen[14];
> > > + int veth_ifindex;
> > > + int ipip_ifindex;
> > > + int err;
> > > + int i;
> > > +
> > > + struct {
> > > + const char *msg;
> > > + const void *data_in;
> > > + __u32 data_size_in;
> > > + int *ifindex;
> > > + int err;
> > > + int ret;
> > > + bool success_on_tc;
> > > + } tests[] = {
> > > + /* Empty packets are always rejected. */
> > > +
> > > + {
> > > + /* BPF_PROG_RUN ETH_HLEN size check */
> > > + .msg = "veth empty ingress packet",
> > > + .data_in = NULL,
> > > + .data_size_in = 0,
> > > + .ifindex = &veth_ifindex,
> > > + .err = -EINVAL,
> > > + },
> > > + {
> > > + /* BPF_PROG_RUN ETH_HLEN size check */
> > > + .msg = "ipip empty ingress packet",
> > > + .data_in = NULL,
> > > + .data_size_in = 0,
> > > + .ifindex = &ipip_ifindex,
> > > + .err = -EINVAL,
> > > + },
> > > +
> > > + /* ETH_HLEN-sized packets:
> > > + * - can not be redirected at LWT_XMIT
> > > + * - can be redirected at TC to non-tunneling dest
> > > + */
> > > +
> > > + {
> > > + /* __bpf_redirect_common */
> > > + .msg = "veth ETH_HLEN packet ingress",
> > > + .data_in = eth_hlen,
> > > + .data_size_in = sizeof(eth_hlen),
> > > + .ifindex = &veth_ifindex,
> > > + .ret = -ERANGE,
> > > + .success_on_tc = true,
> > > + },
> > > + {
> > > + /* __bpf_redirect_no_mac
> > > + *
> > > + * lwt: skb->len=0 <= skb_network_offset=0
> > > + * tc: skb->len=14 <= skb_network_offset=14
> > > + */
> > > + .msg = "ipip ETH_HLEN packet ingress",
> > > + .data_in = eth_hlen,
> > > + .data_size_in = sizeof(eth_hlen),
> > > + .ifindex = &ipip_ifindex,
> > > + .ret = -ERANGE,
> > > + },
> > > +
> > > + /* ETH_HLEN+1-sized packet should be redirected. */
> > > +
> > > + {
> > > + .msg = "veth ETH_HLEN+1 packet ingress",
> > > + .data_in = eth_hlen_pp,
> > > + .data_size_in = sizeof(eth_hlen_pp),
> > > + .ifindex = &veth_ifindex,
> > > + },
> > > + {
> > > + .msg = "ipip ETH_HLEN+1 packet ingress",
> > > + .data_in = eth_hlen_pp,
> > > + .data_size_in = sizeof(eth_hlen_pp),
> > > + .ifindex = &ipip_ifindex,
> > > + },
> > > + };
> > > +
> > > + SYS("ip netns add empty_skb");
> > > + tok = open_netns("empty_skb");
> > > + SYS("ip link add veth0 type veth peer veth1");
> > > + SYS("ip link set dev veth0 up");
> > > + SYS("ip link set dev veth1 up");
> > > + SYS("ip addr add 10.0.0.1/8 dev veth0");
> > > + SYS("ip addr add 10.0.0.2/8 dev veth1");
> > > + veth_ifindex = if_nametoindex("veth0");
> > > +
> > > + SYS("ip link add ipip0 type ipip local 10.0.0.1 remote 10.0.0.2");
> > > + SYS("ip link set ipip0 up");
> > > + SYS("ip addr add 192.168.1.1/16 dev ipip0");
> > > + ipip_ifindex = if_nametoindex("ipip0");
> > > +
> > > + bpf_obj = empty_skb__open_and_load();
> > > + if (!ASSERT_OK_PTR(bpf_obj, "open skeleton"))
> > > + goto out;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > > + bpf_object__for_each_program(prog, bpf_obj->obj) {
> > > + char buf[128];
> > > + bool at_tc = !strncmp(bpf_program__section_name(prog), "tc", 2);
> > > +
> > > + tattr.data_in = tests[i].data_in;
> > > + tattr.data_size_in = tests[i].data_size_in;
> > > +
> > > + tattr.data_size_out = 0;
> > > + bpf_obj->bss->ifindex = *tests[i].ifindex;
> > > + bpf_obj->bss->ret = 0;
> > > + err = bpf_prog_test_run_opts(bpf_program__fd(prog), &tattr);
> > > + sprintf(buf, "err: %s [%s]", tests[i].msg,
> bpf_program__name(prog));
> > > +
> > > + if (at_tc && tests[i].success_on_tc)
> > > + ASSERT_GE(err, 0, buf);
> > > + else
> > > + ASSERT_EQ(err, tests[i].err, buf);
> > > + sprintf(buf, "ret: %s [%s]", tests[i].msg,
> bpf_program__name(prog));
> > > + if (at_tc && tests[i].success_on_tc)
> > > + ASSERT_GE(bpf_obj->bss->ret, 0, buf);
> > > + else
> > > + ASSERT_EQ(bpf_obj->bss->ret, tests[i].ret, buf);
> > > + }
> > > + }
> > > +
> > > +out:
> > > + if (bpf_obj)
> > > + empty_skb__destroy(bpf_obj);
> > > + if (tok)
> > > + close_netns(tok);
> > > + system("ip netns del empty_skb");
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/empty_skb.c
> b/tools/testing/selftests/bpf/progs/empty_skb.c
> > > new file mode 100644
> > > index 000000000000..4b0cd6753251
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/empty_skb.c
> > > @@ -0,0 +1,37 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > > +#include <linux/bpf.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_endian.h>
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +int ifindex;
> > > +int ret;
> > > +
> > > +SEC("lwt_xmit")
> > > +int redirect_ingress(struct __sk_buff *skb)
> > > +{
> > > + ret = bpf_clone_redirect(skb, ifindex, BPF_F_INGRESS);
> > > + return 0;
> > > +}
> > > +
> > > +SEC("lwt_xmit")
> > > +int redirect_egress(struct __sk_buff *skb)
> > > +{
> > > + ret = bpf_clone_redirect(skb, ifindex, 0);
> > > + return 0;
> > > +}
> > > +
> > > +SEC("tc")
> > > +int tc_redirect_ingress(struct __sk_buff *skb)
> > > +{
> > > + ret = bpf_clone_redirect(skb, ifindex, BPF_F_INGRESS);
> > > + return 0;
> > > +}
> > > +
> > > +SEC("tc")
> > > +int tc_redirect_egress(struct __sk_buff *skb)
> > > +{
> > > + ret = bpf_clone_redirect(skb, ifindex, 0);
> > > + return 0;
> > > +}
> > > --
> > > 2.38.1.584.g0f3c55d4c2-goog
> > >
next prev parent reply other threads:[~2022-11-23 17:17 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-21 18:03 [PATCH bpf-next v2 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect Stanislav Fomichev
2022-11-21 18:03 ` [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable Stanislav Fomichev
2022-11-23 12:23 ` Jiri Olsa
2022-11-23 16:18 ` Yonghong Song
2022-11-23 17:17 ` sdf [this message]
2022-11-23 19:07 ` Yonghong Song
2022-11-23 19:54 ` Stanislav Fomichev
2022-11-23 21:20 ` Jiri Olsa
2023-09-08 16:54 ` Daniel Borkmann
2023-09-08 17:17 ` Stanislav Fomichev
2022-11-21 21:00 ` [PATCH bpf-next v2 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect patchwork-bot+netdevbpf
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=Y35VrXvKBFg2RJ7y@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=kpsingh@kernel.org \
--cc=martin.lau@linux.dev \
--cc=olsajiri@gmail.com \
--cc=song@kernel.org \
--cc=yhs@fb.com \
--cc=yhs@meta.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox