netdev - Re: [PATCH net-next v7] net: ipv4: Cache pmtu for all packet paths if multipath enabled

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] [day] [month] [year] [list]

Message-ID: <ZyuKkekr2QSF3jUS@debian>
Date: Wed, 6 Nov 2024 16:26:09 +0100
From: Guillaume Nault <gnault@...hat.com>
To: Ido Schimmel <idosch@...sch.org>
Cc: Vladimir Vdovin <deliran@...dict.gg>, netdev@...r.kernel.org,
	dsahern@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	linux-kselftest@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	shuah@...nel.org, horms@...nel.org
Subject: Re: [PATCH net-next v7] net: ipv4: Cache pmtu for all packet paths
 if multipath enabled

On Mon, Nov 04, 2024 at 07:44:29PM +0200, Ido Schimmel wrote:
> 1. We should set the same MTU in both paths as otherwise we don't know
> which MTU will be cached and what to pass to check_pmtu_value() as the
> expected value. I did see that check_pmtu_value() accepts "any", but I
> think it's better to check for a specific value.
> 
> 2. route_get_dst_pmtu_from_exception() is not very flexible in the
> keywords it accepts for "ip route get" and we need to pass "oif". It can
> be solved by [1] (please test), but Guillaume might have a better idea.
> Then, the above hunk can be replaced by [2].

Thanks for bringing this to my attention Ido!
I fully agree with this approach (and I also fully agree with your other
feedbacks).

Nice to see this bug fixed and get a selftest!

> [1]
> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> index 569bce8b6383..6e790d38e5d9 100755
> --- a/tools/testing/selftests/net/pmtu.sh
> +++ b/tools/testing/selftests/net/pmtu.sh
> @@ -1076,23 +1076,15 @@ link_get_mtu() {
>  }
>  
>  route_get_dst_exception() {
> -	ns_cmd="${1}"
> -	dst="${2}"
> -	dsfield="${3}"
> -
> -	if [ -z "${dsfield}" ]; then
> -		dsfield=0
> -	fi
> +	ns_cmd="${1}"; shift
>  
> -	${ns_cmd} ip route get "${dst}" dsfield "${dsfield}"
> +	${ns_cmd} ip route get "$@"
>  }
>  
>  route_get_dst_pmtu_from_exception() {
> -	ns_cmd="${1}"
> -	dst="${2}"
> -	dsfield="${3}"
> +	ns_cmd="${1}"; shift
>  
> -	mtu_parse "$(route_get_dst_exception "${ns_cmd}" "${dst}" "${dsfield}")"
> +	mtu_parse "$(route_get_dst_exception "${ns_cmd}" "$@")"
>  }
>  
>  check_pmtu_value() {
> @@ -1235,10 +1227,10 @@ test_pmtu_ipv4_dscp_icmp_exception() {
>  	run_cmd "${ns_a}" ping -q -M want -Q "${dsfield}" -c 1 -w 1 -s "${len}" "${dst2}"
>  
>  	# Check that exceptions have been created with the correct PMTU
> -	pmtu_1="$(route_get_dst_pmtu_from_exception "${ns_a}" "${dst1}" "${policy_mark}")"
> +	pmtu_1="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst1} dsfield ${policy_mark})"
>  	check_pmtu_value "1400" "${pmtu_1}" "exceeding MTU" || return 1
>  
> -	pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" "${dst2}" "${policy_mark}")"
> +	pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst2} dsfield ${policy_mark})"
>  	check_pmtu_value "1500" "${pmtu_2}" "exceeding MTU" || return 1
>  }
>  
> @@ -1285,9 +1277,9 @@ test_pmtu_ipv4_dscp_udp_exception() {
>  		UDP:"${dst2}":50000,tos="${dsfield}"
>  
>  	# Check that exceptions have been created with the correct PMTU
> -	pmtu_1="$(route_get_dst_pmtu_from_exception "${ns_a}" "${dst1}" "${policy_mark}")"
> +	pmtu_1="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst1} dsfield ${policy_mark})"
>  	check_pmtu_value "1400" "${pmtu_1}" "exceeding MTU" || return 1
> -	pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" "${dst2}" "${policy_mark}")"
> +	pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst2} dsfield ${policy_mark})"
>  	check_pmtu_value "1500" "${pmtu_2}" "exceeding MTU" || return 1
>  }
> 
> [2]
> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> index a3c3f7f99e5b..10b8ac2d7f47 100755
> --- a/tools/testing/selftests/net/pmtu.sh
> +++ b/tools/testing/selftests/net/pmtu.sh
> @@ -2399,19 +2399,11 @@ test_pmtu_ipv4_mp_exceptions() {
>  	# Ping and expect two nexthop exceptions for two routes in nh group
>  	run_cmd ${ns_a} ping -q -M want -i 0.1 -c 1 -s 1800 "${dummy4_b_addr}"
>  
> -	# Do route lookup before checking cached exceptions.
> -	# The following commands are needed for dst entries to be cached
> -	# in both paths exceptions and therefore dumped to user space
> -	run_cmd ${ns_a} ip route get ${dummy4_b_addr} oif veth_A-R1
> -	run_cmd ${ns_a} ip route get ${dummy4_b_addr} oif veth_A-R2
> -
> -	# Check cached exceptions
> -	if [ "$(${ns_a} ip -oneline route list cache | grep mtu | wc -l)" -ne 2 ]; then
> -		err "  there are not enough cached exceptions"
> -		fail=1
> -	fi
> -
> -	return ${fail}
> +	# Check that exceptions have been created with the correct PMTU
> +	pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dummy4_b_addr} oif veth_A-R1)"
> +	check_pmtu_value "1500" "${pmtu}" "exceeding MTU (veth_A-R1)" || return 1
> +	pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dummy4_b_addr} oif veth_A-R2)"
> +	check_pmtu_value "1500" "${pmtu}" "exceeding MTU (veth_A-R2)" || return 1
>  }
>  
>  usage() {
>