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 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.