Re: [PATCH net-next] tcp: Check space before adding MPTCP options

[Mo Yuanhao]

在 2024/12/4 19:01, Matthieu Baerts 写道:
> Hi MoYuanhao,
>
> +Cc MPTCP mailing list.
>
> (Please cc the MPTCP list next time)
>
> On 04/12/2024 09:58, MoYuanhao wrote:
> > Ensure enough space before adding MPTCP options in tcp_syn_options()
> > Added a check to verify sufficient remaining space
> > before inserting MPTCP options in SYN packets.
> > This prevents issues when space is insufficient.
>
> Thank you for this patch. I'm surprised we all missed this check, but
> yes it is missing.
>
> As mentioned by Eric in his previous email, please add a 'Fixes' tag.
> For bug-fixes, you should also Cc stable and target 'net', not 'net-next':
>
> Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing
> connections")
> Cc: stable@xxxxxxxxxxxxxxx
>
>
> Regarding the code, it looks OK to me, as we did exactly that with
> mptcp_synack_options(). In mptcp_established_options(), we pass
> 'remaining' because many MPTCP options can be set, but not here. So I
> guess that's fine to keep the code like that, especially for the 'net' tree.
>
>
> Also, and linked to Eric's email, did you have an issue with that, or is
> it to prevent issues in the future?
>
>
> One last thing, please don’t repost your patches within one 24h period, see:
>
>    https://docs.kernel.org/process/maintainer-netdev.html
>
>
> Because the code is OK to me, and the same patch has already been sent
> twice to the netdev ML within a few hours, I'm going to apply this patch
> in our MPTCP tree with the suggested modifications. Later on, we will
> send it for inclusion in the net tree.
>
> pw-bot: awaiting-upstream
>
> (Not sure this pw-bot instruction will work as no net/mptcp/* files have
> been modified)
>
> Cheers,
> Matt
Hi Matt,

Thank you for your feedback!

I have made the suggested updates to the patch (version 2):

I’ve added the Fixes tag and Cc'd the stable@xxxxxxxxxxxxxxx list.
The target branch has been adjusted to net as per your suggestion.
I will make sure to Cc the MPTCP list in future submissions.

Regarding your question, this patch was created to prevent potential 
issues related to insufficient space for MPTCP options in the future. I 
didn't encounter a specific issue, but it seemed like a necessary 
safeguard to ensure robustness when handling SYN packets with MPTCP options.

Additionally, I have made further optimizations to the patch, which are 
included in the attached version. I believe it would be more elegant to 
introduce a new function, mptcp_set_option(), similar to 
mptcp_set_option_cond(), to handle MPTCP options.

This is my first time replying to a message in a Linux mailing list, so 
if there are any formatting issues or mistakes, please point them out 
and I will make sure to correct them in future submissions.

Thanks again for your review and suggestions. Looking forward to seeing 
the patch applied to the MPTCP tree and later inclusion in the net tree.

Best regards,

MoYuanhaoFrom 12904db548bdd8011895fe071d7a420ccc6584f8 Mon Sep 17 00:00:00 2001
From: MoYuanhao <moyuanhao3676@xxxxxxx>
Date: Thu, 5 Dec 2024 10:18:15 +0800
Subject: [PATCH net v2] tcp: Check space before adding MPTCP options

Ensure enough space before adding MPTCP options in tcp_syn_options()
Added a check to verify sufficient remaining space
before inserting MPTCP options in SYN packets.
This prevents issues when space is insufficient.

Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
Signed-off-by: MoYuanhao <moyuanhao3676@xxxxxxx>
---
 net/ipv4/tcp_output.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5485a70b5fe5..401eb14c870d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -792,6 +792,21 @@ static void smc_set_option_cond(const struct tcp_sock *tp,
 #endif
 }
 
+static void mptcp_set_option(struct sock *sk, const struct sk_buff *skb,
+				struct tcp_out_options *opts, unsigned int *remaining)
+{
+	if (sk_is_mptcp(sk)) {
+		unsigned int size;
+
+		if (mptcp_syn_options(sk, skb, &size, &opts->mptcp)) {
+			if (*remaining >= size) {
+				opts->options |= OPTION_MPTCP;
+				*remaining -= size;
+			}
+		}
+	}
+}
+
 static void mptcp_set_option_cond(const struct request_sock *req,
 				  struct tcp_out_options *opts,
 				  unsigned int *remaining)
@@ -879,14 +894,7 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 
 	smc_set_option(tp, opts, &remaining);
 
-	if (sk_is_mptcp(sk)) {
-		unsigned int size;
-
-		if (mptcp_syn_options(sk, skb, &size, &opts->mptcp)) {
-			opts->options |= OPTION_MPTCP;
-			remaining -= size;
-		}
-	}
+	mptcp_set_option(sk,skb,opts, &remaining);
 
 	bpf_skops_hdr_opt_len(sk, skb, NULL, NULL, 0, opts, &remaining);
 
-- 
2.25.1