Re: [PATCH net-next] selftests: net: Introduce first PMTU test

[Stefano Brivio]

On Mon, 5 Mar 2018 22:26:42 -0700
David Ahern  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 ..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 XX)"
> > +NS_B="ns-$(mktemp -u XX)"
> > +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 (' || (...)' or ' && (...)') 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)
> > +   ;;
> > +   *)
> > +  

Re: [PATCH net-next] selftests: net: Introduce first PMTU test

[David Ahern]

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 ..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 XX)"
> +NS_B="ns-$(mktemp -u XX)"
> +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 (' || (...)' or ' && (...)') 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" ] && 

[PATCH net-next] selftests: net: Introduce first PMTU test

[Stefano Brivio]

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 
Signed-off-by: Stefano Brivio 
---
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 ..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 XX)"
+NS_B="ns-$(mktemp -u XX)"
+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)
+