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

[Matthieu Baerts <matttbe@kernel.org>]

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.