diff mbox series

[net-next,v2,5/6] mptcp: support rstreason for passive reset

Message ID 20240404072047.11490-6-kerneljasonxing@gmail.com (mailing list archive)
State Superseded
Headers show
Series Implement reset reason mechanism to detect | expand

Commit Message

Jason Xing April 4, 2024, 7:20 a.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

It relys on what reset options in MPTCP does as rfc8684 says. Reusing
this logic can save us much energy. This patch replaces all the prior
NOT_SPECIFIED reasons.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/mptcp/subflow.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

Mat Martineau April 4, 2024, 8:33 p.m. UTC | #1 
On Thu, 4 Apr 2024, Jason Xing wrote:

> From: Jason Xing <kernelxing@tencent.com>
>
> It relys on what reset options in MPTCP does as rfc8684 says. Reusing
> this logic can save us much energy. This patch replaces all the prior
> NOT_SPECIFIED reasons.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> net/mptcp/subflow.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index a68d5d0f3e2a..24668d3020aa 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -304,7 +304,10 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
>
> 	dst_release(dst);
> 	if (!req->syncookie)
> -		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +		/* According to RFC 8684, 3.2. Starting a New Subflow,
> +		 * we should use an "MPTCP specific error" reason code.
> +		 */
> +		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);

Hi Jason -

In this case, the MPTCP reset reason is set in subflow_check_req(). Looks 
like it uses EMPTCP but that isn't guaranteed to stay the same. I think it 
would be better to extract the reset reason from the skb extension or the 
subflow context "reset_reason" field.


> 	return NULL;
> }
>
> @@ -371,7 +374,10 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
>
> 	dst_release(dst);
> 	if (!req->syncookie)
> -		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +		/* According to RFC 8684, 3.2. Starting a New Subflow,
> +		 * we should use an "MPTCP specific error" reason code.
> +		 */
> +		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);

Same issue here.

> 	return NULL;
> }
> #endif
> @@ -778,6 +784,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 	bool fallback, fallback_is_fatal;
> 	struct mptcp_sock *owner;
> 	struct sock *child;
> +	int reason;
>
> 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
>
> @@ -833,7 +840,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 		 */
> 		if (!ctx || fallback) {
> 			if (fallback_is_fatal) {
> -				subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
> +				reason = MPTCP_RST_EMPTCP;
> +				subflow_add_reset_reason(skb, reason);
> 				goto dispose_child;
> 			}
> 			goto fallback;
> @@ -861,7 +869,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 		} else if (ctx->mp_join) {
> 			owner = subflow_req->msk;
> 			if (!owner) {
> -				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
> +				reason = MPTCP_RST_EPROHIBIT;
> +				subflow_add_reset_reason(skb, reason);
> 				goto dispose_child;
> 			}
>
> @@ -875,13 +884,18 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 					 ntohs(inet_sk((struct sock *)owner)->inet_sport));
> 				if (!mptcp_pm_sport_in_anno_list(owner, sk)) {
> 					SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTACKRX);
> +					reason = MPTCP_RST_EUNSPEC;

I think the MPTCP code here should have been using MPTCP_RST_EPROHIBIT.


- Mat

> 					goto dispose_child;
> 				}
> 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTACKRX);
> 			}
>
> -			if (!mptcp_finish_join(child))
> +			if (!mptcp_finish_join(child)) {
> +				struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +
> +				reason = subflow->reset_reason;
> 				goto dispose_child;
> +			}
>
> 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
> 			tcp_rsk(req)->drop_req = true;
> @@ -901,7 +915,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 	tcp_rsk(req)->drop_req = true;
> 	inet_csk_prepare_for_destroy_sock(child);
> 	tcp_done(child);
> -	req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +	req->rsk_ops->send_reset(sk, skb, convert_mptcp_reason(reason));
>
> 	/* The last child reference will be released by the caller */
> 	return child;
> -- 
> 2.37.3
>
>
>
Jason Xing April 5, 2024, 12:12 a.m. UTC | #2 
Hello Mat,

On Fri, Apr 5, 2024 at 4:33 AM Mat Martineau <martineau@kernel.org> wrote:
>
> On Thu, 4 Apr 2024, Jason Xing wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > It relys on what reset options in MPTCP does as rfc8684 says. Reusing
> > this logic can save us much energy. This patch replaces all the prior
> > NOT_SPECIFIED reasons.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > net/mptcp/subflow.c | 26 ++++++++++++++++++++------
> > 1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index a68d5d0f3e2a..24668d3020aa 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -304,7 +304,10 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
> >
> >       dst_release(dst);
> >       if (!req->syncookie)
> > -             tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +             /* According to RFC 8684, 3.2. Starting a New Subflow,
> > +              * we should use an "MPTCP specific error" reason code.
> > +              */
> > +             tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);
>
> Hi Jason -
>
> In this case, the MPTCP reset reason is set in subflow_check_req(). Looks
> like it uses EMPTCP but that isn't guaranteed to stay the same. I think it
> would be better to extract the reset reason from the skb extension or the
> subflow context "reset_reason" field.

Good suggestions :)

>
>
> >       return NULL;
> > }
> >
> > @@ -371,7 +374,10 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
> >
> >       dst_release(dst);
> >       if (!req->syncookie)
> > -             tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +             /* According to RFC 8684, 3.2. Starting a New Subflow,
> > +              * we should use an "MPTCP specific error" reason code.
> > +              */
> > +             tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);
>
> Same issue here.

Got it.

>
> >       return NULL;
> > }
> > #endif
> > @@ -778,6 +784,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >       bool fallback, fallback_is_fatal;
> >       struct mptcp_sock *owner;
> >       struct sock *child;
> > +     int reason;
> >
> >       pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> >
> > @@ -833,7 +840,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >                */
> >               if (!ctx || fallback) {
> >                       if (fallback_is_fatal) {
> > -                             subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
> > +                             reason = MPTCP_RST_EMPTCP;
> > +                             subflow_add_reset_reason(skb, reason);
> >                               goto dispose_child;
> >                       }
> >                       goto fallback;
> > @@ -861,7 +869,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >               } else if (ctx->mp_join) {
> >                       owner = subflow_req->msk;
> >                       if (!owner) {
> > -                             subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
> > +                             reason = MPTCP_RST_EPROHIBIT;
> > +                             subflow_add_reset_reason(skb, reason);
> >                               goto dispose_child;
> >                       }
> >
> > @@ -875,13 +884,18 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >                                        ntohs(inet_sk((struct sock *)owner)->inet_sport));
> >                               if (!mptcp_pm_sport_in_anno_list(owner, sk)) {
> >                                       SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTACKRX);
> > +                                     reason = MPTCP_RST_EUNSPEC;
>
> I think the MPTCP code here should have been using MPTCP_RST_EPROHIBIT.

I'll update in the V2 of the patch.

Thanks,
Jason
diff mbox series

Patch

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a68d5d0f3e2a..24668d3020aa 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -304,7 +304,10 @@  static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 
 	dst_release(dst);
 	if (!req->syncookie)
-		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+		/* According to RFC 8684, 3.2. Starting a New Subflow,
+		 * we should use an "MPTCP specific error" reason code.
+		 */
+		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);
 	return NULL;
 }
 
@@ -371,7 +374,10 @@  static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 
 	dst_release(dst);
 	if (!req->syncookie)
-		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+		/* According to RFC 8684, 3.2. Starting a New Subflow,
+		 * we should use an "MPTCP specific error" reason code.
+		 */
+		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_MPTCP_RST_EMPTCP);
 	return NULL;
 }
 #endif
@@ -778,6 +784,7 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	bool fallback, fallback_is_fatal;
 	struct mptcp_sock *owner;
 	struct sock *child;
+	int reason;
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
@@ -833,7 +840,8 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 */
 		if (!ctx || fallback) {
 			if (fallback_is_fatal) {
-				subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+				reason = MPTCP_RST_EMPTCP;
+				subflow_add_reset_reason(skb, reason);
 				goto dispose_child;
 			}
 			goto fallback;
@@ -861,7 +869,8 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		} else if (ctx->mp_join) {
 			owner = subflow_req->msk;
 			if (!owner) {
-				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
+				reason = MPTCP_RST_EPROHIBIT;
+				subflow_add_reset_reason(skb, reason);
 				goto dispose_child;
 			}
 
@@ -875,13 +884,18 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 					 ntohs(inet_sk((struct sock *)owner)->inet_sport));
 				if (!mptcp_pm_sport_in_anno_list(owner, sk)) {
 					SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTACKRX);
+					reason = MPTCP_RST_EUNSPEC;
 					goto dispose_child;
 				}
 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTACKRX);
 			}
 
-			if (!mptcp_finish_join(child))
+			if (!mptcp_finish_join(child)) {
+				struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+
+				reason = subflow->reset_reason;
 				goto dispose_child;
+			}
 
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
 			tcp_rsk(req)->drop_req = true;
@@ -901,7 +915,7 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	tcp_rsk(req)->drop_req = true;
 	inet_csk_prepare_for_destroy_sock(child);
 	tcp_done(child);
-	req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	req->rsk_ops->send_reset(sk, skb, convert_mptcp_reason(reason));
 
 	/* The last child reference will be released by the caller */
 	return child;