diff mbox series

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

Message ID 20250329-mptcp-mpj-reject-v1-1-2396d5666e8f@kernel.org (mailing list archive)
State Accepted
Commit 0331c038b8f13eb5ebb8208d3836f03df4353c3d
Delegated to: Matthieu Baerts
Headers show
Series mptcp: fix MPJoinAckHMacFailure, add MPJoinRejected | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 19 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Matthieu Baerts March 29, 2025, 4:26 p.m. UTC 
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(-)

Comments

Geliang Tang March 31, 2025, 1:28 a.m. UTC | #1 
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;
>
Geliang Tang March 31, 2025, 1:54 a.m. UTC | #2 
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;
> > 
> 
>
diff mbox series

Patch

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;