netdev - Re: [PATCH net 2/2] selftests: pmtu: add explicit tests for pmtu exceptions cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Feb 2019 18:50:45 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net 2/2] selftests: pmtu: add explicit tests for pmtu
 exceptions cleanup

On Fri, 22 Feb 2019 17:06:33 +0100
Paolo Abeni <pabeni@...hat.com> wrote:

> Add a couple of new tests, explicitly checking that the kernel
> timely releases pmtu exceptions on related device removal.
> This is mostly a regression test vs the issue fixed by:
> https://patchwork.ozlabs.org/patch/1045488/
> 
> Only 2 new test cases have been added, instead of extending all
> the existing ones, because the reproducer requires executing
> several commands and would slow down too much the tests otherwise.
> 
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  tools/testing/selftests/net/pmtu.sh | 52 ++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> index 634e91e8fe25..4bc72bc26899 100755
> --- a/tools/testing/selftests/net/pmtu.sh
> +++ b/tools/testing/selftests/net/pmtu.sh
> @@ -135,7 +135,9 @@ tests="
>  	pmtu_vti6_default_mtu		vti6: default MTU assignment
>  	pmtu_vti4_link_add_mtu		vti4: MTU setting on link creation
>  	pmtu_vti6_link_add_mtu		vti6: MTU setting on link creation
> -	pmtu_vti6_link_change_mtu	vti6: MTU changes on link changes"
> +	pmtu_vti6_link_change_mtu	vti6: MTU changes on link changes
> +	pmtu_ipv4_exception_cleanup	ipv4: cleanup of cached exceptions
> +	pmtu_ipv6_exception_cleanup	ipv6: cleanup of cached exceptions"
>  
>  NS_A="ns-$(mktemp -u XXXXXX)"
>  NS_B="ns-$(mktemp -u XXXXXX)"
> @@ -1006,6 +1008,54 @@ test_pmtu_vti6_link_change_mtu() {
>  	return ${fail}
>  }
>  
> +test_cleanup_vxlanY_or_geneveY_exception() {
> +	outer="${1}"
> +	encap="${2}"

As you don't implement GENEVE tests, this function can be kept simpler.

> +	ll_mtu=4000
> +
> +	which taskset 2>/dev/null | return 2
> +	which timeout 2>/dev/null | return 2

This test will not be skipped if taskset or timeout are not available,
I guess you meant: || return 2.

Besides, 'which' can output to both stdout and stderr. See how it's
used anywhere else in this script, e.g.:

	which ping6 > /dev/null 2>&1 && ping6=$(which ping6) || ping6=$(which ping)

You might also want to skip this test on non-SMP.

You should also indicate why the test is skipped, see how it's done in
other places, e.g.:

	${ns_a} ip link add dummy0 mtu 1500 type dummy
	[ $? -ne 0 ] && err "  dummy not supported" && return 2

> +	cpu_list=`grep processor /proc/cpuinfo | cut -d ' ' -f 2`

For consistency, please use $( ... )

If you grep -m2, then you can skip the ncpus thing below.

> +
> +	setup namespaces routing ${encap}${outer} || return 2
> +	trace "${ns_a}" ${encap}_a   "${ns_b}"  ${encap}_b \
> +	      "${ns_a}" veth_A-R1    "${ns_r1}" veth_R1-A \
> +	      "${ns_b}" veth_B-R1    "${ns_r1}" veth_R1-B
> +
> +	# Create route exception by exceeding link layer MTU
> +	mtu "${ns_a}"  veth_A-R1 $((${ll_mtu} + 1000))
> +	mtu "${ns_r1}" veth_R1-A $((${ll_mtu} + 1000))
> +	mtu "${ns_b}"  veth_B-R1 ${ll_mtu}
> +	mtu "${ns_r1}" veth_R1-B ${ll_mtu}
> +
> +	mtu "${ns_a}" ${encap}_a $((${ll_mtu} + 1000))
> +	mtu "${ns_b}" ${encap}_b $((${ll_mtu} + 1000))
> +
> +	# fill exception cache for multiple cpus [2]
> +	# we can always use inner ipv4 for that

Please keep comments consistent with the rest, "Fill ... CPUs (2) ... IPv4".

> +	ncpus=0
> +	for cpu in $cpu_list; do

Nit: ${cpu_list}

> +		taskset --cpu-list ${cpu} ${ns_a} ping -q -M want -i 0.1 -w 1 -s $((${ll_mtu} + 500)) ${tunnel4_b_addr} > /dev/null
> +		ncpus=$((ncpus + 1))
> +		[ ${ncpus} -gt 1 ] && break
> +	done
> +
> +	fail=0

No need for fail=0...

> +	if ! timeout 1 ${ns_a} ip link del dev veth_A-R1; then
> +		err "  can't delete veth device in a timely manner, pmtu dst likely leaked"

(For consistency: "PMTU")

> +		fail=1

...just return 1 here and

> +	fi
> +	return ${fail}

...skip the explicit return. The return code comes from the last
command executed.

> +}
> +
> +test_pmtu_ipv6_exception_cleanup() {
> +	test_cleanup_vxlanY_or_geneveY_exception 6 vxlan
> +}
> +
> +test_pmtu_ipv4_exception_cleanup() {
> +	test_cleanup_vxlanY_or_geneveY_exception 4 vxlan
> +}
> +
>  usage() {
>  	echo
>  	echo "$0 [OPTIONS] [TEST]..."

-- 
Stefano