[PATCH mptcp-next v2 0/5] mptcp: reset fully_estab after fallback & reject MPJ earlier

[PATCH mptcp-next v2 0/5] mptcp: reset fully_estab after fallback & reject MPJ earlier

[Matthieu Baerts (NGI0)]

Bailing out early when we can: reset the fully established when a
fallback is done, then reject the MPJ earlier, and finally stop
processing connections early with the in-kernel PM. Plus two
refactoring patches.

Maybe the first patch should be backported? Eventually the second
and fourth ones if we find related issues? (That's why I put the
refactoring patches after.)

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- patch 2: why the new racy check is OK, +ed, support old kernels & synC
- patch 3: new
- patch 4: safer approach
- patch 5: new
- Link to v1: https://patch.msgid.link/20260814-mptcp-reset-mpj-early-v1-0-3ca3260f9474@kernel.org

---
Matthieu Baerts (NGI0) (5):
      mptcp: no longer fully_established after a fallback
      mptcp: reject MP_JOIN earlier
      mptcp: subflow: split subflow_check_req()
      mptcp: pm: kernel: skip operating on closing connections
      mptcp: pm: kernel: avoid mptcp_pm_is_userspace checks

 net/mptcp/mib.c                                 |   1 +
 net/mptcp/mib.h                                 |   1 +
 net/mptcp/pm_kernel.c                           |  18 +-
 net/mptcp/protocol.c                            |   1 +
 net/mptcp/subflow.c                             | 210 +++++++++++++-----------
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  55 +++++--
 6 files changed, 171 insertions(+), 115 deletions(-)
---
base-commit: bcc5638fabf418e763621a432840cf55287e604d
change-id: 20260814-mptcp-reset-mpj-early-dcc7d4d92df9

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v2 1/5] mptcp: no longer fully_established after a fallback

[Matthieu Baerts (NGI0)]

When a fallback is done, the MPTCP_FALLBACK_DONE bit is set, which is
protected by the fallback lock. That's good, but while at it, the msk
fully established state can also be reset, to easily prevent some
operations.

This is just an optimisation to bail early in some situations: with the
PM, or when receiving an MP_JOIN. This is not a protection that requires
locks to avoid races: the fallback lock and other variables are there
for that.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..f11393e51757 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -94,6 +94,7 @@ bool __mptcp_try_fallback(struct mptcp_sock *msk, int fb_mib)
 		return false;
 	}
 
+	WRITE_ONCE(msk->fully_established, false);
 	msk->allow_subflows = false;
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
 	clear_bit(MPTCP_RTX_ENABLED, &msk->flags);

-- 
2.55.0

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

[Matthieu Baerts (NGI0)]

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)
 {
 	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);
 	if (!msk) {
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
+		*reason = MPTCP_RST_EMPTCP;
+		return NULL;
+	}
+
+	/* 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);
 		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;
 		}
 
@@ -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_req, 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
@@ -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
 

-- 
2.55.0

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

[sashiko-bot]

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

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

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 12310 bytes --]

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
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.

>  
> @@ -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?

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
>  

[-- Attachment #2: 0001-Squash-to-mptcp-subflow-split-subflow_check_req.patch --]
[-- Type: text/x-patch, Size: 4332 bytes --]

From d8c7793a9eef189987d93f8e1ef52c5842758e27 Mon Sep 17 00:00:00 2001
Message-ID: <d8c7793a9eef189987d93f8e1ef52c5842758e27.1793334905.git.geliang@kernel.org>
From: Geliang Tang <geliang@kernel.org>
Date: Fri, 30 Oct 2026 12:35:01 +0800
Subject: [PATCH] Squash to "mptcp: subflow: split subflow_check_req()"

Signed-off-by: Geliang Tang <geliang@kernel.org>
---
 net/mptcp/subflow.c | 71 +++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3134fe8ad55b..8226fb1e3fde 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -117,6 +117,47 @@ static void subflow_req_create_thmac(struct mptcp_subflow_request_sock *subflow_
 	subflow_req->thmac = get_unaligned_be64(hmac);
 }
 
+static void subflow_add_reset_reason(struct sk_buff *skb, u8 reason);
+
+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);
+	int local_id;
+
+	subflow_req->msk = mptcp_token_get_sock(sock_net(req_to_sk(req)),
+						subflow_req->token);
+
+	/* Can't fall back to TCP in this case. */
+	if (!subflow_req->msk) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+		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(subflow_req->msk)) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINDISALLOWED);
+		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
+		return -EPERM;
+	}
+
+	local_id = mptcp_pm_get_local_id(subflow_req->msk,
+					 (struct sock_common *)req);
+	if (local_id < 0) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+		return -EPERM;
+	}
+	subflow_req->local_id = local_id;
+	subflow_req->request_bkup = mptcp_pm_is_backup(subflow_req->msk,
+						       (struct sock_common *)req);
+	return 0;
+}
+
 static void subflow_init_req(struct request_sock *req, const struct sock *sk_listener)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
@@ -157,7 +198,6 @@ static int subflow_check_req_mp_join(struct request_sock *req,
 				     struct mptcp_options_received *mp_opt)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
-	int local_id;
 
 	subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
 	subflow_req->mp_join = 1;
@@ -165,26 +205,9 @@ static int subflow_check_req_mp_join(struct request_sock *req,
 	subflow_req->remote_id = mp_opt->join_id;
 	subflow_req->token = mp_opt->token;
 	subflow_req->remote_nonce = mp_opt->nonce;
-	/* The msk refcount will be decremented in subflow_req_destructor */
-	subflow_req->msk = mptcp_token_get_sock(sock_net(req_to_sk(req)),
-						subflow_req->token);
 
-	/* Can't fall back to TCP in this case. */
-	if (!subflow_req->msk) {
-		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
-		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+	if (subflow_token_join_request(req, skb))
 		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(subflow_req->msk)) {
-		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINDISALLOWED);
-		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
-		return -EPERM;
-	}
 
 	if (subflow_use_different_sport(subflow_req->msk, sk_listener)) {
 		pr_debug("syn inet_sport=%d %d\n",
@@ -198,16 +221,6 @@ static int subflow_check_req_mp_join(struct request_sock *req,
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX);
 	}
 
-	local_id = mptcp_pm_get_local_id(subflow_req->msk,
-					 (struct sock_common *)req);
-	if (local_id < 0) {
-		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
-		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
-		return -EPERM;
-	}
-	subflow_req->local_id = local_id;
-	subflow_req->request_bkup = mptcp_pm_is_backup(subflow_req->msk,
-						       (struct sock_common *)req);
 	subflow_req_create_thmac(subflow_req);
 
 	if (unlikely(req->syncookie))
-- 
2.55.0

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

[Matthieu Baerts]

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.

[PATCH mptcp-next v2 3/5] mptcp: subflow: split subflow_check_req()

[Matthieu Baerts (NGI0)]

subflow_check_req() is quite long, and hard to read. Plus the
subflow_token_join_request helper is confusing: it also modifies
subflow_req to set the local_id and request_bkup.

This is a pure refactoring, to reduce subflow_check_req size, no
behavioural changes intended. With one exception: it is no longer
needed to call sock_put(msk) when new subflows are not accepted,
or the local_id is negative, because subflow_req->msk is now set
earlier, and the msk refcount will be decremented in the destructor.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: new. (Paolo)
---
 net/mptcp/subflow.c | 219 +++++++++++++++++++++++++++-------------------------
 1 file changed, 114 insertions(+), 105 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index f9785e20e209..be6c8ce48fcd 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -117,44 +117,6 @@ 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,
-						     u8 *reason)
-{
-	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
-	struct mptcp_sock *msk;
-	int local_id;
-
-	msk = mptcp_token_get_sock(sock_net(req_to_sk(req)), subflow_req->token);
-	if (!msk) {
-		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
-		*reason = MPTCP_RST_EMPTCP;
-		return NULL;
-	}
-
-	/* 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);
-		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;
-	}
-	subflow_req->local_id = local_id;
-	subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
-
-	return msk;
-}
-
 static void subflow_init_req(struct request_sock *req, const struct sock *sk_listener)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
@@ -189,6 +151,112 @@ static int subflow_reset_req_endp(struct request_sock *req, struct sk_buff *skb)
 	return -EPERM;
 }
 
+static int subflow_check_req_mp_join(struct request_sock *req,
+				     const struct sock *sk_listener,
+				     struct sk_buff *skb,
+				     struct mptcp_options_received *mp_opt)
+{
+	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
+	int local_id;
+
+	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;
+	/* The msk refcount will be decremented in subflow_req_destructor */
+	subflow_req->msk = mptcp_token_get_sock(sock_net(req_to_sk(req)),
+						subflow_req->token);
+
+	/* Can't fall back to TCP in this case. */
+	if (!subflow_req->msk) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+		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(subflow_req->msk)) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINDISALLOWED);
+		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
+		return -EPERM;
+	}
+
+	if (subflow_use_different_sport(subflow_req->msk, sk_listener)) {
+		pr_debug("syn inet_sport=%d %d\n",
+			 ntohs(inet_sk(sk_listener)->inet_sport),
+			 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
+		if (!mptcp_pm_announced_has_ssk(subflow_req->msk, sk_listener)) {
+			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
+			subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
+			return -EPERM;
+		}
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX);
+	}
+
+	local_id = mptcp_pm_get_local_id(subflow_req->msk,
+					 (struct sock_common *)req);
+	if (local_id < 0) {
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+		return -EPERM;
+	}
+	subflow_req->local_id = local_id;
+	subflow_req->request_bkup = mptcp_pm_is_backup(subflow_req->msk,
+						       (struct sock_common *)req);
+	subflow_req_create_thmac(subflow_req);
+
+	if (unlikely(req->syncookie))
+		subflow_init_req_cookie_join_save(subflow_req, skb);
+
+	pr_debug("token=%u, remote_nonce=%u msk=%p\n", subflow_req->token,
+		 subflow_req->remote_nonce, subflow_req->msk);
+
+	return 0;
+}
+
+static int subflow_check_req_mp_capable(struct request_sock *req,
+					struct sk_buff *skb)
+{
+	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
+	int err, retries = MPTCP_TOKEN_MAX_RETRIES;
+
+	subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
+again:
+	do {
+		get_random_bytes(&subflow_req->local_key,
+				 sizeof(subflow_req->local_key));
+	} while (subflow_req->local_key == 0);
+
+	if (unlikely(req->syncookie)) {
+		mptcp_crypto_key_sha(subflow_req->local_key,
+				     &subflow_req->token,
+				     &subflow_req->idsn);
+		if (mptcp_token_exists(subflow_req->token)) {
+			if (retries-- > 0)
+				goto again;
+			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_TOKENFALLBACKINIT);
+		} else {
+			subflow_req->mp_capable = 1;
+		}
+		return 0;
+	}
+
+	err = mptcp_token_new_request(req);
+	if (err == 0)
+		subflow_req->mp_capable = 1;
+	else if (retries-- > 0)
+		goto again;
+	else
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_TOKENFALLBACKINIT);
+
+	return 0;
+}
+
 /* Init mptcp request socket.
  *
  * Returns an error code if a JOIN has failed and a TCP reset
@@ -234,74 +302,15 @@ static int subflow_check_req(struct request_sock *req,
 		return subflow_reset_req_endp(req, skb);
 	}
 
-	if (opt_mp_capable && listener->request_mptcp) {
-		int err, retries = MPTCP_TOKEN_MAX_RETRIES;
+	if (!listener->request_mptcp)
+		return 0;
 
-		subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
-again:
-		do {
-			get_random_bytes(&subflow_req->local_key, sizeof(subflow_req->local_key));
-		} while (subflow_req->local_key == 0);
+	if (opt_mp_capable)
+		return subflow_check_req_mp_capable(req, skb);
 
-		if (unlikely(req->syncookie)) {
-			mptcp_crypto_key_sha(subflow_req->local_key,
-					     &subflow_req->token,
-					     &subflow_req->idsn);
-			if (mptcp_token_exists(subflow_req->token)) {
-				if (retries-- > 0)
-					goto again;
-				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_TOKENFALLBACKINIT);
-			} else {
-				subflow_req->mp_capable = 1;
-			}
-			return 0;
-		}
-
-		err = mptcp_token_new_request(req);
-		if (err == 0)
-			subflow_req->mp_capable = 1;
-		else if (retries-- > 0)
-			goto again;
-		else
-			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, &reason);
-
-		/* Can't fall back to TCP in this case. */
-		if (!subflow_req->msk) {
-			subflow_add_reset_reason(skb, reason);
-			return -EPERM;
-		}
-
-		if (subflow_use_different_sport(subflow_req->msk, sk_listener)) {
-			pr_debug("syn inet_sport=%d %d\n",
-				 ntohs(inet_sk(sk_listener)->inet_sport),
-				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
-			if (!mptcp_pm_announced_has_ssk(subflow_req->msk, sk_listener)) {
-				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
-				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
-				return -EPERM;
-			}
-			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX);
-		}
-
-		subflow_req_create_thmac(subflow_req);
-
-		if (unlikely(req->syncookie))
-			subflow_init_req_cookie_join_save(subflow_req, skb);
-
-		pr_debug("token=%u, remote_nonce=%u msk=%p\n", subflow_req->token,
-			 subflow_req->remote_nonce, subflow_req->msk);
-	}
+	if (opt_mp_join)
+		return subflow_check_req_mp_join(req, sk_listener, skb,
+						 &mp_opt);
 
 	return 0;
 }
@@ -2148,7 +2157,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->token = subflow_req->token;
 
 		/* the subflow req id is valid, fetched via subflow_check_req()
-		 * and subflow_token_join_request()
+		 * and subflow_check_req_mp_join()
 		 */
 		subflow_set_local_id(new_ctx, subflow_req->local_id);
 	}

-- 
2.55.0

[PATCH mptcp-next v2 4/5] mptcp: pm: kernel: skip operating on closing connections

[Matthieu Baerts (NGI0)]

When iterating over each MPTCP connection after having manipulated MPTCP
endpoints, there is no need to operate on closing (or not ready)
connections.

We can then safely skip those.

Note that mptcp_nl_add_subflow_or_signal_addr() was already checking the
fully_established state, but it is better to check the connection state
with mptcp_is_fully_established(): no need to create new subflows or
signal new addresses if we are closing.

For the other states, better not to check the connection state: it might
still be interesting to do some signalling while the connection is half
closed.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: don't check the connection state, except for new subflow/ADD_ADDR.
---
 net/mptcp/pm_kernel.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..096ffea80f61 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -969,7 +969,7 @@ static int mptcp_nl_add_subflow_or_signal_addr(struct net *net,
 		struct sock *sk = (struct sock *)msk;
 		struct mptcp_addr_info mpc_addr;
 
-		if (!READ_ONCE(msk->fully_established) ||
+		if (!mptcp_is_fully_established(sk) ||
 		    mptcp_pm_is_userspace(msk))
 			goto next;
 
@@ -1095,7 +1095,8 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		struct sock *sk = (struct sock *)msk;
 		bool remove_subflow;
 
-		if (mptcp_pm_is_userspace(msk))
+		if (!READ_ONCE(msk->fully_established) ||
+		    mptcp_pm_is_userspace(msk))
 			goto next;
 
 		lock_sock(sk);
@@ -1141,7 +1142,9 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 		struct mptcp_addr_info anno_addr;
 		bool announced;
 
-		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
+		if (list_empty(&msk->conn_list) ||
+		    !READ_ONCE(msk->fully_established) ||
+		    mptcp_pm_is_userspace(msk))
 			goto next;
 
 		mptcp_local_address((struct sock_common *)msk, &msk_local);
@@ -1288,7 +1291,8 @@ static void mptcp_nl_flush_addrs_list(struct net *net,
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 
-		if (!mptcp_pm_is_userspace(msk)) {
+		if (READ_ONCE(msk->fully_established) &&
+		    !mptcp_pm_is_userspace(msk)) {
 			lock_sock(sk);
 			mptcp_pm_flush_addrs_and_subflows(msk, rm_list, NULL);
 			release_sock(sk);
@@ -1502,7 +1506,9 @@ static void mptcp_pm_nl_set_flags_all(struct net *net,
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 
-		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
+		if (list_empty(&msk->conn_list) ||
+		    !READ_ONCE(msk->fully_established) ||
+		    mptcp_pm_is_userspace(msk))
 			goto next;
 
 		lock_sock(sk);

-- 
2.55.0

[PATCH mptcp-next v2 5/5] mptcp: pm: kernel: avoid mptcp_pm_is_userspace checks

[Matthieu Baerts (NGI0)]

It is strange to see them in the kernel code, plus it is more logical
that way: only do in-kernel PM specific for this PM.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: new
---
 net/mptcp/pm_kernel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 096ffea80f61..670db79ff4d3 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -970,7 +970,7 @@ static int mptcp_nl_add_subflow_or_signal_addr(struct net *net,
 		struct mptcp_addr_info mpc_addr;
 
 		if (!mptcp_is_fully_established(sk) ||
-		    mptcp_pm_is_userspace(msk))
+		    !mptcp_pm_is_kernel(msk))
 			goto next;
 
 		/* if the endp linked to the init sf is re-added with a != ID */
@@ -1096,7 +1096,7 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		bool remove_subflow;
 
 		if (!READ_ONCE(msk->fully_established) ||
-		    mptcp_pm_is_userspace(msk))
+		    !mptcp_pm_is_kernel(msk))
 			goto next;
 
 		lock_sock(sk);
@@ -1144,7 +1144,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 
 		if (list_empty(&msk->conn_list) ||
 		    !READ_ONCE(msk->fully_established) ||
-		    mptcp_pm_is_userspace(msk))
+		    !mptcp_pm_is_kernel(msk))
 			goto next;
 
 		mptcp_local_address((struct sock_common *)msk, &msk_local);
@@ -1292,7 +1292,7 @@ static void mptcp_nl_flush_addrs_list(struct net *net,
 		struct sock *sk = (struct sock *)msk;
 
 		if (READ_ONCE(msk->fully_established) &&
-		    !mptcp_pm_is_userspace(msk)) {
+		    mptcp_pm_is_kernel(msk)) {
 			lock_sock(sk);
 			mptcp_pm_flush_addrs_and_subflows(msk, rm_list, NULL);
 			release_sock(sk);
@@ -1508,7 +1508,7 @@ static void mptcp_pm_nl_set_flags_all(struct net *net,
 
 		if (list_empty(&msk->conn_list) ||
 		    !READ_ONCE(msk->fully_established) ||
-		    mptcp_pm_is_userspace(msk))
+		    !mptcp_pm_is_kernel(msk))
 			goto next;
 
 		lock_sock(sk);

-- 
2.55.0

Re: [PATCH mptcp-next v2 0/5] mptcp: reset fully_estab after fallback & reject MPJ earlier

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/40975021307

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/78ffe8cb00da
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1196695


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)