[PATCH net 0/5] mptcp: misc fixes for v7.2-rc6

[PATCH net 0/5] mptcp: misc fixes for v7.2-rc6

[Matthieu Baerts (NGI0)]

Here are various unrelated fixes:

- Patch 1: harden incoming MPTCP suboptions parsing by rejecting
  non-combinable ones. A fix for v5.6.

- Patch 2: fix a data race in the ADD_ADDR timer callback. A fix for
  v5.13.

- Patch 3: correctly catch data corruption during the MPTCP join
  selftest by marking tests as failed, instead of only printing a
  warning. A fix for v5.18.

- Patch 4: deal with MPTFO with a valid token, but no data in the SYN. A
  fix for v6.2.

- Patch 5: reclaim forward-allocated memory in case of error on the
  receive side. A fix for v6.19.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Gang Yan (1):
      selftests: mptcp: join: mark tests with data corruption as failed

Matthieu Baerts (NGI0) (1):
      mptcp: avoid combining some incoming suboptions

Paolo Abeni (1):
      mptcp: reclaim forward-allocated memory on RX path errors

Qing Luo (1):
      mptcp: pm: fix data race in add_addr timer callback

Wyatt Feng (1):
      mptcp: fastopen: only mark MPTFO subflows with SYN data

 net/mptcp/fastopen.c                            |  7 ++--
 net/mptcp/options.c                             | 55 +++++++++++++++++++++++++
 net/mptcp/pm.c                                  |  6 ++-
 net/mptcp/protocol.c                            |  7 ++++
 net/mptcp/protocol.h                            |  1 +
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  4 +-
 6 files changed, 73 insertions(+), 7 deletions(-)
---
base-commit: 78f75d632f74b8de0f081a128588f7c37d0d1164
change-id: 20260728-net-mptcp-misc-fixes-7-2-rc6-22213c66ef75

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Matthieu Baerts (NGI0)]

Some MPTCP suboptions are mutually exclusive according to the RFC8684,
but also because in different places, the code doesn't expect some
combinations to be present. That's specially true for suboptions that
would be present twice, but with different attributes.

The new restrictions are the same as the ones applied on the output
side, with mptcp_write_options. The same rules can be reused:

  Which options can be used together?

  X: mutually exclusive
  O: often used together
  C: can be used together in some cases
  P: could be used together but we prefer not to (optimisations)

  | Opt: | MPC  | MPJ  | DSS  | ADD  |  RM  | PRIO | FAIL |  FC  |
  |------|------|------|------|------|------|------|------|------|
  | MPC  |------|------|------|------|------|------|------|------|
  | MPJ  |  X   |------|------|------|------|------|------|------|
  | DSS  |  X   |  X   |------|------|------|------|------|------|
  | ADD  |  X   |  X   |  P   |------|------|------|------|------|
  | RM   |  C   |  C   |  C   |  P   |------|------|------|------|
  | PRIO |  X   |  C   |  C   |  C   |  C   |------|------|------|
  | FAIL |  X   |  X   |  C   |  X   |  X   |  X   |------|------|
  | FC   |  X   |  X   |  X   |  X   |  X   |  X   |  X   |------|
  | RST  |  X   |  X   |  X   |  X   |  X   |  X   |  O   |  O   |
  |------|------|------|------|------|------|------|------|------|

The only difference is with the 'P': another stack could send and
ADD_ADDR with other suboptions (DSS, RM_ADDR), and this should be
allowed.

A point of attention is with the MP_CAPABLE: it could be used with a
RM_ADDR, but there is no reason to add it with a SYN. Note that even
with a 4th ACK, it doesn't seem to be useful, except when IDs are known
in advance via another channel. Better not to break that.

Also, in mp_opt->suboptions, there is also a bit reserved to the
checksum, which can be used in an MP_CAPABLE and a DSS. Each time a DSS
option can be used in parallel with another option, the checksum can be
set, so the verification is combined into a new OPTIONS_MPTCP_DSS macro.

Fixes: eda7acddf808 ("mptcp: Handle MPTCP TCP options")
Cc: stable@vger.kernel.org
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Florian Westphal <fw@strlen.de>
Cc: Davide Caratti <dcaratti@redhat.com>
Cc: Christoph Paasch <cpaasch@openai.com>
Note: Peter Krystad's email address is no longer valid
---
 net/mptcp/options.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.h |  1 +
 2 files changed, 56 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c664023d37ba..3b8539f1c224 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -50,6 +50,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			}
 		}
 
+		/* Only the MPC + ACK can be used with a RM_ADDR */
+		if (subopt == OPTION_MPTCP_MPC_ACK) {
+			if ((mp_opt->suboptions & ~OPTION_MPTCP_RM_ADDR) != 0)
+				break;
+		} else if (mp_opt->suboptions != 0) {
+			break;
+		}
+
 		/* Cfr RFC 8684 Section 3.3.0:
 		 * If a checksum is present but its use had
 		 * not been negotiated in the MP_CAPABLE handshake, the receiver MUST
@@ -122,6 +130,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_MP_JOIN:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_RM_ADDR |
+					    OPTION_MPTCP_PRIO)) != 0)
+			break;
+
 		if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
 			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYN;
 			mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
@@ -153,6 +166,13 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_DSS:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_ADD_ADDR |
+					    OPTION_MPTCP_RM_ADDR |
+					    OPTION_MPTCP_PRIO |
+					    OPTION_MPTCP_FAIL)) != 0)
+			break;
+
 		pr_debug("DSS\n");
 		ptr++;
 
@@ -234,6 +254,12 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_ADD_ADDR:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_RM_ADDR |
+					    OPTION_MPTCP_PRIO)) != 0)
+			break;
+
 		mp_opt->echo = (*ptr++) & MPTCP_ADDR_ECHO;
 		if (!mp_opt->echo) {
 			if (opsize == TCPOLEN_MPTCP_ADD_ADDR ||
@@ -293,6 +319,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_RM_ADDR:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_MPC_ACK |
+					    OPTIONS_MPTCP_MPJ |
+					    OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_ADD_ADDR |
+					    OPTION_MPTCP_PRIO)) != 0)
+			break;
+
 		if (opsize < TCPOLEN_MPTCP_RM_ADDR_BASE + 1 ||
 		    opsize > TCPOLEN_MPTCP_RM_ADDR_BASE + MPTCP_RM_IDS_MAX)
 			break;
@@ -307,6 +341,13 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_MP_PRIO:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_MPJ |
+					    OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_ADD_ADDR |
+					    OPTION_MPTCP_RM_ADDR)) != 0)
+			break;
+
 		if (opsize != TCPOLEN_MPTCP_PRIO)
 			break;
 
@@ -316,6 +357,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_MP_FASTCLOSE:
+		/* Can be used only with RST */
+		if ((mp_opt->suboptions & ~OPTION_MPTCP_RST) != 0)
+			break;
+
 		if (opsize != TCPOLEN_MPTCP_FASTCLOSE)
 			break;
 
@@ -327,6 +372,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_RST:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_FAIL |
+					    OPTION_MPTCP_FASTCLOSE)) != 0)
+			break;
+
 		if (opsize != TCPOLEN_MPTCP_RST)
 			break;
 
@@ -342,6 +392,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_MP_FAIL:
+		/* Can be used with a restricted number of other options */
+		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_RST)) != 0)
+			break;
+
 		if (opsize != TCPOLEN_MPTCP_FAIL)
 			break;
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4a2d40cd7b13..c13680d18994 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -37,6 +37,7 @@
 				 OPTION_MPTCP_MPC_ACK)
 #define OPTIONS_MPTCP_MPJ	(OPTION_MPTCP_MPJ_SYN | OPTION_MPTCP_MPJ_SYNACK | \
 				 OPTION_MPTCP_MPJ_ACK)
+#define OPTIONS_MPTCP_DSS	(OPTION_MPTCP_DSS | OPTION_MPTCP_CSUMREQD)
 
 /* MPTCP option subtypes */
 #define MPTCPOPT_MP_CAPABLE	0

-- 
2.53.0

[PATCH net 2/5] mptcp: pm: fix data race in add_addr timer callback

[Matthieu Baerts (NGI0)]

From: Qing Luo <luoqing@kylinos.cn>

The timer callback reads entry->retrans_times outside pm.lock to decide
whether to call mptcp_pm_subflow_established(). Since
mptcp_pm_announced_del_timer() can concurrently set retrans_times =
ADD_ADDR_RETRANS_MAX under pm.lock, a race condition exists.

I discovered this issue while studying the code. AI tools helped me to
verify the issue can potentially happen under race conditions.

Use a local 'retransmit' flag set inside pm.lock to capture whether
retransmission is still possible. This ensures that
mptcp_pm_subflow_established() is only called when the retransmission
naturally exhausts.

Fixes: 348d5c1dec60 ("mptcp: move to next addr when timeout")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Luo <luoqing@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6afd39aea110..c71dcf887683 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -380,6 +380,7 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
 	struct mptcp_sock *msk = entry->sock;
 	struct sock *sk = (struct sock *)msk;
 	unsigned int timeout = 0;
+	bool retransmit;
 
 	pr_debug("msk=%p\n", msk);
 
@@ -412,14 +413,15 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
 		entry->retrans_times++;
 	}
 
-	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
+	retransmit = entry->retrans_times < ADD_ADDR_RETRANS_MAX;
+	if (retransmit)
 		timeout <<= entry->retrans_times;
 	else
 		timeout = 0;
 
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry->retrans_times == ADD_ADDR_RETRANS_MAX)
+	if (!retransmit)
 		mptcp_pm_subflow_established(msk);
 
 out:

-- 
2.53.0

[PATCH net 3/5] selftests: mptcp: join: mark tests with data corruption as failed

[Matthieu Baerts (NGI0)]

From: Gang Yan <yangang@kylinos.cn>

check_transfer() compares the input and output files byte-by-byte using
`cmp -l "$in" "$out" | while read ...`. Because the while-loop body runs
in a subshell (the script sets neither lastpipe nor pipefail), the
fail_test call inside it -- which sets the global ret/last_test_failed --
and the `return 1` both act on the subshell, not on check_transfer().

check_transfer() thus always falls through to `return 0`, and any data
corruption affecting only the payload (leaving the subflow/PM counters
untouched) is silently reported as PASS.

Fixes: 8117dac3e7c3 ("selftests: mptcp: add invert check in check_transfer")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GLM-5.2
Signed-off-by: Gang Yan <yangang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c0aeffd5cb71..7dc91fac4917 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -584,7 +584,7 @@ check_transfer()
 		mv "$tmpfile" "$out"
 		tmpfile=""
 	fi
-	cmp -l "$in" "$out" | while read -r i a b; do
+	while read -r i a b; do
 		local sum=$((0${a} + 0${b}))
 		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
 			fail_test "$what does not match (in, out):"
@@ -595,7 +595,7 @@ check_transfer()
 		else
 			print_info "$what has inverted byte at ${i}"
 		fi
-	done
+	done < <(cmp -l "$in" "$out")
 
 	return 0
 }

-- 
2.53.0

[PATCH net 4/5] mptcp: fastopen: only mark MPTFO subflows with SYN data

[Matthieu Baerts (NGI0)]

From: Wyatt Feng <bronzed_45_vested@icloud.com>

Passive TCP Fast Open accepts a valid-cookie SYN even when it carries
no data. In that case the child socket's receive queue is intentionally
left empty.

mptcp_fastopen_subflow_synack_set_params() set is_mptfo before checking
for queued SYN data. That made data-less TFO SYNs hit a WARN and, if
the warning was non-fatal, left stale MPTFO state behind. The stale
flag could later trigger a state-confusion bug in
check_fully_established().

Only mark the subflow as MPTFO after confirming that a SYN-data skb is
present. Return quietly when the receive queue is empty.

Fixes: 36b122baf6a8 ("mptcp: add subflow_v(4,6)_send_synack()")
Cc: stable@vger.kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Zhengchuan Liang <zcliangcn@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Assisted-by: Codex:GPT-5.4
Signed-off-by: Wyatt Feng <bronzed_45_vested@icloud.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Dmytro Shytyi <dmytro@shytyi.net>
Note: a v1 has already been shared alone on the netdev ML:
  https://patch.msgid.link/81f26b8fddd59ebb6cecc417fb138d9ff5214e08.1780458440.git.bronzed_45_vested@icloud.com
---
 net/mptcp/fastopen.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 082c46c0f50e..f717750906ff 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -24,12 +24,13 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	sk = subflow->conn;
 	tp = tcp_sk(ssk);
 
-	subflow->is_mptfo = 1;
-
+	/* A valid TFO cookie does not guarantee SYN data. */
 	skb = skb_peek(&ssk->sk_receive_queue);
-	if (WARN_ON_ONCE(!skb))
+	if (!skb)
 		return;
 
+	subflow->is_mptfo = 1;
+
 	/* dequeue the skb from sk receive queue */
 	__skb_unlink(skb, &ssk->sk_receive_queue);
 	skb_ext_reset(skb);

-- 
2.53.0

[PATCH net 5/5] mptcp: reclaim forward-allocated memory on RX path errors

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

After commit 9db5b3cec4ec ("mptcp: borrow forward memory from subflow"),
errors in the receive path prior to queueing skbs into the receive
queue do not trigger forward-allocated memory reclaiming.

Prevent forward memory from growing unboundedly in pathological drop
scenarios by explicitly reclaiming memory when skbs are dropped.

Fixes: 9db5b3cec4ec ("mptcp: borrow forward memory from subflow")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ca644ec53eed..47eaf64a54ba 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -149,6 +149,13 @@ struct sock *__mptcp_nmpc_sk(struct mptcp_sock *msk)
 
 static void mptcp_drop(struct sock *sk, struct sk_buff *skb)
 {
+	/*
+	 * The skb forward memory was already transferred to sk by
+	 * mptcp_borrow_fwdmem(), even before setting the destructor.
+	 */
+	if (!skb->destructor)
+		sk_mem_reclaim(sk);
+
 	sk_drops_skbadd(sk, skb);
 	__kfree_skb(skb);
 }

-- 
2.53.0

Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Jakub Kicinski]

On Tue, 28 Jul 2026 19:11:57 +0200 Matthieu Baerts (NGI0) wrote:
> Some MPTCP suboptions are mutually exclusive according to the RFC8684,
> but also because in different places, the code doesn't expect some
> combinations to be present. That's specially true for suboptions that
> would be present twice, but with different attributes.

Looks like Clashiko has much to say about this patch.
Could you check?

From my perspective I'm not sure if such RFC compliance changes
really belong as Fixes? I don't feel too strongly tho.

Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Matthieu Baerts]

Hi Jakub,

On 30/07/2026 02:23, Jakub Kicinski wrote:
> On Tue, 28 Jul 2026 19:11:57 +0200 Matthieu Baerts (NGI0) wrote:
>> Some MPTCP suboptions are mutually exclusive according to the RFC8684,
>> but also because in different places, the code doesn't expect some
>> combinations to be present. That's specially true for suboptions that
>> would be present twice, but with different attributes.
> 
> Looks like Clashiko has much to say about this patch.
> Could you check?

Sure, I will check that.

Do you think Clashiko could look at patches from the MPTCP ML as well?
Because the (deprecated?) AI review tool we use didn't find anything:


https://netdev-ai.bots.linux.dev/ai-review.html?id=d1d1ff69-0f15-4812-ad16-e4f6f2df9f6b

Or maybe that's because the model is different, and this can be easily
fixed? Or maybe the prompts are different too?

I'm asking, mainly because once patches have been accepted in our tree,
it can be hard to have the original author fixing them. Not to block
other patches too long, I often have to fix them, so I would prefer to
get the same review tools (if possible) to prevent that :)

> From my perspective I'm not sure if such RFC compliance changes
> really belong as Fixes? I don't feel too strongly tho.

I think this fix is important: it is not really about RFC compliances,
but more about the fact the rest of the code is not taking into account
suboptions combinations. For example, if the same suboption is present
twice, but with different values, then the parsing will end up with a
mix of the two. Also, when such combinations are used, it is likely the
remote peer is attempting an attack, so better to block this early.

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

Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Jakub Kicinski]

On Thu, 30 Jul 2026 10:14:53 +0200 Matthieu Baerts wrote:
> On 30/07/2026 02:23, Jakub Kicinski wrote:
> > On Tue, 28 Jul 2026 19:11:57 +0200 Matthieu Baerts (NGI0) wrote:  
> >> Some MPTCP suboptions are mutually exclusive according to the RFC8684,
> >> but also because in different places, the code doesn't expect some
> >> combinations to be present. That's specially true for suboptions that
> >> would be present twice, but with different attributes.  
> > 
> > Looks like Clashiko has much to say about this patch.
> > Could you check?  
> 
> Sure, I will check that.
> 
> Do you think Clashiko could look at patches from the MPTCP ML as well?
> Because the (deprecated?) AI review tool we use didn't find anything:
> 
> https://netdev-ai.bots.linux.dev/ai-review.html?id=d1d1ff69-0f15-4812-ad16-e4f6f2df9f6b
> 
> Or maybe that's because the model is different, and this can be easily
> fixed? Or maybe the prompts are different too?
> 
> I'm asking, mainly because once patches have been accepted in our tree,
> it can be hard to have the original author fixing them. Not to block
> other patches too long, I often have to fix them, so I would prefer to
> get the same review tools (if possible) to prevent that :)

Sorry about that, I know it's annoying. Not the best time TBH.
I'm modifying our instances very actively, I don't want to have
to worry about the extra surface of integrations. Hopefully things
converge soon and we can switch you over and kill that old service
we had completely.

Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Matthieu Baerts]

Hi Jakub,

On 30/07/2026 22:27, Jakub Kicinski wrote:
> On Thu, 30 Jul 2026 10:14:53 +0200 Matthieu Baerts wrote:
>> On 30/07/2026 02:23, Jakub Kicinski wrote:
>>> On Tue, 28 Jul 2026 19:11:57 +0200 Matthieu Baerts (NGI0) wrote:  
>>>> Some MPTCP suboptions are mutually exclusive according to the RFC8684,
>>>> but also because in different places, the code doesn't expect some
>>>> combinations to be present. That's specially true for suboptions that
>>>> would be present twice, but with different attributes.  
>>>
>>> Looks like Clashiko has much to say about this patch.
>>> Could you check?  
>>
>> Sure, I will check that.
>>
>> Do you think Clashiko could look at patches from the MPTCP ML as well?
>> Because the (deprecated?) AI review tool we use didn't find anything:
>>
>> https://netdev-ai.bots.linux.dev/ai-review.html?id=d1d1ff69-0f15-4812-ad16-e4f6f2df9f6b
>>
>> Or maybe that's because the model is different, and this can be easily
>> fixed? Or maybe the prompts are different too?
>>
>> I'm asking, mainly because once patches have been accepted in our tree,
>> it can be hard to have the original author fixing them. Not to block
>> other patches too long, I often have to fix them, so I would prefer to
>> get the same review tools (if possible) to prevent that :)
> 
> Sorry about that, I know it's annoying. Not the best time TBH.
> I'm modifying our instances very actively, I don't want to have
> to worry about the extra surface of integrations. Hopefully things
> converge soon and we can switch you over and kill that old service
> we had completely.

No problem, I understand and can wait.

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

Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Matthieu Baerts]

Hello,

TL;DR: a v2 is expected

pw-bot: cr

I'm replying here to Clashiko's comments:

> mptcp: avoid combining some incoming suboptions
> 
> This adds combination checks to the incoming MPTCP suboption parser in
> mptcp_parse_option(), reusing the exclusivity table already applied on the
> transmit side, plus a new OPTIONS_MPTCP_DSS helper macro that folds the
> checksum bit into the DSS bit for those checks.
> 
> A few questions about the receive-side behaviour below.
> 
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index c664023d37ba..3b8539f1c224 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -50,6 +50,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  			}
>>  		}
>>  
>> +		/* Only the MPC + ACK can be used with a RM_ADDR */
>> +		if (subopt == OPTION_MPTCP_MPC_ACK) {
>> +			if ((mp_opt->suboptions & ~OPTION_MPTCP_RM_ADDR) != 0)
>> +				break;
> 
> Can this drop either the keys or the data mapping when a peer puts an
> MP_CAPABLE(ACK) and a separate DSS in the same third ACK?
> 
> mptcp_get_options() walks the TCP options in wire order and calls
> mptcp_parse_option() once per option, so each gate only sees the options
> that came earlier in the segment:
> 
>   [DSS, MP_CAPABLE ACK] -> this gate sees OPTION_MPTCP_DSS set and breaks,
>                            so sndr_key/rcvr_key are never stored
>   [MP_CAPABLE ACK, DSS] -> the DSS gate below does not allow
>                            OPTIONS_MPTCP_MPC, so the mapping is dropped
> 
> The comment a few lines further down in this same case quotes RFC 8684
> Section 3.1, "the data parameters in a MP_CAPABLE are semantically
> equivalent to those in a DSS option and can be used interchangeably", which
> does not seem to forbid a separate DSS in the same segment.
> 
> Losing the MP_CAPABLE here ends in a fallback to plain TCP; losing the DSS
> leaves the payload of that ACK without a mapping. Neither path bumps a MIB
> counter nor emits a pr_debug, so should there be some counter to make the
> drop visible?

An MPC ACK is not supposed to be used with a DSS: the MPC ACK can carry
a Data-Level Length, and optional Checksum, which are the same as the
ones we can find in a DSS, but they can be present in an MPC ACK because
a DSS is not supposed to be sent in combination to an MPC ACK.

Note that an MPC ACK without data takes 20B, 24B with data. A DSS with
data is taking 20 to 28B, not leaving much space out of the 40B. That's
one reason why they shouldn't be used together.

>> +		} else if (mp_opt->suboptions != 0) {
>> +			break;
>> +		}
> 
> Since the checks are evaluated against the suboptions accumulated so far,
> does the surviving suboption now depend on the order the peer chose, which
> RFC 8684 does not define?
> 
>   [RM_ADDR, MP_CAPABLE SYN] -> this else-if branch breaks, OPTION_MPTCP_MPC_SYN
>                                is never set, subflow_check_req() sees no MPC and
>                                the passive socket stays plain TCP
>   [MP_CAPABLE SYN, RM_ADDR] -> only the RM_ADDR is dropped, MPTCP still works
> 
>   [ADD_ADDR, MP_JOIN SYN]   -> the MP_JOIN gate below tolerates only
>                                RM_ADDR|PRIO, so the join is dropped
>   [MP_JOIN SYN, ADD_ADDR]   -> only the ADD_ADDR is dropped
> 
> Would it be preferable to decide on the full set of parsed suboptions once
> the whole option list has been walked, so that the outcome does not depend
> on sender-side ordering?

I don't think so: having such combinations either means the remote peer
is buggy, or is evil. I don't think the code should try to support these
bad cases, and it should do the simplest action when this is seen: stop
processing the current suboption.

> Do these two gates make the existing MP_CAPABLE + MP_JOIN handling in
> net/mptcp/subflow.c unreachable?
> 
> subflow_check_req() has:
> 
> 	if (opt_mp_capable) {
> 		...
> 		if (opt_mp_join)
> 			return 0;
> 	}
> 
> and mptcp_subflow_init_cookie_req() has:
> 
> 	if (opt_mp_capable && opt_mp_join)
> 		return -EINVAL;

Correct: but it sounds better to keep them here for the moment, just an
extra cheap check. They can be removed in -next in a follow-up patch.

> After this patch mp_opt->suboptions can no longer hold an MPC bit together
> with an MPJ bit, so both branches look dead, and a SYN carrying both
> suboptions is no longer treated as ambiguous but handled as whichever option
> appeared first. Should the commit message mention that semantic change,
> given the Fixes and Cc: stable tags?

Now, all inconsistent combinations are handled the same way. I don't
think we should add an exception to this MPC+MPJ case. A precision could
be added to the commit message for this unexpected error path.

>> +
>>  		/* Cfr RFC 8684 Section 3.3.0:
>>  		 * If a checksum is present but its use had
>>  		 * not been negotiated in the MP_CAPABLE handshake, the receiver MUST
>> @@ -122,6 +130,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_MP_JOIN:
>> +		/* Can be used with a restricted number of other options */
>> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_RM_ADDR |
>> +					    OPTION_MPTCP_PRIO)) != 0)
>> +			break;
>> +
> 
> Is an MP_JOIN(ACK) combined with a DSS still expected to work?
> 
> MPJ_ACK is 24 bytes and a DSS with a 32-bit mapping is 14 bytes, so 38 bytes
> fit in the TCP option space and both were parsed before this patch. With
> this mask, [DSS, MP_JOIN ACK] drops the MP_JOIN and [MP_JOIN ACK, DSS] drops
> the mapping in the DSS gate below, leaving the payload of the fourth ACK
> without a mapping.

14 bytes is without the "base": Kind + Length + Subtype + Flags. So 4
more bytes are required (+ 2 for the alignment). The total then exceeds 40.

>>  		if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
>>  			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYN;
>>  			mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
>> @@ -153,6 +166,13 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_DSS:
>> +		/* Can be used with a restricted number of other options */
>> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_ADD_ADDR |
>> +					    OPTION_MPTCP_RM_ADDR |
>> +					    OPTION_MPTCP_PRIO |
>> +					    OPTION_MPTCP_FAIL)) != 0)
>> +			break;
>> +
> 
> This isn't a bug introduced by this patch, but it is the same class of
> malformed-combination state the patch is trying to constrain, and the new
> masks cannot see it because they only look at suboption bits.
> 
> In this case the DSS flags are committed to mp_opt before the length is
> validated:
> 
> net/mptcp/options.c:mptcp_parse_option() {
> 	...
> 	flags = (*ptr++) & MPTCP_DSS_FLAG_MASK;
> 	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);
> 	...
> 	if (opsize != expected_opsize &&
> 	    opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
> 		break;
> 	...
> }
> 
> On that break path no suboption bit is set, so mp_opt->suboptions is still
> 0 and both this gate and the MP_CAPABLE gate above are passed by a second
> option in the same segment, while use_ack/ack64/use_map/data_fin keep the
> values from the rejected DSS.
> 
> mptcp_get_options() only clears the four-byte status group:
> 
> 	*(u32 *)&mp_opt->status = 0;
> 
> so data_ack, data_seq, subflow_seq, data_len and csum of the caller's
> on-stack struct mptcp_options_received are untouched unless the matching
> parse path writes them.
> 
> Two sequences that reach the consumer with those stale flags:
> 
>   [malformed DSS with HAS_ACK, MP_CAPABLE ACK+DATA]
>       MP_CAPABLE sets OPTION_MPTCP_DSS, use_ack is still 1, so
>       mptcp_incoming_options() runs
> 
> 	if (mp_opt.use_ack)
> 		ack_update_msk(msk, sk, &mp_opt);
> 
>       with an uninitialized mp_opt.data_ack feeding mptcp_expand_seq() and
>       the msk->snd_una / msk->wnd_end bookkeeping.
> 
>   [malformed DSS with HAS_MAP|DATA_FIN, valid ack-only DSS]
>       data_fin is only reassigned under if (mp_opt->use_map), so the stale
>       data_fin survives, and on a pure ACK mptcp_incoming_options() runs
> 
> 	if (mp_opt.data_fin && mp_opt.data_len == 1 &&
> 	    mptcp_update_rcv_data_fin(msk, mp_opt.data_seq, mp_opt.dsn64))
> 
>       with uninitialized data_len/data_seq; if the leftover data_len happens
>       to be 1, mptcp_update_rcv_data_fin() records a DATA_FIN at an
>       arbitrary sequence.
> 
> Would it make sense to move the DSS scratch-field assignments after the
> opsize validation, or to zero the whole mp_opt in mptcp_get_options(), so
> the new combination checks operate on a clean state?

Indeed, but that's an existing issue. This will be addressed in a
follow-up patch.

>>  		pr_debug("DSS\n");
>>  		ptr++;
>>  
> 
> [ ... ]
> 
>> @@ -316,6 +357,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_MP_FASTCLOSE:
>> +		/* Can be used only with RST */
>> +		if ((mp_opt->suboptions & ~OPTION_MPTCP_RST) != 0)
>> +			break;
>> +
> 
> Does this stop an MP_FASTCLOSE carried in a plain ACK from being honoured?
> 
> RFC 8684 Section 3.5 allows MP_FASTCLOSE in an ACK, and this parser
> deliberately does not require TCPHDR_RST for MP_FASTCLOSE, unlike the
> MPTCPOPT_RST case which does:
> 
> 	if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
> 		break;
> 
> An ACK on an established subflow normally also carries a DSS data-ACK, so
> either the FASTCLOSE is dropped here (DSS parsed first) or the DSS is
> dropped by the DSS gate above (FASTCLOSE parsed first, since that mask does
> not include OPTION_MPTCP_FASTCLOSE). When the FASTCLOSE is dropped,
> msk->rcv_fastclose is never set and the connection is not aborted.
> 
> The existing consumer looks like it was written for exactly that
> combination:
> 
> net/mptcp/options.c:mptcp_incoming_options() {
> 	if (unlikely(mp_opt.suboptions != OPTION_MPTCP_DSS)) {
> 		if ((mp_opt.suboptions & OPTION_MPTCP_FASTCLOSE) &&
> 		    READ_ONCE(msk->local_key) == mp_opt.rcvr_key) {
> 			WRITE_ONCE(msk->rcv_fastclose, true);
> 	...
> 		if (!(mp_opt.suboptions & OPTION_MPTCP_DSS))
> 			return true;
> 	}
> 	...
> }
> 
> With this restriction the FASTCLOSE plus DSS fall-through into the DSS
> handling can no longer be reached. Was the intent to make that path dead, or
> should FASTCLOSE be allowed together with OPTIONS_MPTCP_DSS as well?
> 
>>  		if (opsize != TCPOLEN_MPTCP_FASTCLOSE)
>>  			break;
>>  
> 
> [ ... ]

Yes, indeed, in the rcv path, it is possible another networking stack
sends an MP_FASTCLOSE with a DSS, and that's valid.

This will be addressed in v2.

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