netdev - Re: [PATCH bpf 2/2] selftests/bpf: tunnel: add sanity test for checksums

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7372590a-f40b-17d1-f780-3bd1ce4f30bb@linux.dev>
Date:   Tue, 20 Dec 2022 15:21:08 -0800
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, daniel@...earbox.net
Subject: Re: [PATCH bpf 2/2] selftests/bpf: tunnel: add sanity test for
 checksums

On 12/19/22 4:47 PM, Jakub Kicinski wrote:
> Simple netdevsim based test. Netdevsim will validate xmit'ed
> packets, in particular we care about checksum sanity (along
> the lines of checks inside skb_checksum_help()). Triggering
> skb_checksum_help() directly would require the right HW device
> or a crypto device setup, netdevsim is much simpler.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   drivers/net/netdevsim/netdev.c                |  5 ++++
>   tools/testing/selftests/bpf/test_tc_tunnel.sh | 27 +++++++++++++++++++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 6db6a75ff9b9..e4808a6d37a4 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -33,6 +33,11 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	if (!nsim_ipsec_tx(ns, skb))
>   		goto out;
>   
> +	/* Validate the packet */
> +	if (skb->ip_summed == CHECKSUM_PARTIAL)
> +		WARN_ON_ONCE((unsigned int)skb_checksum_start_offset(skb) >=
> +			     skb_headlen(skb));
> +
>   	u64_stats_update_begin(&ns->syncp);
>   	ns->tx_packets++;
>   	ns->tx_bytes += skb->len;
> diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> index 334bdfeab940..4dac87f6a6fa 100755
> --- a/tools/testing/selftests/bpf/test_tc_tunnel.sh
> +++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> @@ -15,6 +15,7 @@ readonly ns1_v4=192.168.1.1
>   readonly ns2_v4=192.168.1.2
>   readonly ns1_v6=fd::1
>   readonly ns2_v6=fd::2
> +readonly nsim_v4=192.168.2.1
>   
>   # Must match port used by bpf program
>   readonly udpport=5555
> @@ -67,6 +68,10 @@ cleanup() {
>   	if [[ -n $server_pid ]]; then
>   		kill $server_pid 2> /dev/null
>   	fi
> +
> +	if [ -e /sys/bus/netdevsim/devices/netdevsim1 ]; then
> +	    echo 1 > /sys/bus/netdevsim/del_device
> +	fi
>   }
>   
>   server_listen() {
> @@ -93,6 +98,25 @@ verify_data() {
>   	fi
>   }
>   
> +decap_sanity() {
> +    echo "test decap sanity"
> +    modprobe netdevsim
> +    echo 1 1 > /sys/bus/netdevsim/new_device
> +    udevadm settle
> +    nsim=$(ls /sys/bus/netdevsim/devices/netdevsim1/net/)
> +    ip link set dev $nsim up
> +    ip addr add dev $nsim $nsim_v4/24
> +
> +    tc qdisc add dev $nsim clsact
> +    tc filter add dev $nsim egress \
> +       bpf direct-action object-file ${BPF_FILE} section decap
> +
> +    echo abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz | \
> +	nc -u 192.168.2.2 7777

Thanks for the fix and the idea on how to test it.

I have posted a patch to translate this test to a test for test_progs that can 
finish and exit such that it can be run continuously in CI.  The test attaches a 
tc-bpf at lo and the bpf prog directly checks for the skb->ip_summed == 
CHECKSUM_NONE and the broken csum_start condition.

If the test_progs patch looks good, patch 1 can be landed first and then land 
the test_progs patch.  wdyt?