Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: join: validate subflow retry after link failure

[netdev-bot+sashiko@kernel.org]

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

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

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] The post-close check in link_failure_tests() sets
  `net.mptcp.close_timeout=5` and says 'stopped at the latest by the…
- [Low] The new test never checks whether its netfilter rules were
  installed.
- [Low] link_failure_tests() now ends with `cinfail=""` and `sinfail=""`.
- [Low] The commit message says 'The big files used for the long
  connection are removed at the end'.
- [Low] The first sync point with the background job ignores its result:
  `wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1` at line 2676.

--

selftests: mptcp: join: validate subflow retry after link failure

This adds a test to mptcp_join.sh. While a long transfer runs in the
background, nftables drops the client packets from one subflow address,
and the test checks that the subflow is re-created from the same address
once the packets go through again. It does the same for the initial
subflow, then closes the connection while a subflow is being re-created
and checks the netlink events.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..bbeff6739c5ac 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2151,6 +2151,69 @@ wait_attempt_fail()

[ ... ]

> +sf_retry_after_block()
> +{
> +	local syn_tx ack_rx before after
> +
> +	syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
> +	ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
> +	before=$(sf_from "${1}")
> +
> +	print_check "subflow from ${1}"
> +	if [ -z "${before}" ]; then
> +		fail_test "no established subflow from ${1} before the block"
> +		return
> +	fi
> +
> +	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
> +	if ! wait_counter $ns2 "MPTcpExtMPJoinSynTx" $((${syn_tx:-0} + 1)); then
> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		fail_test "subflow from ${1} not re-created"
> +		return
> +	fi

[Severity: Low]
What happens if this nft rule can't be installed? The test ignores the
result of init_nftables, of this nft add rule, and of the drop rule added
in the third phase in link_failure_tests().

If the insertion fails, the subflow keeps working and wait_counter times
out. The test then reports "subflow from X not re-created" (or "no
subflow re-created before the close") when it should have been skipped.

The other nft add rule users in mptcp_join.sh check the result and skip.
For example, reset_with_tcp_filter():

	if ! ip netns exec "$ns" nft add rule ip filter "${chain}" \
			ip saddr "${src}" meta l4proto tcp "${target,,}"; then
		mark_as_skipped "unable to set the filter rules"

Could this test use the same check-and-skip pattern? In practice this
only shows up on kernels without IPv4 nftables support, since the
selftest config gets it via CONFIG_NF_TABLES_INET.

[ ... ]

> @@ -2585,6 +2648,67 @@ link_failure_tests()
>  		chk_stale_nr $ns2 1 -1 2
>  		chk_link_usage $ns2 ns2eth3 $cinsent 50
>  	fi
> +
> +	# subflows closed after a silent link failure, then re-created: the
> +	# additional one, then the initial one. Then close the connection
> +	# while a subflow is being re-created.
> +	if reset_with_events "subflow retry after link failure" &&
> +	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_kernel_retry$"; then
> +		init_nftables
> +		init_shapers
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> +		ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
> +		pm_nl_set_limits $ns1 0 4
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> +		# created here, not in the background job, for cleanup()
> +		if [ -z "$cinfail" ]; then
> +			cinfail=$(mktemp)
> +		fi
> +		if [ -z "$sinfail" ]; then
> +			sinfail=$(mktemp)
> +		fi
> +		{ timeout_test=120 test_linkfail=131072 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1

[Severity: Low]
Should the return value of this wait_counter be checked?

mktemp only creates empty files in the foreground. The background
run_tests() still fills both 128 MiB files from /dev/urandom with
bs=1024 before mptcp_connect starts:

	if [ -z "$cinfail" ]; then
		cinfail=$(mktemp)
	fi
	make_file "$cinfail" "client" $size

On slow or debug kernels (KASAN, lockdep), could that plus the
connection setup take longer than the ~30 second wait_counter budget?

If so, sf_retry_after_block 10.0.3.2 runs before any join is
established. It fails with the misleading "no established subflow from
10.0.3.2 before the block", and the later checks fail too.

> +		sf_retry_after_block 10.0.3.2
> +		sf_retry_after_block 10.0.1.2
> +
> +		ip netns exec $ns2 nft add rule ip filter OUTPUT \
> +			ip saddr 10.0.3.2 drop
> +		wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
> +		local retrying=$?
> +		mptcp_lib_kill_group_wait $tests_pid
> +
> +		# stopped at the latest by the close timeout
> +		print_check "no subflow connecting after close"
> +		for _ in $(seq $((timeout_poll * 10))); do
> +			ip netns exec $ns2 ss -tnH state syn-sent | grep -q . ||
> +				break
> +			sleep 0.1
> +		done

[Severity: Medium]
Does the net.mptcp.close_timeout=5 setting above apply at this point?

After mptcp_lib_kill_group_wait, the msk is orphaned. __mptcp_close()
calls sock_orphan(sk), which sets SOCK_DEAD, before it calls
mptcp_start_tout_timer(sk). For a dead msk, mptcp_close_timeout()
ignores the sysctl:

net/mptcp/ctrl.c:mptcp_close_timeout() {
	if (sock_flag(sk, SOCK_DEAD))
		return TCP_TIMEWAIT_LEN;
	return mptcp_get_pernet(sock_net(sk))->close_timeout;
}

So the guaranteed bound is 60 seconds, but this loop polls for only
timeout_poll * 10 * 0.1 seconds (about 30 seconds).

mptcp_pm_kernel_retry() sets icsk_syn_retries to 15 on the SYN-SENT
subflow. The subflow only goes away sooner in one of two ways:

  - a fastclose, when one side has unread data at close
  - the client DATA_FIN, sent through
    mptcp_check_send_data_fin()->mptcp_subflow_shutdown()->tcp_disconnect()

The DATA_FIN goes out only after the whole pending send queue has been
pushed.

If both sides close gracefully and the client send queue doesn't drain
in time, could this report "subflow still connecting after close" on a
correct kernel? In that case the comment "stopped at the latest by the
close timeout" would not hold either.

[ ... ]

> +		# big files: do not keep them, the next tests create their own
> +		rm -f "$cinfail" "$sinfail"

[Severity: Low]
The commit message says:

    The big files used for the long connection are removed at the end.

Doesn't $cinsent also hold a big file at this point? With
test_linkfail=131072, do_transfer() copies the client stream into it:

	tee "$cinsent" < "$cinfail" | \

That can be tens of MB, up to 128 MB. Only a later do_transfer() with
test_linkfail != 0 rewrites $cinsent; in a full run that is
fastclose_tests(), many subtests later. Otherwise cleanup() removes it
at exit.

On a tmpfs /tmp, that memory stays in use through all the subtests in
between. Should $cinsent be removed or truncated here as well?

> +		cinfail=""
> +		sinfail=""

[Severity: Low]
Can clearing these variables leave temp files behind in later subtests?

Before this patch, link_failure_tests() left cinfail and sinfail
pointing at files owned by the parent shell.

Later subtests in userspace_tests() and endpoint_tests() call
run_tests() in a background subshell, for example:

	{ timeout_test=120 test_linkfail=128 speed=5 \

Inside the subshell, run_tests() sees empty variables and creates new
files:

	if [ -z "$cinfail" ]; then
		cinfail=$(mktemp)
	fi

These assignments never reach the parent shell, so cleanup() at exit
does not remove those files:

	rm -f "$cin" "$cout" "$sinfail"
	rm -f "$sin" "$sout" "$cinsent" "$cinfail"

In a full run, fastclose_tests() runs in the foreground in between and
sets both variables again. With "mptcp_join.sh -l -u" or
"mptcp_join.sh -l -I", though, each background subtest would leave two
temp files in /tmp.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016055010.980819-1-anilkaushikwireless%40gmail.com