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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for the review.

On 30/10/2026 05:43, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, 2026-10-30 at 00:21 +0100, Matthieu Baerts (NGI0) wrote:
>> When a SYN + MP_JOIN is received, it is good to check directly if the
>> MP_JOIN is allowed, then checking that later at the establishment of
>> the
>> new subflow.
>>
>> This avoids situations where the 3WHS is done, then the joined
>> subflow
>> is rejected.
>>
>> This changes the behaviour, and the new behaviour is tracked with the
>> MPJoinDisallowed MIB counter.
>>
>> Note that in the selftests, we need to support older kernels not
>> following this new behaviour. For that, we can check if the new
>> counter
>> is present: if it is, the join rejected counter should be used
>> instead,
>> and we should see more SYN+ACK. One exception for the SYN cookies
>> case
>> were the reject was already done early but MPJoinRejected was used
>> instead.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> v2:
>>  - Explain that mptcp_can_accept_new_subflow() is racy, but OK.
>> (Paolo)
>>  - support older kernels with the previous behaviour.
>>  - no more special case for syncookie, switch to new counter.
>>  - s/MPJoinDisallow/MPJoinDisallowed/ (+ ed). (Sashiko)
>> ---
>>  net/mptcp/mib.c                                 |  1 +
>>  net/mptcp/mib.h                                 |  1 +
>>  net/mptcp/subflow.c                             | 31 +++++++++-----
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 55
>> +++++++++++++++++++------
>>  4 files changed, 64 insertions(+), 24 deletions(-)
>>
>> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
>> index 608cb568897c..b1710082bbca 100644
>> --- a/net/mptcp/mib.c
>> +++ b/net/mptcp/mib.c
>> @@ -34,6 +34,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>>  	SNMP_MIB_ITEM("MPJoinAckNoCtx", MPTCP_MIB_MPJOINACKNOCTX),
>>  	SNMP_MIB_ITEM("MPJoinRejected", MPTCP_MIB_JOINREJECTED),
>>  	SNMP_MIB_ITEM("MPJoinNotEstablished",
>> MPTCP_MIB_MPJOINNOTESTABLISHED),
>> +	SNMP_MIB_ITEM("MPJoinDisallowed",
>> MPTCP_MIB_MPJOINDISALLOWED),
>>  	SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
>>  	SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr",
>> MPTCP_MIB_JOINSYNTXCREATSKERR),
>>  	SNMP_MIB_ITEM("MPJoinSynTxBindErr",
>> MPTCP_MIB_JOINSYNTXBINDERR),
>> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
>> index 1ebdb55e9534..e97ee2d952a7 100644
>> --- a/net/mptcp/mib.h
>> +++ b/net/mptcp/mib.h
>> @@ -29,6 +29,7 @@ enum linux_mptcp_mib_field {
>>  	MPTCP_MIB_MPJOINACKNOCTX,	/* MP_RST: no subflow
>> context on ACK */
>>  	MPTCP_MIB_JOINREJECTED,		/* The PM rejected
>> the JOIN request */
>>  	MPTCP_MIB_MPJOINNOTESTABLISHED,	/* MP_RST: JOIN on
>> not-fully-established msk */
>> +	MPTCP_MIB_MPJOINDISALLOWED,	/* RST JOIN early if
>> disallowed: fallback/PM */
>>  	MPTCP_MIB_JOINSYNTX,		/* Sending a SYN + MP_JOIN
>> */
>>  	MPTCP_MIB_JOINSYNTXCREATSKERR,	/* Not able to create a
>> socket when sending a SYN + MP_JOIN */
>>  	MPTCP_MIB_JOINSYNTXBINDERR,	/* Not able to bind() the
>> address when sending a SYN + MP_JOIN */
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 78e886b20258..f9785e20e209 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -117,7 +117,8 @@ static void subflow_req_create_thmac(struct
>> mptcp_subflow_request_sock *subflow_
>>  	subflow_req->thmac = get_unaligned_be64(hmac);
>>  }
>>  
>> -static struct mptcp_sock *subflow_token_join_request(struct
>> request_sock *req)
>> +static struct mptcp_sock *subflow_token_join_request(struct
>> request_sock *req,
>> +						     u8 *reason)
> 
> We modified subflow_token_join_request() in this patch, but then
> removed it in the next one, which seemed a little odd to me. I suggest

I did that because it is not clear to me if this patch *might* need to
be backported to fix some issues. I don't think we should, but just in
case, I left the refactoring (patch 3/5) after this one.

> not removing this function - just changing its prototype would be
> enough. For example:
> 
> static int subflow_token_join_request(struct request_sock *req,
>                                       struct sk_buff *skb)
> 
>>  {
>>  	struct mptcp_subflow_request_sock *subflow_req =
>> mptcp_subflow_rsk(req);
>>  	struct mptcp_sock *msk;
>> @@ -126,12 +127,25 @@ static struct mptcp_sock
>> *subflow_token_join_request(struct request_sock *req)
>>  	msk = mptcp_token_get_sock(sock_net(req_to_sk(req)),
>> subflow_req->token);
> 
> We can set mptcp_token_get_sock() to "subflow_req->msk" instead of
> "msk":
> 
> subflow_req->msk = mptcp_token_get_sock(sock_net(req_to_sk(req)),
> 	                                subflow_req->token);
> 
>>  	if (!msk) {
> 
> Then we can use subflow_req->msk to replace msk:
> 
> 	if (!subflow_req->msk) {
> 
>>  		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
>> +		*reason = MPTCP_RST_EMPTCP;
> 
> Set the skb reset_reason directly:
> 
> 		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
> 
> This requires declaring subflow_add_reset_reason at the beginning of
> the file.
> 
>> +		return NULL;
> 
> No longer return "struct mptcp_sock *"; return an error code instead:
> 
> 		return -EPERM;
>> +	}
>> +
>> +	/* Stop it early if the subflow cannot be accepted.
>> +	 * This check is inherently racy, the consistency safeguard
>> is given by
>> +	 * the fallback check at finish_join time.
>> +	 */
>> +	if (!mptcp_can_accept_new_subflow(msk)) {
>> +		SUBFLOW_REQ_INC_STATS(req,
>> MPTCP_MIB_MPJOINDISALLOWED);
>> +		*reason = MPTCP_RST_EPROHIBIT;
>> +		sock_put((struct sock *)msk);
> 
> sock_put is no longer needed here, because the msk reference is now
> owned by subflow_req (set immediately) and will be released by the
> destructor.
> 
>>  		return NULL;
>>  	}
>>  
>>  	local_id = mptcp_pm_get_local_id(msk, (struct sock_common
>> *)req);
>>  	if (local_id < 0) {
>>  		SUBFLOW_REQ_INC_STATS(req,
>> MPTCP_MIB_MPJOINNOIDFOUND);
>> +		*reason = MPTCP_RST_EMPTCP;
>>  		sock_put((struct sock *)msk);
>>  		return NULL;
>>  	}
>> @@ -252,17 +266,19 @@ static int subflow_check_req(struct
>> request_sock *req,
>>  			SUBFLOW_REQ_INC_STATS(req,
>> MPTCP_MIB_TOKENFALLBACKINIT);
>>  
>>  	} else if (opt_mp_join && listener->request_mptcp) {
>> +		u8 reason;
>> +
>>  		subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
>>  		subflow_req->mp_join = 1;
>>  		subflow_req->backup = mp_opt.backup;
>>  		subflow_req->remote_id = mp_opt.join_id;
>>  		subflow_req->token = mp_opt.token;
>>  		subflow_req->remote_nonce = mp_opt.nonce;
>> -		subflow_req->msk = subflow_token_join_request(req);
>> +		subflow_req->msk = subflow_token_join_request(req,
>> &reason);
>>  
>>  		/* Can't fall back to TCP in this case. */
>>  		if (!subflow_req->msk) {
>> -			subflow_add_reset_reason(skb,
>> MPTCP_RST_EMPTCP);
>> +			subflow_add_reset_reason(skb, reason);
>>  			return -EPERM;
>>  		}
> 
> Then use subflow_token_join_request this way:
> 
> 	if (subflow_token_join_request(req, skb))
> 		return -EPERM;
> 
> This way, in the next patch we only need to extract the two functions
> subflow_check_req_mp_join() and subflow_check_req_mp_capable(), and
> subflow_token_join_request() continues to be used in
> subflow_check_req_mp_join().
> 
> WDYT? Attached is a squash-to patch with my changes.

I think I would prefer keeping the refactoring (patch 3/5) and move it
before this one (2/5), no?

I don't think that's a good idea to assign various fields of subflow_req
in subflow_token_join_request(). That's why I removed this function.

>> @@ -280,15 +296,8 @@ static int subflow_check_req(struct request_sock
>> *req,
>>  
>>  		subflow_req_create_thmac(subflow_req);
>>  
>> -		if (unlikely(req->syncookie)) {
>> -			if
>> (!mptcp_can_accept_new_subflow(subflow_req->msk)) {
>> -				SUBFLOW_REQ_INC_STATS(req,
>> MPTCP_MIB_JOINREJECTED);
>> -				subflow_add_reset_reason(skb,
>> MPTCP_RST_EPROHIBIT);
>> -				return -EPERM;
>> -			}
>> -
>> +		if (unlikely(req->syncookie))
>>  			subflow_init_req_cookie_join_save(subflow_re
>> q, skb);
>> -		}
>>  
>>  		pr_debug("token=%u, remote_nonce=%u msk=%p\n",
>> subflow_req->token,
>>  			 subflow_req->remote_nonce, subflow_req-
>>> msk);
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index c3ccce96b45b..308fad861afc 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> 
> Also, could the selftests part be put into a separate patch?

I don't know: here, we are changing the behaviour, so without it, the
selftests are broken. Because this patch 2/5 is quite big, I can split
it, but then I need to clearly state that the kernel code changes the
behaviour and a modification of the selftests is required. Hopefully we
will not need to backport this patch 2/5, so the split is OK.

> Thanks,
> -Geliang
> 
>> @@ -78,6 +78,7 @@ unset join_ack_no_mpjoin
>>  unset join_ack_no_ctx
>>  unset join_not_established
>>  unset join_no_id_found
>> +unset join_disallowed
>>  
>>  unset rst_md5sig
>>  unset rst_dss
>> @@ -1595,6 +1596,7 @@ chk_join_nr()
>>  	local ack_no_ctx=${join_ack_no_ctx:-0}
>>  	local not_established=${join_not_established:-0}
>>  	local no_id_found=${join_no_id_found:-0}
>> +	local disallowed=${join_disallowed:-0}
>>  	local rc=${KSFT_PASS}
>>  	local count
>>  	local with_cookie
>> @@ -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
>> +
>>  	print_results "join Rx" ${rc}
>>  
>>  	join_syn_tx="${join_syn_tx:-${syn_nr}}" \
>> @@ -2180,8 +2209,8 @@ subflows_tests()
>>  		pm_nl_set_limits $ns2 0 1
>>  		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>  		run_tests $ns1 $ns2 10.0.1.1
>> -		join_syn_rej=1 \
>> -			chk_join_nr 1 1 0
>> +		join_disallowed=1 \
>> +			chk_join_nr 1 0 0
>>  	fi
>>  
>>  	# subflow
>> @@ -2210,8 +2239,8 @@ subflows_tests()
>>  		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>  		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
>>  		run_tests $ns1 $ns2 10.0.1.1
>> -		join_syn_rej=1 \
>> -			chk_join_nr 2 2 1
>> +		join_disallowed=1 \
>> +			chk_join_nr 2 1 1
>>  	fi
>>  
>>  	# single subflow, dev
>> @@ -3546,7 +3575,7 @@ syncookies_tests()
>>  		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>  		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
>>  		run_tests $ns1 $ns2 10.0.1.1
>> -		join_syn_rej=1 \
>> +		join_disallowed=1 \
>>  			chk_join_nr 2 1 1
>>  	fi
>>  
>> @@ -4092,8 +4121,8 @@ userspace_tests()
>>  		pm_nl_set_limits $ns2 1 1
>>  		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>  		run_tests $ns1 $ns2 10.0.1.1
>> -		join_syn_rej=1 \
>> -			chk_join_nr 1 1 0
>> +		join_disallowed=1 \
>> +			chk_join_nr 1 0 0
>>  	fi
>>  
>>  	# userspace pm type does not send join
>> @@ -4116,8 +4145,8 @@ userspace_tests()
>>  		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>  		sflags=backup speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>> -		join_syn_rej=1 \
>> -			chk_join_nr 1 1 0
>> +		join_disallowed=1 \
>> +			chk_join_nr 1 0 0
>>  		chk_prio_nr 0 0 0 0
>>  	fi
>>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.