Message-ID: <20180306110608.2b9d9d15@epycfail>
Date:   Tue, 6 Mar 2018 11:11:06 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     David Ahern <dsahern@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Wei Wang <weiwan@...gle.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Maciej Żenczykowski 
        <maze@...gle.com>, Xiumei Mu <xmu@...hat.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] selftests: net: Introduce first PMTU test

On Mon, 5 Mar 2018 22:26:42 -0700
David Ahern <dsahern@...il.com> wrote:

> On 3/5/18 3:45 PM, Stefano Brivio wrote:
> > diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> > new file mode 100755
> > index 000000000000..eb186ca3e5e4
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/pmtu.sh
> > @@ -0,0 +1,159 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Check that route PMTU values match expectations
> > +#
> > +# Tests currently implemented:
> > +#
> > +# - test_pmtu_vti6_exception
> > +#	Set up vti6 tunnel on top of veth, with xfrm states and policies, in two
> > +#	namespaces with matching endpoints. Check that route exception is
> > +#	created by exceeding link layer MTU with ping to other endpoint. Then
> > +#	decrease and increase MTU of tunnel, checking that route exception PMTU
> > +#	changes accordingly
> > +
> > +NS_A="ns-$(mktemp -u XXXXXX)"
> > +NS_B="ns-$(mktemp -u XXXXXX)"
> > +ns_a="ip netns exec ${NS_A}"
> > +ns_b="ip netns exec ${NS_B}"
> > +
> > +veth6_a_addr="fd00:1::a"
> > +veth6_b_addr="fd00:1::b"
> > +veth6_mask="64"
> > +
> > +vti6_a_addr="fd00:2::a"
> > +vti6_b_addr="fd00:2::b"
> > +vti6_mask="64"
> > +
> > +setup_namespaces() {
> > +	ip netns add ${NS_A} || return 1
> > +	ip netns add ${NS_B}  
> 
> for basic config commands that should work every encasing in
> set -e
> ...
> set +e
> 
> simplifies error handling. IMO, it is relevant for netns and veth config
> commands. Not so much for the xfrm commands which need to load modules
> or depend on features that need config options.

Still I would prefer in that case to handle the error explicitly and be
verbose about what went wrong, because also netns and veth might be
missing. Sure, USER_NS is in 'config', but one might actually not use
that file, or try to run this as a single test.

> > +
> > +	return 0
> > +}
> > +
> > +setup_veth() {
> > +	${ns_a} ip link add veth_a type veth peer name veth_b || return 1
> > +	${ns_a} ip link set veth_b netns ${NS_B}
> > +	
> > +	${ns_a} ip link set veth_a up
> > +	${ns_b} ip link set veth_b up
> > +
> > +	${ns_a} ip addr add ${veth6_a_addr}/${veth6_mask} dev veth_a
> > +	${ns_b} ip addr add ${veth6_b_addr}/${veth6_mask} dev veth_b
> > +
> > +	return 0
> > +}
> > +
> > +setup_vti6() {
> > +	${ns_a} ip link add vti_a type vti6 local ${veth6_a_addr} remote ${veth6_b_addr} key 10 || return 1
> > +	${ns_b} ip link add vti_b type vti6 local ${veth6_b_addr} remote ${veth6_a_addr} key 10
> > +
> > +	${ns_a} ip link set vti_a up
> > +	${ns_b} ip link set vti_b up
> > +
> > +	${ns_a} ip addr add ${vti6_a_addr}/${vti6_mask} dev vti_a
> > +	${ns_b} ip addr add ${vti6_b_addr}/${vti6_mask} dev vti_b
> > +
> > +	return 0
> > +}
> > +
> > +setup_xfrm() {
> > +	${ns_a} ip -6 xfrm state add src ${veth6_a_addr} dst ${veth6_b_addr} spi 0x1000 proto esp aead "rfc4106(gcm(aes))" 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel || return 1
> > +	${ns_a} ip -6 xfrm state add src ${veth6_b_addr} dst ${veth6_a_addr} spi 0x1001 proto esp aead "rfc4106(gcm(aes))" 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > +	${ns_a} ip -6 xfrm policy add dir out mark 10 tmpl src ${veth6_a_addr} dst ${veth6_b_addr} proto esp mode tunnel
> > +	${ns_a} ip -6 xfrm policy add dir in mark 10 tmpl src ${veth6_b_addr} dst ${veth6_a_addr} proto esp mode tunnel
> > +
> > +	${ns_b} ip -6 xfrm state add src ${veth6_a_addr} dst ${veth6_b_addr} spi 0x1000 proto esp aead "rfc4106(gcm(aes))" 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > +	${ns_b} ip -6 xfrm state add src ${veth6_b_addr} dst ${veth6_a_addr} spi 0x1001 proto esp aead "rfc4106(gcm(aes))" 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > +	${ns_b} ip -6 xfrm policy add dir out mark 10 tmpl src ${veth6_b_addr} dst ${veth6_a_addr} proto esp mode tunnel
> > +	${ns_b} ip -6 xfrm policy add dir in mark 10 tmpl src ${veth6_a_addr} dst ${veth6_b_addr} proto esp mode tunnel
> > +
> > +	return 0
> > +}
> > +
> > +setup() {
> > +	tunnel_type="$1"
> > +
> > +	[ "$(id -u)" -ne 0 ] && (echo "SKIP: need to run as root" && exit 0)
> > +
> > +	setup_namespaces || (echo "SKIP: namespaces not supported" && exit 0)
> > +	setup_veth || (echo "SKIP: veth not supported" && exit 0)  
> 
> You use this style ('<command> || (...)' or '<command> && (...)') a lot
> and it does not actually exit the script. You can verify by adding
> 'return 1' to either function.

I forgot that runs in a subshell, will fix in v2 by reversing the
return values, so that I can just have function && echo ... && exit 0,
which is not ambiguous on any shell.

> > +
> > +	case ${tunnel_type} in
> > +	"vti6")
> > +		setup_vti6 && (echo "SKIP: vti6 not supported" && exit 0)
> > +		setup_xfrm && (echo "SKIP: xfrm not supported" && exit 0)
> > +		;;
> > +	*)
> > +		;;
> > +	esac
> > +}
> > +
> > +cleanup() {
> > +	ip netns del ${NS_A} 2 > /dev/null
> > +	ip netns del ${NS_B} 2 > /dev/null
> > +}
> > +
> > +mtu() {
> > +	ns_cmd="${1}"
> > +	dev="${2}"
> > +	mtu="${3}"
> > +
> > +	${ns_cmd} ip link set dev ${dev} mtu ${mtu}
> > +}
> > +
> > +route_get_dst_exception() {
> > +	dst="${1}"
> > +
> > +	${ns_a} ip -6 route get "${dst}" | tail -n1 | tr -s ' '
> > +}
> > +
> > +route_get_dst_pmtu_from_exception() {
> > +	dst="${1}"
> > +
> > +	exception="$(route_get_dst_exception ${dst})"
> > +	next=0
> > +	for i in ${exception}; do
> > +		[ ${next} -eq 1 ] && echo "${i}" && return
> > +		[ "${i}" = "mtu" ] && next=1
> > +	done
> > +}
> > +
> > +test_pmtu_vti6_exception() {
> > +	setup vti6
> > +
> > +	# Create route exception by exceeding link layer MTU
> > +	mtu "${ns_a}" vti_a 5000
> > +	mtu "${ns_b}" vti_b 5000
> > +	${ns_a} ping6 -q -i 0.1 -w 2 -s 60000 ${vti6_b_addr} > /dev/null
> > +
> > +	# Check that exception was created
> > +	exception="$(route_get_dst_exception ${vti6_b_addr})"
> > +	case ${exception} in
> > +		" cache expires "*) : ;;
> > +		*) echo "FAIL: Tunnel exceeding link layer MTU didn't create route exception"; exit 1 ;;  
> 
> I see errors on this check and did not have time to resolve why.
>     bash -x pmtu.sh
> shows
> + exception='fd00:2::b from :: dev vti_a src fd00:2::a metric 0 expires
> 598sec mtu 1426 pref medium'
> 
> You are expecting 'cache expires' so perhaps a difference in iproute2
> output?

It seems so. I'll make this more robust in v2, by just checking for
"mtu <n>" in whatever position, without piping to tail and tr. After
all, I'm looking for a PMTU exception here, not any exception.

> > +	esac
> > +
> > +	# Decrease tunnel MTU, check for PMTU decrease in route exception
> > +	mtu "${ns_a}" vti_a 3000
> > +	if [ "$(route_get_dst_pmtu_from_exception ${vti6_b_addr})" -ne 3000 ]; then
> > +		echo "FAIL: Decreasing tunnel MTU didn't decrease route exception PMTU"
> > +		exit 1
> > +	fi
> > +
> > +	# Increase tunnel MTU, check for PMTU increase in route exception
> > +	mtu "${ns_a}" vti_a 9000
> > +	if [ "$(route_get_dst_pmtu_from_exception ${vti6_b_addr})" -ne 9000 ]; then
> > +		echo "FAIL: Increasing tunnel MTU didn't increase route exception PMTU"
> > +		exit 1
> > +	fi
> > +
> > +	echo "PASS"
> > +}
> > +
> > +trap cleanup EXIT
> > +
> > +test_pmtu_vti6_exception
> > +
> > +exit 0
> >   
> 
> Also, I see a lot of messages on console:
> 
> [11057.340816] ip6_tunnel: vti_a xmit: Local address not yet configured!

This is because the address on vti_a is not configured immediately as
soon as I add it. I'm going to add a 'sleep 1' after configuring.

> This is a good test case to add given the layers involved.

Thanks for your review, I'll post v2 soon.

-- 
Stefano

Powered by blists - more mailing lists