[PATCH net-next 0/2] mptcp: add reset reasons in skb in more cases

[PATCH net-next 0/2] mptcp: add reset reasons in skb in more cases

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

The first patch only removes the check while the second adds reasons into
some places.

Jason Xing (2):
  mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()
  mptcp: add reset reason options in some places

 include/net/mptcp.h |  5 +----
 net/mptcp/subflow.c | 21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.37.3

[PATCH net-next 1/2] mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Before this, what mptcp_reset_option() checks is totally the same as
mptcp_get_ext() does, so we could skip it.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/mptcp.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index fb996124b3d5..42d13ee26619 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -215,10 +215,7 @@ __be32 mptcp_get_reset_option(const struct sk_buff *skb);
 
 static inline __be32 mptcp_reset_option(const struct sk_buff *skb)
 {
-	if (skb_ext_exist(skb, SKB_EXT_MPTCP))
-		return mptcp_get_reset_option(skb);
-
-	return htonl(0u);
+	return mptcp_get_reset_option(skb);
 }
 #else
 
-- 
2.37.3

[PATCH net-next 2/2] mptcp: add reset reason options in some places

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

The reason codes are handled in two ways nowadays (quoting Mat Martineau):
1. Sending in the MPTCP option on RST packets when there is no subflow
context available (these use subflow_add_reset_reason() and directly call
a TCP-level send_reset function)
2. The "normal" way via subflow->reset_reason. This will propagate to both
the outgoing reset packet and to a local path manager process via netlink
in mptcp_event_sub_closed()

RFC 8684 defines the skb reset reason behaviour which is not required
even though in some places:

    A host sends a TCP RST in order to close a subflow or reject
    an attempt to open a subflow (MP_JOIN). In order to let the
    receiving host know why a subflow is being closed or rejected,
    the TCP RST packet MAY include the MP_TCPRST option (Figure 15).
    The host MAY use this information to decide, for example, whether
    it tries to re-establish the subflow immediately, later, or never.

Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support")
introduced this feature about three years ago, we can fully use it.
There remains some places where we could insert reason into skb as
we can see in this patch.

Many thanks to Mat for help:)

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

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1626dd20c68f..49f746d91884 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 		return dst;
 
 	dst_release(dst);
-	if (!req->syncookie)
+	if (!req->syncookie) {
+		struct mptcp_ext *mpext = mptcp_get_ext(skb);
+
+		if (mpext)
+			subflow_add_reset_reason(skb, mpext->reset_reason);
 		tcp_request_sock_ops.send_reset(sk, skb);
+	}
 	return NULL;
 }
 
@@ -368,8 +373,13 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 		return dst;
 
 	dst_release(dst);
-	if (!req->syncookie)
+	if (!req->syncookie) {
+		struct mptcp_ext *mpext = mptcp_get_ext(skb);
+
+		if (mpext)
+			subflow_add_reset_reason(skb, mpext->reset_reason);
 		tcp6_request_sock_ops.send_reset(sk, skb);
+	}
 	return NULL;
 }
 #endif
@@ -873,13 +883,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);
+					subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 					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(child);
+
+				subflow_add_reset_reason(skb, subflow->reset_reason);
 				goto dispose_child;
+			}
 
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
 			tcp_rsk(req)->drop_req = true;
-- 
2.37.3

Re: [PATCH net-next 1/2] mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()

[Paolo Abeni]

On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Before this, what mptcp_reset_option() checks is totally the same as
> mptcp_get_ext() does, so we could skip it.

Note that the somewhat duplicate test is (a possibly not great)
optimization to avoid jumping in the mptcp code (possible icache
misses) for plain TCP sockets.

I guess we want to maintain it.

Cheers,

Paolo

Re: [PATCH net-next 1/2] mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()

[Jason Xing]

Hello Paolo,

On Fri, Apr 5, 2024 at 3:47 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Before this, what mptcp_reset_option() checks is totally the same as
> > mptcp_get_ext() does, so we could skip it.
>
> Note that the somewhat duplicate test is (a possibly not great)
> optimization to avoid jumping in the mptcp code (possible icache
> misses) for plain TCP sockets.
>
> I guess we want to maintain it.

Okay, I just read code and found the duplication but may I ask why it
has something to do with icache misses?

Thanks,
Jason

>
> Cheers,
>
> Paolo
>

Re: [PATCH net-next 2/2] mptcp: add reset reason options in some places

[Paolo Abeni]

On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> The reason codes are handled in two ways nowadays (quoting Mat Martineau):
> 1. Sending in the MPTCP option on RST packets when there is no subflow
> context available (these use subflow_add_reset_reason() and directly call
> a TCP-level send_reset function)
> 2. The "normal" way via subflow->reset_reason. This will propagate to both
> the outgoing reset packet and to a local path manager process via netlink
> in mptcp_event_sub_closed()
> 
> RFC 8684 defines the skb reset reason behaviour which is not required
> even though in some places:
> 
>     A host sends a TCP RST in order to close a subflow or reject
>     an attempt to open a subflow (MP_JOIN). In order to let the
>     receiving host know why a subflow is being closed or rejected,
>     the TCP RST packet MAY include the MP_TCPRST option (Figure 15).
>     The host MAY use this information to decide, for example, whether
>     it tries to re-establish the subflow immediately, later, or never.
> 
> Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support")
> introduced this feature about three years ago, we can fully use it.
> There remains some places where we could insert reason into skb as
> we can see in this patch.
> 
> Many thanks to Mat for help:)
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/mptcp/subflow.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1626dd20c68f..49f746d91884 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
>  		return dst;
>  
>  	dst_release(dst);
> -	if (!req->syncookie)
> +	if (!req->syncookie) {
> +		struct mptcp_ext *mpext = mptcp_get_ext(skb);
> +
> +		if (mpext)
> +			subflow_add_reset_reason(skb, mpext->reset_reason);

uhm? subflow_add_reset_reason() will do:

	mptcp_ext_add(skb)->reset_reason = mpext->reset_reason

The above looks like a no-op. 

Possibly we should instead ensure that subflow_check_req() calls
subflow_add_reset_reason() with reasonable arguments on all the error
paths?!?

Something alike the (completely untested) following

Cheers,

Paolo
---
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6042a47da61b..298c6342a78c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req,
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
 	 * TCP option space.
 	 */
-	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
+	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) {
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
 		return -EINVAL;
+	}
 #endif
 
 	mptcp_get_options(skb, &mp_opt);
@@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req,
 				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
 			if (!mptcp_pm_sport_in_anno_list(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);
@@ -227,10 +230,12 @@ 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_init_req_cookie_join_save(subflow_req, skb);
-			else
+			if (!mptcp_can_accept_new_subflow(subflow_req->msk)) {
+				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				return -EPERM;
+			}
+
+			subflow_init_req_cookie_join_save(subflow_req, skb);
 		}
 
 		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,

Re: [PATCH net-next 1/2] mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()

[Paolo Abeni]

On Fri, 2024-04-05 at 15:58 +0800, Jason Xing wrote:
> Hello Paolo,
> 
> On Fri, Apr 5, 2024 at 3:47 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > > 
> > > Before this, what mptcp_reset_option() checks is totally the same as
> > > mptcp_get_ext() does, so we could skip it.
> > 
> > Note that the somewhat duplicate test is (a possibly not great)
> > optimization to avoid jumping in the mptcp code (possible icache
> > misses) for plain TCP sockets.
> > 
> > I guess we want to maintain it.
> 
> Okay, I just read code and found the duplication but may I ask why it
> has something to do with icache misses?

The first check/mptcp_get_ext() is in mptcp_reset_option() /
tcp_v4_send_reset(). For plain TCP socket it will fail and the
execution will continue inside the same compilation unit. The code
locality should avoid icaches misses around there.

Removing such check, even when processing plain TCP packets, the code
execution will have to call into mptcp_get_reset_option() in the mptcp
code, decreasing the code locality and increasing the chance of icache
misses.

I don't have actual profile data, so this is an early optimization (and
thus root of all evil), but sounds reasonable to me (yep, I'm biased!)

Cheers,

Paolo

Re: [PATCH net-next 2/2] mptcp: add reset reason options in some places

[Jason Xing]

On Fri, Apr 5, 2024 at 4:16 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The reason codes are handled in two ways nowadays (quoting Mat Martineau):
> > 1. Sending in the MPTCP option on RST packets when there is no subflow
> > context available (these use subflow_add_reset_reason() and directly call
> > a TCP-level send_reset function)
> > 2. The "normal" way via subflow->reset_reason. This will propagate to both
> > the outgoing reset packet and to a local path manager process via netlink
> > in mptcp_event_sub_closed()
> >
> > RFC 8684 defines the skb reset reason behaviour which is not required
> > even though in some places:
> >
> >     A host sends a TCP RST in order to close a subflow or reject
> >     an attempt to open a subflow (MP_JOIN). In order to let the
> >     receiving host know why a subflow is being closed or rejected,
> >     the TCP RST packet MAY include the MP_TCPRST option (Figure 15).
> >     The host MAY use this information to decide, for example, whether
> >     it tries to re-establish the subflow immediately, later, or never.
> >
> > Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support")
> > introduced this feature about three years ago, we can fully use it.
> > There remains some places where we could insert reason into skb as
> > we can see in this patch.
> >
> > Many thanks to Mat for help:)
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/mptcp/subflow.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 1626dd20c68f..49f746d91884 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
> >               return dst;
> >
> >       dst_release(dst);
> > -     if (!req->syncookie)
> > +     if (!req->syncookie) {
> > +             struct mptcp_ext *mpext = mptcp_get_ext(skb);
> > +
> > +             if (mpext)
> > +                     subflow_add_reset_reason(skb, mpext->reset_reason);
>
> uhm? subflow_add_reset_reason() will do:
>
>         mptcp_ext_add(skb)->reset_reason = mpext->reset_reason
>
> The above looks like a no-op.

Ah, my bad. Actually I didn't add the mpext test statement in my original patch.

Yes, you're right. The 'if (mpext)' is totally unnecessary.

>
> Possibly we should instead ensure that subflow_check_req() calls
> subflow_add_reset_reason() with reasonable arguments on all the error
> paths?!?

Absolutely yes, it would be great. The reason I didn't touch them is
I'm still studying how to specify the error kind.

>
> Something alike the (completely untested) following
>
> Cheers,
>
> Paolo
> ---
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6042a47da61b..298c6342a78c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req,
>         /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>          * TCP option space.
>          */
> -       if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
> +       if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) {
> +               subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
>                 return -EINVAL;
> +       }
>  #endif
>
>         mptcp_get_options(skb, &mp_opt);
> @@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req,
>                                  ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
>                         if (!mptcp_pm_sport_in_anno_list(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);
> @@ -227,10 +230,12 @@ 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_init_req_cookie_join_save(subflow_req, skb);
> -                       else
> +                       if (!mptcp_can_accept_new_subflow(subflow_req->msk)) {
> +                               subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
>                                 return -EPERM;
> +                       }
> +
> +                       subflow_init_req_cookie_join_save(subflow_req, skb);
>                 }
>
>                 pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
>

Great! You complete it! Thanks for your instructions.

I'll test it and update it soon.

Thanks,
Jason

Re: [PATCH net-next 1/2] mptcp: don't need to check SKB_EXT_MPTCP in mptcp_reset_option()

[Jason Xing]

Hello Paolo,

On Fri, Apr 5, 2024 at 4:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2024-04-05 at 15:58 +0800, Jason Xing wrote:
> > Hello Paolo,
> >
> > On Fri, Apr 5, 2024 at 3:47 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Before this, what mptcp_reset_option() checks is totally the same as
> > > > mptcp_get_ext() does, so we could skip it.
> > >
> > > Note that the somewhat duplicate test is (a possibly not great)
> > > optimization to avoid jumping in the mptcp code (possible icache
> > > misses) for plain TCP sockets.
> > >
> > > I guess we want to maintain it.
> >
> > Okay, I just read code and found the duplication but may I ask why it
> > has something to do with icache misses?
>
> The first check/mptcp_get_ext() is in mptcp_reset_option() /
> tcp_v4_send_reset(). For plain TCP socket it will fail and the
> execution will continue inside the same compilation unit. The code
> locality should avoid icaches misses around there.
>
> Removing such check, even when processing plain TCP packets, the code
> execution will have to call into mptcp_get_reset_option() in the mptcp
> code, decreasing the code locality and increasing the chance of icache
> misses.

Interesting. Thanks for the explanation:)

>
> I don't have actual profile data, so this is an early optimization (and
> thus root of all evil), but sounds reasonable to me (yep, I'm biased!)

I'll drop this patch.

Thanks,
Jason

>
> Cheers,
>
> Paolo
>