The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present
@ 2026-07-09 19:19 Michael Bommarito
  2026-07-10 16:19 ` Matthieu Baerts
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Bommarito @ 2026-07-09 19:19 UTC (permalink / raw)
  To: Matthieu Baerts, Mat Martineau
  Cc: Geliang Tang, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Gang Yan, mptcp, netdev, linux-kernel

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.

Move the pr_debug() that logs the parsed DSS flags below the mapping
block, so it reports the final data_fin value instead of the stale one
it would otherwise print before the assignment.

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>
---
v3:
 - Move the pr_debug() DSS flag log below the mapping block so it prints
   the final data_fin value (Gang Yan).  data_fin is now assigned only
   inside the use_map block, so logging it earlier would report a stale
   value.  No functional change to the fix.
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.

Link to v1: https://lore.kernel.org/all/20260617215725.1116295-1-michael.bommarito@gmail.com/
Link to v2: https://lore.kernel.org/all/20260707171730.2679013-1-michael.bommarito@gmail.com/

 net/mptcp/options.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index dff3fd5d3b559..1b74ca5b6a595 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -157,17 +157,11 @@ 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;
 		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) {
@@ -178,12 +172,18 @@ 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
 				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
 		 */
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present
  2026-07-09 19:19 [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present Michael Bommarito
@ 2026-07-10 16:19 ` Matthieu Baerts
  2026-07-10 17:17   ` Michael Bommarito
  0 siblings, 1 reply; 4+ messages in thread
From: Matthieu Baerts @ 2026-07-10 16:19 UTC (permalink / raw)
  To: Michael Bommarito, Mat Martineau
  Cc: Geliang Tang, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Gang Yan, mptcp, netdev, linux-kernel

Hi Michael,

On 09/07/2026 21:19, Michael Bommarito wrote:
> 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.
> 
> Move the pr_debug() that logs the parsed DSS flags below the mapping
> block, so it reports the final data_fin value instead of the stale one
> it would otherwise print before the assignment.

Thank you for the v3, it looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I tried to reproduce it on my side using packetdrill-mptcp [1], but I
was not able to test with KMSAN: my kernel boot, but is stuck when KMSAN
is enabled... By chance, if you can try this reproducer on your side,
with and without your patched kernel, that would be great :)

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

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present
  2026-07-10 16:19 ` Matthieu Baerts
@ 2026-07-10 17:17   ` Michael Bommarito
  2026-07-10 17:30     ` Matthieu Baerts
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Bommarito @ 2026-07-10 17:17 UTC (permalink / raw)
  To: Matthieu Baerts
  Cc: Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Gang Yan, mptcp,
	netdev, linux-kernel

On Fri, Jul 10, 2026 at 12:19 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> I tried to reproduce it on my side using packetdrill-mptcp [1], but I
> was not able to test with KMSAN: my kernel boot, but is stuck when KMSAN
> is enabled... By chance, if you can try this reproducer on your side,
> with and without your patched kernel, that would be great :)
>
> [1] https://github.com/multipath-tcp/packetdrill/pull/203

Can you try with more RAM and make sure that you have these set too?

CONFIG_CRYPTO_USER_API=y
CONFIG_CRYPTO_USER_API_HASH=y

I was able to get your PR #203 test to trigger after using -m 8G +
AF_ALG under vng

  BUG: KMSAN: uninit-value in mptcp_incoming_options+0x1da9
   mptcp_incoming_options
   tcp_data_queue
   tcp_rcv_established
   tcp_v4_do_rcv
   tcp_v4_rcv
   ip_protocol_deliver_rcu
   ip_local_deliver_finish
   ip_local_deliver
   ip_rcv

Thanks,
Mike

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present
  2026-07-10 17:17   ` Michael Bommarito
@ 2026-07-10 17:30     ` Matthieu Baerts
  0 siblings, 0 replies; 4+ messages in thread
From: Matthieu Baerts @ 2026-07-10 17:30 UTC (permalink / raw)
  To: Michael Bommarito
  Cc: Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Gang Yan, mptcp,
	netdev, linux-kernel

On 10/07/2026 19:17, Michael Bommarito wrote:
> On Fri, Jul 10, 2026 at 12:19 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>>
>> I tried to reproduce it on my side using packetdrill-mptcp [1], but I
>> was not able to test with KMSAN: my kernel boot, but is stuck when KMSAN
>> is enabled... By chance, if you can try this reproducer on your side,
>> with and without your patched kernel, that would be great :)
>>
>> [1] https://github.com/multipath-tcp/packetdrill/pull/203
> 
> Can you try with more RAM and make sure that you have these set too?
> 
> CONFIG_CRYPTO_USER_API=y
> CONFIG_CRYPTO_USER_API_HASH=y

Thank you for the suggestion. Yes, I did use more RAM (just tried with
8G just to be sure), and with the CRYPTO_USER_API (needed for
packetdrill), but the VM stalls at boot time:

  Starting KernelMemorySanitizer
  ATTENTION: KMSAN is a debugging tool! Do not use it on production
machines!

I didn't investigate more, but I'm using KMSAN on another similar
machine with syzkaller without issues.
> I was able to get your PR #203 test to trigger after using -m 8G +
> AF_ALG under vng

Nice, thank you for having checked that! I guess I can merge this PR then.

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-07-10 17:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-09 19:19 [PATCH net v3] mptcp: only set DATA_FIN when a mapping is present Michael Bommarito
2026-07-10 16:19 ` Matthieu Baerts
2026-07-10 17:17   ` Michael Bommarito
2026-07-10 17:30     ` Matthieu Baerts

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox