[PATCH mptcp-next 0/5] mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected

[PATCH mptcp-next 0/5] mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected

[Matthieu Baerts (NGI0)]

Recently, during a debugging session using local MPTCP connections, I
noticed MPJoinAckHMacFailure was strangely not zero on the server side.

The first patch fixes this issue, and the second one validates it. These
two patches can be applied in mptcp-net.

There is a small refactoring in the 3rd patch.

Patch 4 adds MPJoinRejected to track when the PM rejects patches, and
patch 5 validates it.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (5):
      mptcp: only inc MPJoinAckHMacFailure for HMAC failures
      selftests: mptcp: validate MPJoin HMacFailure counters
      mptcp: pass right struct to subflow_hmac_valid
      mptcp: add MPJoinRejected MIB counter
      selftests: mptcp: validate MPJoinRejected counter

 net/mptcp/mib.c                                 |  1 +
 net/mptcp/mib.h                                 |  1 +
 net/mptcp/protocol.c                            |  4 ++-
 net/mptcp/subflow.c                             | 18 +++++-----
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 44 ++++++++++++++++++++++---
 5 files changed, 54 insertions(+), 14 deletions(-)
---
base-commit: aca18798220853f0d7a8d01c28213e0d3fccfbfc
change-id: 20250328-mptcp-mpj-reject-addd081553f5

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

[PATCH mptcp-next 1/5] mptcp: only inc MPJoinAckHMacFailure for HMAC failures

[Matthieu Baerts (NGI0)]

Recently, during a debugging session using local MPTCP connections, I
noticed MPJoinAckHMacFailure was not zero on the server side. The
counter was in fact incremented when the PM rejected new subflows,
because the 'subflow' limit was reached.

The fix is easy, simply dissociating the two cases: only the HMAC
validation check should increase MPTCP_MIB_JOINACKMAC counter.

Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Note: this is a fix for mptcp-net
---
 net/mptcp/subflow.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 409bd415ef1d190d5599658d01323ad8c8a9be93..24c2de1891bdf31dfe04ef2077113563aad0e666 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -899,13 +899,17 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 				goto dispose_child;
 			}
 
-			if (!subflow_hmac_valid(req, &mp_opt) ||
-			    !mptcp_can_accept_new_subflow(subflow_req->msk)) {
+			if (!subflow_hmac_valid(req, &mp_opt)) {
 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
 				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				goto dispose_child;
 			}
 
+			if (!mptcp_can_accept_new_subflow(owner)) {
+				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
+				goto dispose_child;
+			}
+
 			/* move the msk reference ownership to the subflow */
 			subflow_req->msk = NULL;
 			ctx->conn = (struct sock *)owner;

-- 
2.48.1

[PATCH mptcp-next 2/5] selftests: mptcp: validate MPJoin HMacFailure counters

[Matthieu Baerts (NGI0)]

The parent commit fixes an issue around these counters where one of them
-- MPJoinAckHMacFailure -- was wrongly incremented in some cases.

This makes sure the counter is always 0. It should be incremented only
in case of corruption, or a wrong implementation, which should not be
the case in these selftests.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Note: this can go to mptcp-net as well, with the parent patch.
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 13a3b68181ee14eb628a858e5738094c3c936b74..5060b7e24f94550246c2b1f0465dcaf42b869313 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1441,6 +1441,15 @@ chk_join_nr()
 		fi
 	fi
 
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynAckHMacFailure")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "0" ]; then
+		rc=${KSFT_FAIL}
+		print_check "ack HMAC"
+		fail_test "got $count JOIN[s] synack HMAC failure expected 0"
+	fi
+
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
 	if [ -z "$count" ]; then
 		rc=${KSFT_SKIP}
@@ -1450,6 +1459,15 @@ chk_join_nr()
 		fail_test "got $count JOIN[s] ack rx expected $ack_nr"
 	fi
 
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckHMacFailure")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "0" ]; then
+		rc=${KSFT_FAIL}
+		print_check "synack HMAC"
+		fail_test "got $count JOIN[s] ack HMAC failure expected 0"
+	fi
+
 	print_results "join Rx" ${rc}
 
 	join_syn_tx="${join_syn_tx:-${syn_nr}}" \

-- 
2.48.1

[PATCH mptcp-next 3/5] mptcp: pass right struct to subflow_hmac_valid

[Matthieu Baerts (NGI0)]

subflow_hmac_valid() needs to access the MPTCP socket and the subflow
request, but not the request sock that is passed in argument.

Instead, the subflow request can be directly passed to avoid getting it
via an additional cast.

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

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 24c2de1891bdf31dfe04ef2077113563aad0e666..e7951786a97c91190c7341d2c586a1f4acc05ed5 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -745,15 +745,11 @@ struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *op
 EXPORT_SYMBOL(mptcp_subflow_reqsk_alloc);
 
 /* validate hmac received in third ACK */
-static bool subflow_hmac_valid(const struct request_sock *req,
+static bool subflow_hmac_valid(const struct mptcp_subflow_request_sock *subflow_req,
 			       const struct mptcp_options_received *mp_opt)
 {
-	const struct mptcp_subflow_request_sock *subflow_req;
+	struct mptcp_sock *msk = subflow_req->msk;
 	u8 hmac[SHA256_DIGEST_SIZE];
-	struct mptcp_sock *msk;
-
-	subflow_req = mptcp_subflow_rsk(req);
-	msk = subflow_req->msk;
 
 	subflow_generate_hmac(READ_ONCE(msk->remote_key),
 			      READ_ONCE(msk->local_key),
@@ -899,7 +895,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 				goto dispose_child;
 			}
 
-			if (!subflow_hmac_valid(req, &mp_opt)) {
+			if (!subflow_hmac_valid(subflow_req, &mp_opt)) {
 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
 				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				goto dispose_child;

-- 
2.48.1

[PATCH mptcp-next 4/5] mptcp: add MPJoinRejected MIB counter

[Matthieu Baerts (NGI0)]

This counter is useful to understand why some paths are rejected, and
not created as expected.

It is incremented when receiving a connection request, if the PM didn't
allow the creation of new subflows.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/mib.c      | 1 +
 net/mptcp/mib.h      | 1 +
 net/mptcp/protocol.c | 4 +++-
 net/mptcp/subflow.c  | 2 ++
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 19eb9292bd6093a760b41f98c1774fd2490c48e3..0c24545f0e8df95b3475bfccc7a2f2ce440f7ad2 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -28,6 +28,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure", MPTCP_MIB_JOINSYNACKMAC),
 	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
 	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
+	SNMP_MIB_ITEM("MPJoinRejected", MPTCP_MIB_JOINREJECTED),
 	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 128282982843a07614a46f9b2c2f7c708306c769..250c6b77977e8f846b5741304f7841a922f51967 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -23,6 +23,7 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK + MP_JOIN */
+	MPTCP_MIB_JOINREJECTED,		/* The PM rejected the JOIN request */
 	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/protocol.c b/net/mptcp/protocol.c
index 9042a86e003036f8eb9680bc6c520fb57d92dbd0..8ba1a4f225bd2014ebdc973e752702028ee5f31a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3527,8 +3527,10 @@ bool mptcp_finish_join(struct sock *ssk)
 		return true;
 	}
 
-	if (!mptcp_pm_allow_new_subflow(msk))
+	if (!mptcp_pm_allow_new_subflow(msk)) {
+		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_JOINREJECTED);
 		goto err_prohibited;
+	}
 
 	/* If we can't acquire msk socket lock here, let the release callback
 	 * handle it
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index e7951786a97c91190c7341d2c586a1f4acc05ed5..15613d691bfef6800268ae75b62508736865f44a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -247,6 +247,7 @@ static int subflow_check_req(struct request_sock *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;
 			}
@@ -902,6 +903,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			}
 
 			if (!mptcp_can_accept_new_subflow(owner)) {
+				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINREJECTED);
 				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				goto dispose_child;
 			}

-- 
2.48.1

[PATCH mptcp-next 5/5] selftests: mptcp: validate MPJoinRejected counter

[Matthieu Baerts (NGI0)]

The parent commit adds this new counter, incremented when receiving a
connection request, if the PM didn't allow the creation of new subflows.

Most of the time, it is then kept at 0, except when the PM limits cause
the receiver side to reject new MPJoin connections. This is the case in
the following tests:

 - single subflow, limited by server
 - multiple subflows, limited by server
 - subflows limited by server w cookies
 - userspace pm type rejects join
 - userspace pm type prevents mp_prio

Simply set join_syn_rej=1 when checking the MPJoin counters for these
tests.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 26 ++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 5060b7e24f94550246c2b1f0465dcaf42b869313..7f868d68ab2867241aaad1c8739a662f232e625f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -62,6 +62,7 @@ unset sflags
 unset fastclose
 unset fullmesh
 unset speed
+unset join_syn_rej
 unset join_csum_ns1
 unset join_csum_ns2
 unset join_fail_nr
@@ -1403,6 +1404,7 @@ chk_join_nr()
 	local syn_nr=$1
 	local syn_ack_nr=$2
 	local ack_nr=$3
+	local syn_rej=${join_syn_rej:-0}
 	local csum_ns1=${join_csum_ns1:-0}
 	local csum_ns2=${join_csum_ns2:-0}
 	local fail_nr=${join_fail_nr:-0}
@@ -1468,6 +1470,15 @@ chk_join_nr()
 		fail_test "got $count JOIN[s] ack HMAC failure expected 0"
 	fi
 
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinRejected")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "$syn_rej" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn rejected"
+		fail_test "got $count JOIN[s] syn rejected expected $syn_rej"
+	fi
+
 	print_results "join Rx" ${rc}
 
 	join_syn_tx="${join_syn_tx:-${syn_nr}}" \
@@ -1963,7 +1974,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
-		chk_join_nr 1 1 0
+		join_syn_rej=1 \
+			chk_join_nr 1 1 0
 	fi
 
 	# subflow
@@ -1992,7 +2004,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
-		chk_join_nr 2 2 1
+		join_syn_rej=1 \
+			chk_join_nr 2 2 1
 	fi
 
 	# single subflow, dev
@@ -3061,7 +3074,8 @@ 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
-		chk_join_nr 2 1 1
+		join_syn_rej=1 \
+			chk_join_nr 2 1 1
 	fi
 
 	# test signal address with cookies
@@ -3545,7 +3559,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
-		chk_join_nr 1 1 0
+		join_syn_rej=1 \
+			chk_join_nr 1 1 0
 	fi
 
 	# userspace pm type does not send join
@@ -3568,7 +3583,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
-		chk_join_nr 1 1 0
+		join_syn_rej=1 \
+			chk_join_nr 1 1 0
 		chk_prio_nr 0 0 0 0
 	fi
 

-- 
2.48.1

Re: [PATCH mptcp-next 0/5] mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected

[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: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14147512989

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


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)

Re: [PATCH mptcp-next 2/5] selftests: mptcp: validate MPJoin HMacFailure counters

[Geliang Tang]

Hi Matt,

Thanks for this patch.

On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
> The parent commit fixes an issue around these counters where one of
> them
> -- MPJoinAckHMacFailure -- was wrongly incremented in some cases.
> 
> This makes sure the counter is always 0. It should be incremented
> only
> in case of corruption, or a wrong implementation, which should not be
> the case in these selftests.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Note: this can go to mptcp-net as well, with the parent patch.
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 18
> ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index
> 13a3b68181ee14eb628a858e5738094c3c936b74..5060b7e24f94550246c2b1f0465
> dcaf42b869313 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1441,6 +1441,15 @@ chk_join_nr()
>  		fi
>  	fi
>  
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynAckHMacFailure")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "0" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "ack HMAC"

Should here be "synack HMAC"...

> +		fail_test "got $count JOIN[s] synack HMAC failure
> expected 0"
> +	fi
> +
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
>  	if [ -z "$count" ]; then
>  		rc=${KSFT_SKIP}
> @@ -1450,6 +1459,15 @@ chk_join_nr()
>  		fail_test "got $count JOIN[s] ack rx expected
> $ack_nr"
>  	fi
>  
> +	count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMPJoinAckHMacFailure")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "0" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "synack HMAC"

... and here be "ack HMAC"?

Thanks,
-Geliang

> +		fail_test "got $count JOIN[s] ack HMAC failure
> expected 0"
> +	fi
> +
>  	print_results "join Rx" ${rc}
>  
>  	join_syn_tx="${join_syn_tx:-${syn_nr}}" \
> 

Re: [PATCH mptcp-next 1/5] mptcp: only inc MPJoinAckHMacFailure for HMAC failures

[Geliang Tang]

Hi Matt,

On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
> Recently, during a debugging session using local MPTCP connections, I
> noticed MPJoinAckHMacFailure was not zero on the server side. The
> counter was in fact incremented when the PM rejected new subflows,
> because the 'subflow' limit was reached.
> 
> The fix is easy, simply dissociating the two cases: only the HMAC
> validation check should increase MPTCP_MIB_JOINACKMAC counter.
> 
> Fixes: 4cf8b7e48a09 ("subflow: introduce and use
> mptcp_can_accept_new_subflow()")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Note: this is a fix for mptcp-net
> ---
>  net/mptcp/subflow.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index
> 409bd415ef1d190d5599658d01323ad8c8a9be93..24c2de1891bdf31dfe04ef20771
> 13563aad0e666 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -899,13 +899,17 @@ static struct sock *subflow_syn_recv_sock(const
> struct sock *sk,
>  				goto dispose_child;
>  			}
>  
> -			if (!subflow_hmac_valid(req, &mp_opt) ||
> -			   
> !mptcp_can_accept_new_subflow(subflow_req->msk)) {
> +			if (!subflow_hmac_valid(req, &mp_opt)) {
>  				SUBFLOW_REQ_INC_STATS(req,
> MPTCP_MIB_JOINACKMAC);
>  				subflow_add_reset_reason(skb,
> MPTCP_RST_EPROHIBIT);
>  				goto dispose_child;
>  			}
>  
> +			if (!mptcp_can_accept_new_subflow(owner)) {
> +				subflow_add_reset_reason(skb,
> MPTCP_RST_EPROHIBIT);
> +				goto dispose_child;
> +			}

This can be merged with the previous check

	if (!owner) {
		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
		goto dispose_child;
	}

as:

	if (!owner || !mptcp_can_accept_new_subflow(owner)) {
		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
		goto dispose_child;
	}

WDYT?

Thanks,
-Geliang

> +
>  			/* move the msk reference ownership to the
> subflow */
>  			subflow_req->msk = NULL;
>  			ctx->conn = (struct sock *)owner;
> 

Re: [PATCH mptcp-next 1/5] mptcp: only inc MPJoinAckHMacFailure for HMAC failures

[Geliang Tang]

On Mon, 2025-03-31 at 09:28 +0800, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
> > Recently, during a debugging session using local MPTCP connections,
> > I
> > noticed MPJoinAckHMacFailure was not zero on the server side. The
> > counter was in fact incremented when the PM rejected new subflows,
> > because the 'subflow' limit was reached.
> > 
> > The fix is easy, simply dissociating the two cases: only the HMAC
> > validation check should increase MPTCP_MIB_JOINACKMAC counter.
> > 
> > Fixes: 4cf8b7e48a09 ("subflow: introduce and use
> > mptcp_can_accept_new_subflow()")
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > ---
> > Note: this is a fix for mptcp-net
> > ---
> >  net/mptcp/subflow.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index
> > 409bd415ef1d190d5599658d01323ad8c8a9be93..24c2de1891bdf31dfe04ef207
> > 71
> > 13563aad0e666 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -899,13 +899,17 @@ static struct sock
> > *subflow_syn_recv_sock(const
> > struct sock *sk,
> >  				goto dispose_child;
> >  			}
> >  
> > -			if (!subflow_hmac_valid(req, &mp_opt) ||
> > -			   
> > !mptcp_can_accept_new_subflow(subflow_req->msk)) {
> > +			if (!subflow_hmac_valid(req, &mp_opt)) {
> >  				SUBFLOW_REQ_INC_STATS(req,
> > MPTCP_MIB_JOINACKMAC);
> >  				subflow_add_reset_reason(skb,
> > MPTCP_RST_EPROHIBIT);
> >  				goto dispose_child;
> >  			}
> >  
> > +			if (!mptcp_can_accept_new_subflow(owner))
> > {
> > +				subflow_add_reset_reason(skb,
> > MPTCP_RST_EPROHIBIT);
> > +				goto dispose_child;
> > +			}
> 
> This can be merged with the previous check
> 
> 	if (!owner) {
> 		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
> 		goto dispose_child;
> 	}
> 
> as:
> 
> 	if (!owner || !mptcp_can_accept_new_subflow(owner)) {
> 		subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
> 		goto dispose_child;
> 	}
> 
> WDYT?

I saw the changes in patch 4, the two checks cannot be merged then.

so this patch LGTM!

Thanks,
-Geliang

> 
> Thanks,
> -Geliang
> 
> > +
> >  			/* move the msk reference ownership to the
> > subflow */
> >  			subflow_req->msk = NULL;
> >  			ctx->conn = (struct sock *)owner;
> > 
> 
> 

Re: [PATCH mptcp-next 4/5] mptcp: add MPJoinRejected MIB counter

[Geliang Tang]

Hi Matt,

On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
> This counter is useful to understand why some paths are rejected, and
> not created as expected.
> 
> It is incremented when receiving a connection request, if the PM
> didn't
> allow the creation of new subflows.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/mib.c      | 1 +
>  net/mptcp/mib.h      | 1 +
>  net/mptcp/protocol.c | 4 +++-
>  net/mptcp/subflow.c  | 2 ++
>  4 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index
> 19eb9292bd6093a760b41f98c1774fd2490c48e3..0c24545f0e8df95b3475bfccc7a
> 2f2ce440f7ad2 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -28,6 +28,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure",
> MPTCP_MIB_JOINSYNACKMAC),
>  	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
>  	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
> +	SNMP_MIB_ITEM("MPJoinRejected", MPTCP_MIB_JOINREJECTED),
>  	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
> 128282982843a07614a46f9b2c2f7c708306c769..250c6b77977e8f846b5741304f7
> 841a922f51967 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -23,6 +23,7 @@ enum linux_mptcp_mib_field {
>  	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK
> + MP_JOIN */
>  	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN
> */
>  	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK +
> MP_JOIN */
> +	MPTCP_MIB_JOINREJECTED,		/* The PM rejected
> the JOIN request */
>  	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/protocol.c b/net/mptcp/protocol.c
> index
> 9042a86e003036f8eb9680bc6c520fb57d92dbd0..8ba1a4f225bd2014ebdc973e752
> 702028ee5f31a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3527,8 +3527,10 @@ bool mptcp_finish_join(struct sock *ssk)
>  		return true;
>  	}
>  
> -	if (!mptcp_pm_allow_new_subflow(msk))
> +	if (!mptcp_pm_allow_new_subflow(msk)) {
> +		MPTCP_INC_STATS(sock_net(ssk),
> MPTCP_MIB_JOINREJECTED);

nit:

Although the result is the same as sock_net(ssk), what do you think of
using sock_net(parent) here?

Thanks,
-Geliang

>  		goto err_prohibited;
> +	}
>  
>  	/* If we can't acquire msk socket lock here, let the release
> callback
>  	 * handle it
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index
> e7951786a97c91190c7341d2c586a1f4acc05ed5..15613d691bfef6800268ae75b62
> 508736865f44a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -247,6 +247,7 @@ static int subflow_check_req(struct request_sock
> *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;
>  			}
> @@ -902,6 +903,7 @@ static struct sock *subflow_syn_recv_sock(const
> struct sock *sk,
>  			}
>  
>  			if (!mptcp_can_accept_new_subflow(owner)) {
> +				SUBFLOW_REQ_INC_STATS(req,
> MPTCP_MIB_JOINREJECTED);
>  				subflow_add_reset_reason(skb,
> MPTCP_RST_EPROHIBIT);
>  				goto dispose_child;
>  			}
> 

Re: [PATCH mptcp-next 0/5] mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected

[Geliang Tang]

Hi Matt,

Thanks for this set.

On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
> Recently, during a debugging session using local MPTCP connections, I
> noticed MPJoinAckHMacFailure was strangely not zero on the server
> side.
> 
> The first patch fixes this issue, and the second one validates it.
> These
> two patches can be applied in mptcp-net.
> 
> There is a small refactoring in the 3rd patch.
> 
> Patch 4 adds MPJoinRejected to track when the PM rejects patches, and
> patch 5 validates it.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Except for my two small suggestions in patch 2 and patch 4, the rest of
the code LGTM! No need to send a v2, please update them when merging
this set (if you agree with these two small changes).

    Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> ---
> Matthieu Baerts (NGI0) (5):
>       mptcp: only inc MPJoinAckHMacFailure for HMAC failures
>       selftests: mptcp: validate MPJoin HMacFailure counters
>       mptcp: pass right struct to subflow_hmac_valid
>       mptcp: add MPJoinRejected MIB counter
>       selftests: mptcp: validate MPJoinRejected counter
> 
>  net/mptcp/mib.c                                 |  1 +
>  net/mptcp/mib.h                                 |  1 +
>  net/mptcp/protocol.c                            |  4 ++-
>  net/mptcp/subflow.c                             | 18 +++++-----
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 44
> ++++++++++++++++++++++---
>  5 files changed, 54 insertions(+), 14 deletions(-)
> ---
> base-commit: aca18798220853f0d7a8d01c28213e0d3fccfbfc
> change-id: 20250328-mptcp-mpj-reject-addd081553f5
> 
> Best regards,

Re: [PATCH mptcp-next 2/5] selftests: mptcp: validate MPJoin HMacFailure counters

[Matthieu Baerts]

Hi Geliang,

On 30/03/2025 01:15, Geliang Tang wrote:
> 
> Hi Matt,
> 
> Thanks for this patch.
> 
> On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
>> The parent commit fixes an issue around these counters where one of
>> them
>> -- MPJoinAckHMacFailure -- was wrongly incremented in some cases.
>>
>> This makes sure the counter is always 0. It should be incremented
>> only
>> in case of corruption, or a wrong implementation, which should not be
>> the case in these selftests.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Note: this can go to mptcp-net as well, with the parent patch.
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 18
>> ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index
>> 13a3b68181ee14eb628a858e5738094c3c936b74..5060b7e24f94550246c2b1f0465
>> dcaf42b869313 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1441,6 +1441,15 @@ chk_join_nr()
>>  		fi
>>  	fi
>>  
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynAckHMacFailure")
>> +	if [ -z "$count" ]; then
>> +		rc=${KSFT_SKIP}
>> +	elif [ "$count" != "0" ]; then
>> +		rc=${KSFT_FAIL}
>> +		print_check "ack HMAC"
> 
> Should here be "synack HMAC"...
> 
>> +		fail_test "got $count JOIN[s] synack HMAC failure
>> expected 0"
>> +	fi
>> +
>>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
>>  	if [ -z "$count" ]; then
>>  		rc=${KSFT_SKIP}
>> @@ -1450,6 +1459,15 @@ chk_join_nr()
>>  		fail_test "got $count JOIN[s] ack rx expected
>> $ack_nr"
>>  	fi
>>  
>> +	count=$(mptcp_lib_get_counter ${ns1}
>> "MPTcpExtMPJoinAckHMacFailure")
>> +	if [ -z "$count" ]; then
>> +		rc=${KSFT_SKIP}
>> +	elif [ "$count" != "0" ]; then
>> +		rc=${KSFT_FAIL}
>> +		print_check "synack HMAC"
> 
> ... and here be "ack HMAC"?

Good catch! I can fix that when applying the patches.

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

Re: [PATCH mptcp-next 4/5] mptcp: add MPJoinRejected MIB counter

[Matthieu Baerts]

Hi Geliang,

On 31/03/2025 03:59, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
>> This counter is useful to understand why some paths are rejected, and
>> not created as expected.
>>
>> It is incremented when receiving a connection request, if the PM
>> didn't
>> allow the creation of new subflows.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/mib.c      | 1 +
>>  net/mptcp/mib.h      | 1 +
>>  net/mptcp/protocol.c | 4 +++-
>>  net/mptcp/subflow.c  | 2 ++
>>  4 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
>> index
>> 19eb9292bd6093a760b41f98c1774fd2490c48e3..0c24545f0e8df95b3475bfccc7a
>> 2f2ce440f7ad2 100644
>> --- a/net/mptcp/mib.c
>> +++ b/net/mptcp/mib.c
>> @@ -28,6 +28,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>>  	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure",
>> MPTCP_MIB_JOINSYNACKMAC),
>>  	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
>>  	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
>> +	SNMP_MIB_ITEM("MPJoinRejected", MPTCP_MIB_JOINREJECTED),
>>  	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
>> 128282982843a07614a46f9b2c2f7c708306c769..250c6b77977e8f846b5741304f7
>> 841a922f51967 100644
>> --- a/net/mptcp/mib.h
>> +++ b/net/mptcp/mib.h
>> @@ -23,6 +23,7 @@ enum linux_mptcp_mib_field {
>>  	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK
>> + MP_JOIN */
>>  	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN
>> */
>>  	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK +
>> MP_JOIN */
>> +	MPTCP_MIB_JOINREJECTED,		/* The PM rejected
>> the JOIN request */
>>  	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/protocol.c b/net/mptcp/protocol.c
>> index
>> 9042a86e003036f8eb9680bc6c520fb57d92dbd0..8ba1a4f225bd2014ebdc973e752
>> 702028ee5f31a 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -3527,8 +3527,10 @@ bool mptcp_finish_join(struct sock *ssk)
>>  		return true;
>>  	}
>>  
>> -	if (!mptcp_pm_allow_new_subflow(msk))
>> +	if (!mptcp_pm_allow_new_subflow(msk)) {
>> +		MPTCP_INC_STATS(sock_net(ssk),
>> MPTCP_MIB_JOINREJECTED);
> 
> nit:
> 
> Although the result is the same as sock_net(ssk), what do you think of
> using sock_net(parent) here?

We could, but why would you prefer using 'parent' instead of 'ssk'? Ii
picked 'ssk' because that's the one passed in argument of this function.
'parent' is obtained from 'ssk'.

I hope you don't mind if I apply the patch as it is because I already
applied the parent patches. I can always change that later of course.

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

Re: [PATCH mptcp-next 0/5] mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected

[Matthieu Baerts]

Hi Geliang,

On 31/03/2025 04:05, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this set.
> 
> On Sat, 2025-03-29 at 17:26 +0100, Matthieu Baerts (NGI0) wrote:
>> Recently, during a debugging session using local MPTCP connections, I
>> noticed MPJoinAckHMacFailure was strangely not zero on the server
>> side.
>>
>> The first patch fixes this issue, and the second one validates it.
>> These
>> two patches can be applied in mptcp-net.
>>
>> There is a small refactoring in the 3rd patch.
>>
>> Patch 4 adds MPJoinRejected to track when the PM rejects patches, and
>> patch 5 validates it.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Except for my two small suggestions in patch 2 and patch 4, the rest of
> the code LGTM! No need to send a v2, please update them when merging
> this set (if you agree with these two small changes).
> 
>     Reviewed-by: Geliang Tang <geliang@kernel.org>

Thank you for the review!

Now in our tree (with the suggested modification in patch 2):

New patches for t/upstream-net and t/upstream:
- 0331c038b8f1: mptcp: only inc MPJoinAckHMacFailure for HMAC failures
- f80f8ee1b1b7: selftests: mptcp: validate MPJoin HMacFailure counters
- Results: 3804298131e0..c8227c8f8681 (export-net)
- Results: 6ac9a8d5227d..21ef34d86439 (export)

New patches for t/upstream:
- 3f8822824116: mptcp: pass right struct to subflow_hmac_valid
- 7b0845a43565: mptcp: add MPJoinRejected MIB counter
- 49e3c7e5ef6b: selftests: mptcp: validate MPJoinRejected counter
- Results: 21ef34d86439..b291aa1915c1 (export)


Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/cc0ff77a44448f3720df63d496387419c551f415/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/5110f50c1efb4273b3a28b0ae530b282b05af9ba/checks


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