| Message ID | 9c4896b109a39c3fa088844addaa1737a84bbbb5.1690332693.git.yan@cloudflare.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: return proper error codes for lwt redirect | expand |
Apologize for sending previous mail from a wrong app (not text mode). Resending to keep the mailing list thread consistent. On Wed, Jul 26, 2023 at 3:10 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > > Tests BPF redirect at the lwt xmit hook to ensure error handling are > > safe, i.e. won't panic the kernel. > > Are imperative change descriptions still preferred? Hi Markus, I think you linked this to me yesterday that it should be described imperatively: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n155 > > See also: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94 > I don’t follow the purpose of this reference. This points to user impact but this is a selftest, so I don’t see any user impact here. Or is there anything I missed? > > Can remaining wording weaknesses be adjusted accordingly? I am not following this question . Can you be more specific or provide an example? Yan > > Regards, > Markus >
On Tue, Jul 25, 2023 at 06:09 PM -07, Yan Zhai wrote: > Tests BPF redirect at the lwt xmit hook to ensure error handling are > safe, i.e. won't panic the kernel. > > Tested-by: Jakub Sitnicki <jakub@cloudflare.com> > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Signed-off-by: Yan Zhai <yan@cloudflare.com> > --- Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Was Markus unbanned from vger? How are we recieving these emails? regards, dan carpenter
On 7/25/23 6:09 PM, Yan Zhai wrote: > diff --git a/tools/testing/selftests/bpf/progs/test_lwt_redirect.c b/tools/testing/selftests/bpf/progs/test_lwt_redirect.c > new file mode 100644 > index 000000000000..3674e101f68f > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_lwt_redirect.c > @@ -0,0 +1,66 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > +#include "bpf_tracing_net.h" > + > +/* We don't care about whether the packet can be received by network stack. > + * Just care if the packet is sent to the correct device at correct direction > + * and not panic the kernel. > + */ > +static __always_inline int prepend_dummy_mac(struct __sk_buff *skb) > +{ __always_inline is no longer a must for a long time. > + char mac[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0xf, > + 0xe, 0xd, 0xc, 0xb, 0xa, 0x08, 0x00}; > + > + if (bpf_skb_change_head(skb, ETH_HLEN, 0)) { > + bpf_printk("%s: fail to change head", __func__); Avoid using bpf_printk(). The bpf CI runs other tests also. > + return -1; > + } > + > + if (bpf_skb_store_bytes(skb, 0, mac, sizeof(mac), 0)) { > + bpf_printk("%s: fail to update mac", __func__); > + return -1; > + } > + > + return 0; > +} > + > +SEC("redir_ingress") Use SEC("lwt_xmit"). Then the libbpf will figure out the prog type. > +int test_lwt_redirect_in(struct __sk_buff *skb) > +{ > + if (prepend_dummy_mac(skb)) > + return BPF_DROP; > + > + bpf_printk("Redirect skb to link %d ingress", skb->mark); > + return bpf_redirect(skb->mark, BPF_F_INGRESS); > +} > + > +SEC("redir_egress") > +int test_lwt_redirect_out(struct __sk_buff *skb) > +{ > + if (prepend_dummy_mac(skb)) > + return BPF_DROP; > + > + bpf_printk("Redirect skb to link %d egress", skb->mark); > + return bpf_redirect(skb->mark, 0); > +} > + > +SEC("redir_egress_nomac") > +int test_lwt_redirect_out_nomac(struct __sk_buff *skb) > +{ > + int ret = bpf_redirect(skb->mark, 0); > + > + bpf_printk("Redirect skb to link %d egress nomac: %d", skb->mark, ret); > + return ret; > +} > + > +SEC("redir_ingress_nomac") > +int test_lwt_redirect_in_nomac(struct __sk_buff *skb) > +{ > + int ret = bpf_redirect(skb->mark, BPF_F_INGRESS); > + > + bpf_printk("Redirect skb to link %d ingress nomac: %d", skb->mark, ret); > + return ret; > +} > + > +char _license[] SEC("license") = "GPL"; > diff --git a/tools/testing/selftests/bpf/test_lwt_redirect.sh b/tools/testing/selftests/bpf/test_lwt_redirect.sh > new file mode 100755 > index 000000000000..1b7b78b48174 > --- /dev/null > +++ b/tools/testing/selftests/bpf/test_lwt_redirect.sh This has to be written in the test_progs infrastructure in C. Only test_progs is run by the BPF CI. Take a look at other tests in prog_tests/. For example, tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It currently has helpers to add tc qdisc and filter but not adding route yet which could be a useful addition. -- pw-bot: cr
On Fri, Jul 28, 2023 at 03:47 PM -07, Martin KaFai Lau wrote: > On 7/25/23 6:09 PM, Yan Zhai wrote: [...] >> diff --git a/tools/testing/selftests/bpf/test_lwt_redirect.sh >> b/tools/testing/selftests/bpf/test_lwt_redirect.sh >> new file mode 100755 >> index 000000000000..1b7b78b48174 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/test_lwt_redirect.sh > > This has to be written in the test_progs infrastructure in C. Only test_progs is > run by the BPF CI. Take a look at other tests in prog_tests/. For example, > tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It > currently has helpers to add tc qdisc and filter but not adding route yet which > could be a useful addition. Can we help make the BPF CI better so that it also runs other tests in addition test_progs? We have bpf selftests written in shell and even Python. These are sometimes the right tools for the job and make adding tests easier, IMHO. Network setup from C is verbose and tedious. Not to mention, hard to read through. # ./run_kselftest.sh --list bpf:test_verifier bpf:test_tag bpf:test_maps bpf:test_lru_map bpf:test_lpm_map bpf:test_progs bpf:test_dev_cgroup bpf:test_sock bpf:test_sockmap bpf:get_cgroup_id_user bpf:test_cgroup_storage bpf:test_tcpnotify_user bpf:test_sysctl bpf:test_progs-no_alu32 bpf:test_kmod.sh bpf:test_xdp_redirect.sh bpf:test_xdp_redirect_multi.sh bpf:test_xdp_meta.sh bpf:test_xdp_veth.sh bpf:test_offload.py bpf:test_sock_addr.sh bpf:test_tunnel.sh bpf:test_lwt_seg6local.sh bpf:test_lirc_mode2.sh bpf:test_skb_cgroup_id.sh bpf:test_flow_dissector.sh bpf:test_xdp_vlan_mode_generic.sh bpf:test_xdp_vlan_mode_native.sh bpf:test_lwt_ip_encap.sh bpf:test_tcp_check_syncookie.sh bpf:test_tc_tunnel.sh bpf:test_tc_edt.sh bpf:test_xdping.sh bpf:test_bpftool_build.sh bpf:test_bpftool.sh bpf:test_bpftool_metadata.sh bpf:test_doc_build.sh bpf:test_xsk.sh bpf:test_xdp_features.sh
On Mon, Jul 31, 2023 at 2:52 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > On Fri, Jul 28, 2023 at 03:47 PM -07, Martin KaFai Lau wrote: > > On 7/25/23 6:09 PM, Yan Zhai wrote: > > [...] > > >> diff --git a/tools/testing/selftests/bpf/test_lwt_redirect.sh > >> b/tools/testing/selftests/bpf/test_lwt_redirect.sh > >> new file mode 100755 > >> index 000000000000..1b7b78b48174 > >> --- /dev/null > >> +++ b/tools/testing/selftests/bpf/test_lwt_redirect.sh > > > > This has to be written in the test_progs infrastructure in C. Only test_progs is > > run by the BPF CI. Take a look at other tests in prog_tests/. For example, > > tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It > > currently has helpers to add tc qdisc and filter but not adding route yet which > > could be a useful addition. > > Can we help make the BPF CI better so that it also runs other tests in > addition test_progs? Not really. CI is not just running the test. It needs to understand the output, pass it to UI, run in parallel, etc. All the shell scripts are not suitable for long term CI exposure. So I completely agree with Martin. No new shell scripts. All selftests must be in test_progs. > We have bpf selftests written in shell and even Python. These are > sometimes the right tools for the job and make adding tests easier, > IMHO. Network setup from C is verbose and tedious. Not to mention, hard > to read through. For comparison take a look at BPF CI code base and what it takes to run the tests and process the output. There is plenty of work for CI ahead.
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 538df8fb8c42..e3a24d053793 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -66,6 +66,7 @@ TEST_PROGS := test_kmod.sh \ test_xdp_vlan_mode_generic.sh \ test_xdp_vlan_mode_native.sh \ test_lwt_ip_encap.sh \ + test_lwt_redirect.sh \ test_tcp_check_syncookie.sh \ test_tc_tunnel.sh \ test_tc_edt.sh \ diff --git a/tools/testing/selftests/bpf/progs/test_lwt_redirect.c b/tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file mode 100644 index 000000000000..3674e101f68f --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_lwt_redirect.c @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include "bpf_tracing_net.h" + +/* We don't care about whether the packet can be received by network stack. + * Just care if the packet is sent to the correct device at correct direction + * and not panic the kernel. + */ +static __always_inline int prepend_dummy_mac(struct __sk_buff *skb) +{ + char mac[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0xf, + 0xe, 0xd, 0xc, 0xb, 0xa, 0x08, 0x00}; + + if (bpf_skb_change_head(skb, ETH_HLEN, 0)) { + bpf_printk("%s: fail to change head", __func__); + return -1; + } + + if (bpf_skb_store_bytes(skb, 0, mac, sizeof(mac), 0)) { + bpf_printk("%s: fail to update mac", __func__); + return -1; + } + + return 0; +} + +SEC("redir_ingress") +int test_lwt_redirect_in(struct __sk_buff *skb) +{ + if (prepend_dummy_mac(skb)) + return BPF_DROP; + + bpf_printk("Redirect skb to link %d ingress", skb->mark); + return bpf_redirect(skb->mark, BPF_F_INGRESS); +} + +SEC("redir_egress") +int test_lwt_redirect_out(struct __sk_buff *skb) +{ + if (prepend_dummy_mac(skb)) + return BPF_DROP; + + bpf_printk("Redirect skb to link %d egress", skb->mark); + return bpf_redirect(skb->mark, 0); +} + +SEC("redir_egress_nomac") +int test_lwt_redirect_out_nomac(struct __sk_buff *skb) +{ + int ret = bpf_redirect(skb->mark, 0); + + bpf_printk("Redirect skb to link %d egress nomac: %d", skb->mark, ret); + return ret; +} + +SEC("redir_ingress_nomac") +int test_lwt_redirect_in_nomac(struct __sk_buff *skb) +{ + int ret = bpf_redirect(skb->mark, BPF_F_INGRESS); + + bpf_printk("Redirect skb to link %d ingress nomac: %d", skb->mark, ret); + return ret; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/test_lwt_redirect.sh b/tools/testing/selftests/bpf/test_lwt_redirect.sh new file mode 100755 index 000000000000..1b7b78b48174 --- /dev/null +++ b/tools/testing/selftests/bpf/test_lwt_redirect.sh @@ -0,0 +1,174 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# This regression test checks basic lwt redirect functionality, +# making sure the kernel would not crash when redirecting packets +# to a device, regardless its administration state: +# +# 1. redirect to a device egress/ingress should work normally +# 2. redirect to a device egress/ingress should not panic when target is down +# 3. redirect to a device egress/ingress should not panic when target carrier is down +# +# All test setup are simple: redirect ping packet via lwt xmit to cover above +# situations. We do not worry about specific device type, except for the two +# categories of devices that require MAC header and not require MAC header. For +# carrier down situation, we use a vlan device as upper link, and bring down its +# lower device. +# +# Kselftest framework requirement - SKIP code is 4. +ksft_skip=4 +BPF_FILE="test_lwt_redirect.bpf.o" +INGRESS_REDIR_IP=2.2.2.2 +EGRESS_REDIR_IP=3.3.3.3 +INGRESS_REDIR_IP_NOMAC=4.4.4.4 +EGRESS_REDIR_IP_NOMAC=5.5.5.5 +PASS=0 +FAIL=0 + +readonly NS1="ns1-$(mktemp -u XXXXXX)" + +msg="skip all tests:" +if [ $UID != 0 ]; then + echo $msg please run this as root >&2 + exit $ksft_skip +fi + +get_ip_direction() +{ + case $1 in + $INGRESS_REDIR_IP|$INGRESS_REDIR_IP_NOMAC) + echo ingress + ;; + $EGRESS_REDIR_IP|$EGRESS_REDIR_IP_NOMAC) + echo egress + ;; + *) + echo bug + ;; + esac +} + +test_pass() +{ + local testname=$1 + local direction=`get_ip_direction $2` + shift 2 + echo "Pass: $testname $direction $@" + PASS=$((PASS + 1)) +} + +test_fail() +{ + local testname=$1 + local direction=`get_ip_direction $2` + shift 2 + echo "Fail: $testname $direction $@" + FAIL=$((FAIL + 1)) +} + +setup() +{ + ip netns add $NS1 + + ip -n $NS1 link set lo up + ip -n $NS1 link add link_err type dummy + ip -n $NS1 link add link_w_mac type dummy + ip -n $NS1 link add link link_w_mac link_upper type vlan id 1 + ip -n $NS1 link add link_wo_mac type gre remote 4.3.2.1 local 1.2.3.4 + ip -n $NS1 link set link_err up + ip -n $NS1 link set link_w_mac up + ip -n $NS1 link set link_upper up + ip -n $NS1 link set link_wo_mac up + + ip -n $NS1 addr add dev lo 1.1.1.1/32 + + # link_err is only used to make sure packets are redirected instead of + # being routed + ip -n $NS1 route add $INGRESS_REDIR_IP encap bpf xmit \ + obj $BPF_FILE sec redir_ingress dev link_err + ip -n $NS1 route add $EGRESS_REDIR_IP encap bpf xmit \ + obj $BPF_FILE sec redir_egress dev link_err + ip -n $NS1 route add $INGRESS_REDIR_IP_NOMAC encap bpf xmit \ + obj $BPF_FILE sec redir_ingress_nomac dev link_err + ip -n $NS1 route add $EGRESS_REDIR_IP_NOMAC encap bpf xmit \ + obj $BPF_FILE sec redir_egress_nomac dev link_err +} + +cleanup_and_summary() +{ + ip netns del $NS1 + echo PASSED:$PASS FAILED:$FAIL + if [ $FAIL -ne 0 ]; then + exit 1 + else + exit 0 + fi +} + +test_redirect_normal() +{ + local test_name=${FUNCNAME[0]} + local link_name=$1 + local link_id=`ip netns exec $NS1 cat /sys/class/net/${link_name}/ifindex` + local dest=$2 + + ip netns exec $NS1 timeout 2 tcpdump -i ${link_name} -c 1 -n -p icmp >/dev/null 2>&1 & + local jobid=$! + sleep 1 + + # hack: mark indicates the link to redirect to + ip netns exec $NS1 ping -m $link_id $dest -c 1 -w 1 > /dev/null 2>&1 + wait $jobid + + if [ $? -ne 0 ]; then + test_fail $test_name $dest $link_name + else + test_pass $test_name $dest $link_name + fi +} + +test_redirect_no_panic_on_link_down() +{ + local test_name=${FUNCNAME[0]} + local link_name=$1 + local link_id=`ip netns exec $NS1 cat /sys/class/net/${link_name}/ifindex` + local dest=$2 + + ip -n $NS1 link set $link_name down + # hack: mark indicates the link to redirect to + ip netns exec $NS1 ping -m $link_id $dest -c 1 -w 1 >/dev/null 2>&1 + + test_pass $test_name $dest to $link_name + ip -n $NS1 link set $link_name up +} + +test_redirect_no_panic_on_link_carrier_down() +{ + local test_name=${FUNCNAME[0]} + local link_id=`ip netns exec $NS1 cat /sys/class/net/link_upper/ifindex` + local dest=$1 + + ip -n $NS1 link set link_w_mac down + # hack: mark indicates the link to redirect to + ip netns exec $NS1 ping -m $link_id $dest -c 1 -w 1 >/dev/null 2>&1 + + test_pass $test_name $dest to link_upper + ip -n $NS1 link set link_w_mac up +} + +setup + +echo "Testing lwt redirect to devices requiring MAC header" +for dest in $INGRESS_REDIR_IP $EGRESS_REDIR_IP; do + test_redirect_normal link_w_mac $dest + test_redirect_no_panic_on_link_down link_w_mac $dest + test_redirect_no_panic_on_link_carrier_down $dest +done + +echo "Testing lwt redirect to devices not requiring MAC header" +for dest in $INGRESS_REDIR_IP_NOMAC $EGRESS_REDIR_IP_NOMAC; do + test_redirect_normal link_wo_mac $dest + test_redirect_no_panic_on_link_down link_wo_mac $dest +done + +cleanup_and_summary