On Mon, Nov 04, 2024 at 07:44:29PM +0200, Ido Schimmel wrote:
- 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.
- 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() {