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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <2b84e81a-90f8-898f-d320-a29233ff37ad@iogearbox.net>
Date: Fri, 8 Sep 2023 18:54:31 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Stanislav Fomichev <sdf@...gle.com>, bpf@...r.kernel.org
Cc: ast@...nel.org, martin.lau@...ux.dev, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs
 aren't redirectable

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@...gle.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);