Re: [PATCH mptcp-next v2 2/5] mptcp: reject MP_JOIN earlier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: mptcp: handle simultaneous MP_JOIN race in chk_join_nr()
--

commit a8174c281034f9fecaa480fc2e253d29198241eb
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: reject MP_JOIN earlier

This commit moves the mptcp_can_accept_new_subflow() check to
subflow_token_join_request() so disallowed MP_JOIN requests are rejected
upon receiving the SYN, tracked by a new MPJoinDisallowed MIB counter, and
updates the mptcp_join.sh selftest accordingly.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c3ccce96b45b1..308fad861afc1 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

[ ... ]

> @@ -1617,10 +1619,21 @@ chk_join_nr()
>  	if [ -z "$count" ]; then
>  		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$syn_ack_nr" ]; then
> +		# support old kernels without MPTcpExtMPJoinDisallowed counter:
> +		# => connections are rejected later on (MPTcpExtMPJoinRejected)
> +		if [ "${disallowed}" -gt 0 ] &&
> +		   ! mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinDisallowed" >/dev/null; then
> +			syn_rej=$((syn_rej + disallowed))
> +			syn_ack_nr=$((syn_ack_nr + disallowed))
> +		fi
> +
>  		# simult connections exceeding the limit with cookie enabled could go up to
>  		# synack validation as the conn limit can be enforced reliably only after
>  		# the subflow creation
> -		if [ "$with_cookie" != 2 ] || [ "$count" -le "$syn_ack_nr" ] || [ "$count" -gt "$syn_nr" ]; then
> +		if [ "$count" != "$syn_ack_nr" ] && {
> +		     [ "$with_cookie" != 2 ] ||
> +		     [ "$count" -le "$syn_ack_nr" ] ||
> +		     [ "$count" -gt "$syn_nr" ]; }; then
>  			rc=${KSFT_FAIL}
>  			print_check "synack rx"
>  			fail_test "got $count JOIN[s] synack rx expected $syn_ack_nr"
> @@ -1658,9 +1671,18 @@ chk_join_nr()
>  	if [ -z "$count" ]; then
>  		: # ignore skip
>  	elif [ "$count" != "$syn_rej" ]; then
> -		rc=${KSFT_FAIL}
> -		print_check "syn rejected"
> -		fail_test "got $count JOIN[s] syn rejected expected $syn_rej"
> +		# support old kernels without MPTcpExtMPJoinDisallowed counter:
> +		# => with syncookies, the MPJoinRejected was incremented instead
> +		if [ "$with_cookie" = 2 ] && [ "${disallowed}" -gt 0 ] &&
> +		   ! mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinDisallowed" >/dev/null; then
> +			syn_rej=$((syn_rej + disallowed))
> +		fi
> +
> +		if [ "$count" != "$syn_rej" ]; then
> +			rc=${KSFT_FAIL}
> +			print_check "syn rejected"
> +			fail_test "got $count JOIN[s] syn rejected expected $syn_rej"
> +		fi
>  	fi
>  
>  	# Per-event MPTCP_RST_EMPTCP JOIN counters; default 0, gated on
> @@ -1701,6 +1723,13 @@ chk_join_nr()
>  		fail_test "got $count JOIN[s] no id found expected $no_id_found"
>  	fi
>  
> +	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinDisallowed")
> +	if [ -n "$count" ] && [ "$count" != "$disallowed" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "join disallowed"
> +		fail_test "got $count JOIN[s] disallowed expected $disallowed"
> +	fi

[Severity: Medium]
Can simultaneous MP_JOIN attempts cause false failures in chk_join_nr() now
that subflow_token_join_request() checks mptcp_can_accept_new_subflow() on
SYN reception for both syncookie and non-syncookie cases, and uses a separate
counter from 3rd-ACK rejection?

In tests like "multiple subflows, limited by server" in subflows_tests() and
"subflows limited by server w cookies" in syncookies_tests(), the client
initiates two subflows back-to-back in
mptcp_pm_create_subflow_or_signal_addr(). On the server,
msk->pm.accept_subflow is only cleared when the first subflow completes its
3WHS in subflow_syn_recv_sock() -> mptcp_finish_join() ->
mptcp_pm_allow_new_subflow().

If the second MP_JOIN SYN arrives before the first subflow's third ACK is
processed, mptcp_can_accept_new_subflow() in subflow_token_join_request()
still returns true. The server then sends a SYN+ACK for the second subflow
and rejects it later on the third ACK in mptcp_finish_join(), incrementing
MPTcpExtMPJoinRejected instead of MPTcpExtMPJoinDisallowed.

In chk_join_nr(), the allowance for receiving more SYN+ACKs than syn_ack_nr
is still restricted to with_cookie == 2, even though the same SYN-time race
now applies without syncookies. In addition, when an extra SYN+ACK is sent
due to this race, MPTcpExtMPJoinRejected increases and
MPTcpExtMPJoinDisallowed decreases by (count - syn_ack_nr), but syn_rej and
disallowed are not adjusted accordingly before checking those two counters.

Should chk_join_nr() allow count up to syn_nr whenever disallowed > 0 (not
only when with_cookie == 2), and shift the extra (count - syn_ack_nr) from
disallowed to syn_rej when that race occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-mptcp-reset-mpj-early-v2-0-90f119392263@kernel.org?part=2