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

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

[MoYuanhao]

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.

Signed-off-by: MoYuanhao <moyuanhao3676@163.com>
---
 net/ipv4/tcp_output.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5485a70b5fe5..0e5b9a654254 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -883,8 +883,10 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 		unsigned int size;
 
 		if (mptcp_syn_options(sk, skb, &size, &opts->mptcp)) {
-			opts->options |= OPTION_MPTCP;
-			remaining -= size;
+			if (remaining >= size) {
+				opts->options |= OPTION_MPTCP;
+				remaining -= size;
+			}
 		}
 	}
 
-- 
2.25.1

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

[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@vger.kernel.org


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
-- 
Sponsored by the NGI0 Core fund.

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

[Mo Yuanhao]

[-- Attachment #1: Type: text/plain, Size: 2961 bytes --]

在 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@vger.kernel.org
> 
> 
> 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@vger.kernel.org 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,

MoYuanhao

[-- Attachment #2: 0001-tcp-Check-space-before-adding-MPTCP-options.patch --]
[-- Type: text/x-patch, Size: 1854 bytes --]

From 12904db548bdd8011895fe071d7a420ccc6584f8 Mon Sep 17 00:00:00 2001
From: MoYuanhao <moyuanhao3676@163.com>
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@163.com>
---
 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

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

[Eric Dumazet]

On Thu, Dec 5, 2024 at 8:31 AM Mo Yuanhao <moyuanhao3676@163.com> wrote:
>
> 在 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@vger.kernel.org
> >
> >
> > 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@vger.kernel.org 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.

We usually do not refactor for a patch targeting a net tree.

Also, please do not add attachments, we need the patch inline as you did in v1.

As you can see, v2 is not avail in
https://patchwork.kernel.org/project/netdevbpf/list/

Documentation/process/submitting-patches.rst

No MIME, no links, no compression, no attachments.  Just plain text
-------------------------------------------------------------------

Linus and other kernel developers need to be able to read and comment
on the changes you are submitting.  It is important for a kernel
developer to be able to "quote" your changes, using standard e-mail
tools, so that they may comment on specific portions of your code.

For this reason, all patches should be submitted by e-mail "inline". The
easiest way to do this is with ``git send-email``, which is strongly
recommended.  An interactive tutorial for ``git send-email`` is available at
https://git-send-email.io.

If you choose not to use ``git send-email``:

.. warning::

  Be wary of your editor's word-wrap corrupting your patch,
  if you choose to cut-n-paste your patch.

Do not attach the patch as a MIME attachment, compressed or not.
Many popular e-mail applications will not always transmit a MIME
attachment as plain text, making it impossible to comment on your
code.  A MIME attachment also takes Linus a bit more time to process,
decreasing the likelihood of your MIME-attached change being accepted.

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

[Matthieu Baerts]

Hi MoYuanhao,

On 05/12/2024 08:54, Eric Dumazet wrote:
> On Thu, Dec 5, 2024 at 8:31 AM Mo Yuanhao <moyuanhao3676@163.com> wrote:
>>
>> 在 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@vger.kernel.org
>>>
>>>
>>> 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@vger.kernel.org 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.
> 
> We usually do not refactor for a patch targeting a net tree.

Indeed, I agree with Eric. Even if the code looks good, more lines have
been modified, maybe more risks, but also harder to backport to stable.

Also, I already applied your previous patches with the modifications I
suggested -- the code is the same as in v1, only the commit message has
been modified -- in the MPTCP tree:

New patches for t/upstream-net and t/upstream:
- 06dcf123ebe0: tcp: check space before adding MPTCP SYN options
- Results: 4fdf39fbfdb4..05c0ade28862 (export-net)
- Results: 0de5663a2d56..993a44eee93f (export)

Tests have been running too:

https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12158569520

Is this patch not OK for you?

https://github.com/multipath-tcp/mptcp_net-next/commit/06dcf123ebe0

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

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

[MoYuanhao]

Hi Matt and Eric,

>Hi MoYuanhao,

>On 05/12/2024 08:54, Eric Dumazet wrote:
>> On Thu, Dec 5, 2024 at 8:31 AM Mo Yuanhao <moyuanhao3676@163.com> wrote:
>>>
>>> 在 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@vger.kernel.org
>>>>
>>>>
>>>> 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@vger.kernel.org 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.
>> 
>> We usually do not refactor for a patch targeting a net tree.
>
>Indeed, I agree with Eric. Even if the code looks good, more lines have
>been modified, maybe more risks, but also harder to backport to stable.

Thank you for your guidance. I agree with your points, and using the patch from version 1 seems safer.

Best regards,

MoYuanhao

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

[MoYuanhao]

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.

Signed-off-by: MoYuanhao <moyuanhao3676@163.com>
---
 net/ipv4/tcp_output.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5485a70b5fe5..0e5b9a654254 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -883,8 +883,10 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 		unsigned int size;
 
 		if (mptcp_syn_options(sk, skb, &size, &opts->mptcp)) {
-			opts->options |= OPTION_MPTCP;
-			remaining -= size;
+			if (remaining >= size) {
+				opts->options |= OPTION_MPTCP;
+				remaining -= size;
+			}
 		}
 	}
 
-- 
2.25.1

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

[Eric Dumazet]

On Wed, Dec 4, 2024 at 5:07 AM MoYuanhao <moyuanhao3676@163.com> 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.
>
> Signed-off-by: MoYuanhao <moyuanhao3676@163.com>
> ---

If this is happening, then this would be a bug, and a Fixes: tag would
be needed.

If this is not yet happening, but would happen in the future, this
patch would hide a bug.

You forgot to CC MPTCP maintainers and reviewers ?

Matthieu Baerts <matttbe@kernel.org> (maintainer:NETWORKING [MPTCP])
Mat Martineau <martineau@kernel.org> (maintainer:NETWORKING [MPTCP])
Geliang Tang <geliang@kernel.org> (reviewer:NETWORKING [MPTCP])

In my  opinion, we should pass @remaining to mptcp_syn_options()
arguments to avoid side effects.