[PATCH bpf-next v2 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect

[PATCH bpf-next v2 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect

[Stanislav Fomichev]

To avoid potentially breaking existing users.

Both mac/no-mac cases have to be amended; mac_header >= network_header
is not enough (verified with a new test, see next patch).

Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/bpf/test_run.c |  3 ---
 net/core/filter.c  | 11 ++++++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 13d578ce2a09..6fba440efc40 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -979,9 +979,6 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
 {
 	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
 
-	if (!skb->len)
-		return -EINVAL;
-
 	if (!__skb)
 		return 0;
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 2a105fb1ceb2..b6e1b81cdfae 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2124,12 +2124,13 @@ static int __bpf_redirect_no_mac(struct sk_buff *skb, struct net_device *dev,
 {
 	unsigned int mlen = skb_network_offset(skb);
 
+	if (unlikely(skb->len <= mlen)) {
+		kfree_skb(skb);
+		return -ERANGE;
+	}
+
 	if (mlen) {
 		__skb_pull(skb, mlen);
-		if (unlikely(!skb->len)) {
-			kfree_skb(skb);
-			return -ERANGE;
-		}
 
 		/* At ingress, the mac header has already been pulled once.
 		 * At egress, skb_pospull_rcsum has to be done in case that
@@ -2149,7 +2150,7 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
 				 u32 flags)
 {
 	/* Verify that a link layer header is carried */
-	if (unlikely(skb->mac_header >= skb->network_header)) {
+	if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
 		kfree_skb(skb);
 		return -ERANGE;
 	}
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Stanislav Fomichev]

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

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Jiri Olsa]

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

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
> 

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Yonghong Song]

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.

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

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[sdf]

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

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Yonghong Song]

On 11/23/22 9:17 AM, sdf@google.com wrote:
> 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;
>   }

Ya, this does fix the problem. Could you craft a patch for this?

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Stanislav Fomichev]

On Wed, Nov 23, 2022 at 11:07 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 11/23/22 9:17 AM, sdf@google.com wrote:
> > 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;
> >   }
>
> Ya, this does fix the problem. Could you craft a patch for this?

Sure, give me a second..

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Jiri Olsa]

On Wed, Nov 23, 2022 at 11:07:18AM -0800, Yonghong Song wrote:

SNIP

> > > > 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;
> >   }
> 
> Ya, this does fix the problem. Could you craft a patch for this?

same here ;-) thanks

jirka

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Daniel Borkmann]

Hi Stan,

Do you have some cycles to look into the below?

On 11/21/22 7:03 PM, 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>
[...]
> +		/* 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,
> +		},
[...]

This one is now failing in BPF CI on net/net-next ff after the veth driver changed
it's drop error code in [0] from NETDEV_TX_OK (0) to NET_XMIT_DROP (1) :

test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress [redirect_egress]: actual 1 != expected 0
test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
close_netns:PASS:setns 0 nsec
#71      empty_skb:FAIL

The test was testing bpf_clone_redirect which is still okay, just that for the
xmit sides it propagates the error code now into ret and hence the assert fails.
Perhaps we would need to tweak the test case to test for 0 or 1 ... 0 in case
bpf_clone_redirect pushes to ingress, 1 in case it pushes to egress and reaches
veth..

Thanks,
Daniel

   [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=151e887d8ff97e2e42110ffa1fb1e6a2128fb364

> +	};
> +
> +	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);

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Stanislav Fomichev]

On Fri, Sep 8, 2023 at 9:54 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Hi Stan,
>
> Do you have some cycles to look into the below?

Sure, I'll take a look!

> On 11/21/22 7:03 PM, 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>
> [...]
> > +             /* 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,
> > +             },
> [...]
>
> This one is now failing in BPF CI on net/net-next ff after the veth driver changed
> it's drop error code in [0] from NETDEV_TX_OK (0) to NET_XMIT_DROP (1) :
>
> test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress [redirect_egress]: actual 1 != expected 0
> test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> close_netns:PASS:setns 0 nsec
> #71      empty_skb:FAIL
>
> The test was testing bpf_clone_redirect which is still okay, just that for the
> xmit sides it propagates the error code now into ret and hence the assert fails.
> Perhaps we would need to tweak the test case to test for 0 or 1 ... 0 in case
> bpf_clone_redirect pushes to ingress, 1 in case it pushes to egress and reaches
> veth..
>
> Thanks,
> Daniel
>
>    [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=151e887d8ff97e2e42110ffa1fb1e6a2128fb364
>
> > +     };
> > +
> > +     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);
>

Re: [PATCH bpf-next v2 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Mon, 21 Nov 2022 10:03:39 -0800 you wrote:
> To avoid potentially breaking existing users.
> 
> Both mac/no-mac cases have to be amended; mac_header >= network_header
> is not enough (verified with a new test, see next patch).
> 
> Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/2] bpf: Move skb->len == 0 checks into __bpf_redirect
    https://git.kernel.org/bpf/bpf-next/c/114039b34201
  - [bpf-next,v2,2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable
    https://git.kernel.org/bpf/bpf-next/c/68f8e3d4b916

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html