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@xxxxxxx> 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@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.
>> 
>> 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