Re: [PATCH net-next v2 3/3] selftests: net: pmtu: Add IPv6 and asymmetric multipath tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In test_pmtu_ipvX_mp_asym_exceptions()
  (tools/testing/selftests/net/pmtu.sh, IPv4 branch), the return routes…
- [Medium] In the IPv6 branch, mp_flow_via()
  (tools/testing/selftests/net/pmtu.sh) depends on `ip route get ${dst}…
- [Low] mp_flow_via() returns a decimal flow label (`flow="${i}"`, i in
  1..32).

--

selftests: net: pmtu: Add IPv6 and asymmetric multipath tests

This patch makes pmtu_ipv4_mp_exceptions family agnostic and adds an IPv6
variant. It also adds IPv4 and IPv6 multipath tests with asymmetric MTUs
(1400 behind R1, 1300 behind R2). These pick a flow through each router,
using a source address for IPv4 and a flow label for IPv6, and check that
the PMTU exceptions on both paths get updated.

> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> index c7cd271714ef7..6eb79cb76db71 100755
> --- a/tools/testing/selftests/net/pmtu.sh
> +++ b/tools/testing/selftests/net/pmtu.sh

[ ... ]

> @@ -2395,18 +2487,126 @@ test_pmtu_ipv4_mp_exceptions() {

[ ... ]

> +mp_flow_via() {
> +	family="${1}"
> +	dst="${2}"
> +	oif="${3}"
> +
> +	for i in $(seq 1 32); do
> +		if [ ${family} -eq 4 ]; then
> +			flow="${host4_a_net}.${i}"
> +			${ns_a} ip addr add ${flow}/32 dev lo 2>/dev/null
> +			out="$(${ns_a} ip route get ${dst} from ${flow} 2>/dev/null)"
> +		else
> +			flow="${i}"
> +			out="$(${ns_a} ip route get ${dst} from ${host6_a_addr} \
> +				ipproto 58 flowlabel ${flow} 2>/dev/null)"

[Severity: Medium]

What happens here if the installed iproute2 doesn't support the flowlabel
keyword for ip route get? stderr goes to /dev/null, so every iteration
returns empty output. The " dev ${oif} " pattern never matches, and
mp_flow_via() returns 1 after 32 iterations.

test_pmtu_ipvX_mp_asym_exceptions() then runs:

    err "  no flow through veth_A-R1 found"; return 1;

So pmtu_ipv6_mp_asym_exceptions would be reported as a failure instead of
$ksft_skip. The error message also points away from the real cause.

Other selftests check for this first. For example, fib_rule_tests.sh runs:

    fib_check_iproute_support "flowlabel" "flowlabel"

before it uses flowlabel. Could a similar check go here, returning
$ksft_skip when the keyword isn't supported?

> +		fi
> +		case "${out}" in
> +		*" dev ${oif} "*)
> +			echo "${flow}"
> +			return 0
> +			;;
> +		esac
> +	done
> +
> +	return 1
> +}
> +
> +test_pmtu_ipvX_mp_asym_exceptions() {

[ ... ]

> +	if [ ${family} -eq 4 ]; then
> +		# Routes back to the source addresses mp_flow_via() picks
> +		run_cmd ${ns_r1} ip route add ${host4_a_net}.0/24 via ${prefix4}.${a_r1}.2
> +		run_cmd ${ns_r2} ip route add ${host4_a_net}.0/24 via ${prefix4}.${a_r2}.2
> +	fi

[Severity: Medium]

Are these gateways right? The addressing comment in pmtu.sh gives ID 1 to
hosts and ID 2 to routers. That makes ${prefix4}.${a_r1}.2 R1's own address
on veth_R1-A, not host A's.

In this same patch, setup_multipath() routes the routers back to host A
through the .1 address:

    nh_r1_a=${prefix4}.${a_r1}.1
    ...
    nh_r2_a=${prefix4}.${a_r2}.1

The kernel still accepts a local address as the gateway.
fib_check_nh_v4_gw() allows RTN_LOCAL and sets the nexthop scope to
RT_SCOPE_HOST. rt_set_nexthop() only uses the gateway when the scope is
RT_SCOPE_LINK. So the route acts as an on-link route, and R1/R2 send ARP
requests for 192.168.97.N directly.

That address is on host A's lo, not on veth_A-R1 or veth_A-R2. Host A only
answers the ARP when arp_ignore is 0. pmtu.sh never sets arp_ignore, and new
namespaces inherit the IPv4 devconf from init_net by default.

On a host with net.ipv4.conf.all.arp_ignore >= 1, wouldn't the ICMP
fragmentation needed from R1 and the echo replies both be dropped? The test
would then fail with "PMTU exception wasn't created after exceeding MTU via
R1 (veth_A-R1)", and the R2 step would hit the same problem.

Should these be ${prefix4}.${a_r1}.1 and ${prefix4}.${a_r2}.1 instead?

[ ... ]

> +	flow_r1="$(mp_flow_via ${family} ${dst} veth_A-R1)" || {
> +		err "  no flow through veth_A-R1 found"; return 1; }
> +	flow_r2="$(mp_flow_via ${family} ${dst} veth_A-R2)" || {
> +		err "  no flow through veth_A-R2 found"; return 1; }
> +	if [ ${family} -eq 4 ]; then
> +		ping_r1="${ping} -I ${flow_r1}"
> +		ping_r2="${ping} -I ${flow_r2}"
> +	else
> +		ping_r1="${ping} -I ${host6_a_addr} -F ${flow_r1}"
> +		ping_r2="${ping} -I ${host6_a_addr} -F ${flow_r2}"
> +	fi

[Severity: Low]

mp_flow_via() returns a decimal flow label (flow="${i}", with i from 1 to
32), and that string goes unchanged to ping -F here. ping documents -F as
the flow label in hex, and older iputils ping6 builds parse it as base 16.

If the first matching label is 10 or higher, wouldn't the label on the wire
differ from the one probed with ip route get? For example, "10" becomes
0x10. The IPv6 multipath hash includes the flow label, so the echo request
could take the other nexthop, and the 1400/1300 checks would fail.

This depends on how the installed iputils and iproute2 parse the value. The
multipath hash seed is also random per boot, so failures would be
intermittent.

Would passing the label with a 0x prefix to both tools remove the ambiguity?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110095903.84813-1-deliran%40verdict.gg