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

[gang . yan]

July 8, 2026 at 1:17 AM, "Michael Bommarito" <michael.bommarito@xxxxxxxxx mailto:michael.bommarito@xxxxxxxxx?to=%22Michael%20Bommarito%22%20%3Cmichael.bommarito%40gmail.com%3E > wrote:

Hi Michael

Thanks for the patch.


> 
> 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
> 
>  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@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
> ---
> 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:

'''
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index b924209a9b74..f58eb24f4d35 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -162,11 +162,6 @@ static void mptcp_parse_option(const struct sk_buff *skb,
                mp_opt->ack64 = (flags & MPTCP_DSS_ACK64) != 0;
                mp_opt->use_ack = (flags & MPTCP_DSS_HAS_ACK);
 
-               pr_debug("data_fin=%d dsn64=%d use_map=%d ack64=%d use_ack=%d\n",
-                        mp_opt->data_fin, mp_opt->dsn64,
-                        mp_opt->use_map, mp_opt->ack64,
-                        mp_opt->use_ack);
-
                expected_opsize = TCPOLEN_MPTCP_DSS_BASE;
 
                if (mp_opt->use_ack) {
@@ -184,6 +179,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
                                expected_opsize += TCPOLEN_MPTCP_DSS_MAP32;
                }
 
+               pr_debug("data_fin=%d dsn64=%d use_map=%d ack64=%d use_ack=%d\n",
+                        mp_opt->data_fin, mp_opt->dsn64,
+                        mp_opt->use_map, mp_opt->ack64,
+                        mp_opt->use_ack);
+
                /* Always parse any csum presence combination, we will enforce
                 * RFC 8684 Section 3.3.0 checks later in subflow_data_ready
                 */
'''

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.

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.

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

Looking forward to seeing more of your work on the MPTCP mailing list!

Thanks,
Gang
> --
> 2.53.0
>