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() {
>
