netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] selftests: net: Introduce first PMTU test
@ 2018-03-05 22:45 Stefano Brivio
  2018-03-06  5:26 ` David Ahern
  0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2018-03-05 22:45 UTC (permalink / raw)
  To: David S . Miller
  Cc: David Ahern, Wei Wang, Hideaki YOSHIFUJI,
	Maciej Żenczykowski, Xiumei Mu, netdev

One single test implemented so far: test_pmtu_vti6_exception
checks that the PMTU of a route exception, caused by a tunnel
exceeding the link layer MTU, is affected by administrative
changes of the tunnel MTU. Creation of the route exception is
checked too.

Requested-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
This test will currently fail without "[PATCH net v2] ipv6: Reflect
MTU changes on PMTU of exceptions for MTU-less routes"

 tools/testing/selftests/net/Makefile |   2 +-
 tools/testing/selftests/net/pmtu.sh  | 159 +++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/pmtu.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 229a038966e3..785fc18a16b4 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -5,7 +5,7 @@ CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
 CFLAGS += -I../../../../usr/include/
 
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
-TEST_PROGS += fib_tests.sh fib-onlink-tests.sh
+TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh
 TEST_GEN_FILES =  socket
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy
 TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
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}
+
+	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)
+
+	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 ;;
+	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
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] selftests: net: Introduce first PMTU test
  2018-03-05 22:45 [PATCH net-next] selftests: net: Introduce first PMTU test Stefano Brivio
@ 2018-03-06  5:26 ` David Ahern
  2018-03-06 10:11   ` Stefano Brivio
  0 siblings, 1 reply; 3+ messages in thread
From: David Ahern @ 2018-03-06  5:26 UTC (permalink / raw)
  To: Stefano Brivio, David S . Miller
  Cc: Wei Wang, Hideaki YOSHIFUJI, Maciej Żenczykowski, Xiumei Mu,
	netdev

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.

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

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

> +	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 a good test case to add given the layers involved.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] selftests: net: Introduce first PMTU test
  2018-03-06  5:26 ` David Ahern
@ 2018-03-06 10:11   ` Stefano Brivio
  0 siblings, 0 replies; 3+ messages in thread
From: Stefano Brivio @ 2018-03-06 10:11 UTC (permalink / raw)
  To: David Ahern
  Cc: David S. Miller, Wei Wang, Hideaki YOSHIFUJI,
	Maciej Żenczykowski, Xiumei Mu, netdev

On Mon, 5 Mar 2018 22:26:42 -0700
David Ahern <dsahern@gmail.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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-03-06 10:11 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-05 22:45 [PATCH net-next] selftests: net: Introduce first PMTU test Stefano Brivio
2018-03-06  5:26 ` David Ahern
2018-03-06 10:11   ` Stefano Brivio

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).