Re: [PATCH mptcp-net 2/2] selftests: mptcp: join: re-add the initial subflow endp with fullmesh

[netdev-bot+sashiko@kernel.org]

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

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

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] mptcp: pm: fill_remote_addresses_fullmesh() fails to skip
  reset initial subflow

--

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4424,6 +4424,36 @@ endpoint_tests()
>  		chk_rm_nr 4 4
>  	fi
>  
> +	# remove and re-add the endpoint of the initial subflow, with fullmesh
> +	if reset_with_events "delete and re-add, fullmesh" &&
> +	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow,fullmesh
> +		{ timeout_test=120 test_linkfail=128 speed=5 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
> +		wait_mpj 1
> +		chk_subflow_nr "before delete id 0" 2
> +
> +		pm_nl_del_endpoint $ns2 1 10.0.1.2
> +		sleep 0.5
> +		chk_subflow_nr "after delete id 0" 1
> +
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		wait_mpj 2
> +		chk_subflow_nr "after re-add id 0" 2
> +
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		chk_join_nr 2 2 2
> +		chk_rm_nr 1 1
> +	fi
> +
>  	# remove and re-add
>  	if reset_with_events "delete re-add signal" &&
>  	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then

[Severity: Medium]
This only exercises the case where msk->first is still established
when the endpoint is removed. In that case __mptcp_subflow_disconnect()
takes the tcp_shutdown() branch and the subflow context is left intact.
Doesn't the fix from the previous patch still have a hole when the
initial subflow reaches TCP_CLOSE before mptcp_close_ssk() runs? That
can happen after a peer RST or FIN, a timeout, or a shutdown from
TCP_SYN_SENT.

In that case __mptcp_close_ssk() sets subflow->closing and keeps
msk->first in conn_list because dispose_it is false. Then
__mptcp_subflow_disconnect() calls tcp_disconnect() followed by
mptcp_subflow_ctx_reset(). The memset of subflow->reset clears closing
again, zeroes remote_id and sets local_id to -1.

When the fullmesh endpoint is then re-added,
fill_remote_addresses_fullmesh() no longer skips that subflow. The
second loop copies 0.0.0.0 port 0 with id 0 from the disconnected
socket and marks remote id 0 as unavailable, so any live subflow
towards remote id 0 is skipped. __mptcp_subflow_connect() then fails
with -EINVAL on the port 0 address. No subflow towards remote id 0
gets created, which is the same symptom this test is meant to catch.

Would checking the ssk state or the remote port in
fill_remote_addresses_fullmesh() be a more robust guard than
subflow->closing here? Could the selftest also grow a variant where
the initial subflow is reset by the peer before the endpoint is
re-added, so that path is covered too?

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