Re: [PATCH v2] mptcp: only set DATA_FIN when a mapping is present

[Matthieu Baerts <matttbe@kernel.org>]

Hi Michael, Gang,

On 09/07/2026 07:43, gang.yan@linux.dev wrote:
> July 8, 2026 at 1:17 AM, "Michael Bommarito" <michael.bommarito@gmail.com> wrote:

Thank you for this patch and the review!

>> mptcp_get_options() clears only the status group of struct
>> mptcp_options_received; data_seq, subflow_seq and data_len are filled in
>> by mptcp_parse_option() exclusively inside the DSS mapping block, which
>> runs only when the DSS M (mapping present) bit is set.
>>
>> A peer can send a DSS option with the DATA_FIN flag set but the mapping
>> bit clear. The parser then records mp_opt->data_fin while leaving
>> data_len and data_seq uninitialized. For a zero-length segment
>> mptcp_incoming_options() evaluates

By chance, do you have a reproducer? Maybe using Packetdrill with MPTCP
support? [1]
If not, how did you discover this issue?

[1] https://github.com/multipath-tcp/packetdrill/

>>  if (mp_opt.data_fin && mp_opt.data_len == 1 &&
>>  mptcp_update_rcv_data_fin(msk, mp_opt.data_seq, mp_opt.dsn64))
>>
>> which reads the uninitialized data_len and data_seq; KMSAN reports an
>> uninit-value in mptcp_incoming_options(). The stale data_seq can also be
>> fed into the receive-side DATA_FIN sequence tracking.
>>
>> Record the DATA_FIN flag only when the DSS option carries a mapping, so
>> data_fin is never set without data_seq and data_len also being present.
>> data_fin is part of the status group that mptcp_get_options() clears up
>> front, so on the no-map path it stays zero and the zero-length DATA_FIN
>> branch is simply skipped. A DATA_FIN is always transmitted together with
>> a mapping (mptcp_write_data_fin() sets use_map along with data_seq and
>> data_len), so legitimate DATA_FIN handling is unaffected.
>>
>> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
>> ---
>> v2: adopt Paolo Abeni's suggested approach - do not set mp_opt->data_fin
>>  at all unless a mapping is present, rather than gating the consumer in
>>  mptcp_incoming_options() (v1). data_fin then defaults to the value
>>  mptcp_get_options() already clears it to (0) on the no-map path, so
>>  the uninitialized data_len/data_seq are never read.
>>
>>  net/mptcp/options.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index dff3fd5d3b559..6d003b24b969f 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -157,7 +157,6 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  ptr++;
>>
>>  flags = (*ptr++) & MPTCP_DSS_FLAG_MASK;
>> - mp_opt->data_fin = (flags & MPTCP_DSS_DATA_FIN) != 0;
>>  mp_opt->dsn64 = (flags & MPTCP_DSS_DSN64) != 0;
>>  mp_opt->use_map = (flags & MPTCP_DSS_HAS_MAP) != 0;
>>  mp_opt->ack64 = (flags & MPTCP_DSS_ACK64) != 0;
>> @@ -178,6 +177,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  }
>>
>>  if (mp_opt->use_map) {
>> + mp_opt->data_fin = (flags & MPTCP_DSS_DATA_FIN) != 0;
>>  if (mp_opt->dsn64)
>>  expected_opsize += TCPOLEN_MPTCP_DSS_MAP64;
>>  else
> 
> One minor thing I noticed while reviewing: Sashiko previously pointed out
> in a comment that the 'pr_debug' logging 'mp_opt->data_fin' might be
> problematic because it's printed before data_fin is actually assigned
> (since data_fin is now set only inside the if (mp_opt->use_map) block). 
> 
> I think it would be cleaner to move the pr_debug call after the data_fin
> assignment, so the logged value is always meaningful and consistent with
> the actual parsed state, like:

(...)
> This change isn't critical to the fix itself, so feel free to let Paolo/Matt and
> other Maintainers decide if it should go in now, later, or not at all.

Probably best to fix that now, to avoid getting confused when looking at
debug info. Do you mind doing that in a v3, please?

pw-bot: changes-requested

> A couple of additional notes for future submissions (just friendly reminders):
> 
> - The subject-prefix convention for MPTCP patches is typically [PATCH mptcp-net]
>   for bug fixes targeting the net tree, or [PATCH mptcp-next] for new features.
>   Since this is a fix, something like [PATCH mptcp-net v2] would be more appropriate.

For a "one-off" or urgent fix for MPTCP, that's OK to send patches to
the netdev ML directly. In this case, please use the "net" prefix, see:

  https://docs.kernel.org/process/maintainer-netdev.html

For other kind of patches specific to MPTCP, please send them only to
the MPTCP ML with the 'mptcp-net' or 'mptcp-next' prefixes. Just to be
clear: here you can send the v3 to the same people and lists -- you
missed Simon apparently, did you not use get_maintainer.pl script? --
with the 'net' prefix ([PATCH net v3]).

> - It's also helpful to include a Link: to the previous version (v1) in the changelog,
>   so reviewers can easily track the discussion history.

If I may add, please also send new version in a separate thread, not as
a reply, not to confuse various tools, as mentioned in the same doc.

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