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

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.