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