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" ] &&