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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] The last step of the new test is meant to cover a specific
  kernel path: msk close while a re-created subflow is still in SYN_SENT.
- [Low] In run_tests() (mptcp_join.sh:1138-1140), the comment above the
  cinfail branch still says '# create the input file for the failure…

--

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

This adds a link failure test case to mptcp_join.sh for the
subflow-retries limit. Netfilter drops the client packets from one
address at a time, and the test checks that the subflow is re-created
from the same address. At the end the connection is closed while a
subflow is being re-created, and the netlink events are checked.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..a99cee39da63e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1137,7 +1137,7 @@ run_tests()
>  		make_file "$cinfail" "client" $size
>  	# create the input file for the failure test when
>  	# the first failure test run
> -	elif [ "$test_linkfail" -ne 0 ] && [ -z "$cinfail" ]; then
> +	elif [ "$test_linkfail" -ne 0 ] && [ ! -s "$cinfail" ]; then

[Severity: Low]
This isn't a bug, but the comment above still says the file is created
on "the first failure test run". With [ ! -s "$cinfail" ], the file is
now regenerated whenever it is missing or empty. The sinfail branch has
the same change.

The new test depends on this, because it empties cinfail and sinfail at
the end so that later tests write their own content.

Could the comment be updated to match the new condition?

[ ... ]

> @@ -2585,6 +2696,65 @@ link_failure_tests()

[ ... ]

> +		local retrying=1
> +		if nft_drop_from 10.0.3.2; then
> +			wait_counter_inc "${timeout_poll}" \
> +				$ns2 "MPTcpExtMPJoinSynTx"
> +			retrying=$?
> +		fi
> +		mptcp_lib_kill_group_wait $tests_pid
> +		chk_no_sf_connecting ${retrying}

[Severity: Medium]
Does this teardown actually exercise mptcp_close_retry_subflows()?

mptcp_lib_kill_group_wait() sends SIGTERM to every descendant. That
means both the ns1 listener and the ns2 connector are killed while the
128 MB transfers are still running in both directions.

If the client has unread data when it is killed, __mptcp_close() takes
the fastclose branch:

net/mptcp/protocol.c:__mptcp_close() {
    ...
	if (mptcp_data_avail(msk) || timeout < 0 ||
	    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
		...
		mptcp_do_fastclose(sk);
		timeout = 0;
    ...
}

The msk then becomes TCP_CLOSE. Next,
__mptcp_destroy_sock()->mptcp_destroy_common() calls __mptcp_close_ssk()
on every subflow, the SYN_SENT one included.

If the server has unread data instead, it sends MP_FASTCLOSE over the
unblocked 10.0.1.x subflow. On the client, mptcp_check_fastclose() then
moves every subflow to TCP_CLOSE.

In both cases, no_sf_connecting() in chk_no_sf_connecting() would pass
even if mptcp_close_retry_subflows() were removed or broken. The check
can only tell the difference on the non-fastclose path. On that path,
without the helper, the SYN_SENT subflow would stay until the 60 s msk
close timeout, which is longer than timeout_poll.

Is there a way to make sure the close here takes the non-fastclose path,
so that "no subflow connecting after close" really covers the new
helper?

> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		kill_events_pids

[ ... ]

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