[PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage

[PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v20:
 - patch 2: fix Medium issues from review. Move MPTCP_SYNC_SEQ check
   inside the loop under mptcp_data_lock() protection to avoid race with
   concurrent non-atomic cb_flags updates. Change from atomic
   test_and_clear_bit() to non-atomic __test_and_clear_bit() since we're
   under lock. Add msk->tfo_iasn = 0 reset in mptcp_disconnect() to
   prevent stale tfo_iasn from corrupting passive TFO fallback.
 - patch 3: new patch. Fix TFO map_seq sign extension in fastopen.c by
   casting to u64 before negation: -(u64)skb->len instead of -skb->len.
 - patch 4: updated from v19 patch 3. Moved the -(u64)skb->len fix to
   patch 3. Added comment in mptcp_init_skb() explaining why offset > 0
   with out-of-order mapping is correct: mptcp_subflow_get_mapped_dsn()
   returns the DSN for the byte at tp->copied_seq (position 'offset' in
   the skb), and consumed prefix bytes are part of the same logical DSN
   range.
 - patch 6: updated from v19 patch 5. Replace msk_owned_by_me() assertion
   with DEBUG_NET_WARN_ON_ONCE using lockdep_sock_is_held() for both
   process and BH contexts. Add conditional defer logic: inline cleanup
   when caller owns socket lock, defer to worker when caller doesn't.

v19:
 - Replace mptcp_iasn() computed function with msk->tfo_iasn immutable
   snapshot (set in subflow_set_remote_key()), removing tfo_skb_len field
   and the DATA_FIN off-by-one concern.
 - Move MPTCP_SYNC_SEQ handling (test_and_clear_bit + copied_seq update)
   out of __mptcp_sync_rcv_sequence() into callers, since the function
   can be called without the socket lock held.
 - Patch 2: move MPTCP_SYNC_SEQ check in mptcp_release_cb() from
   post-loop to loop start (before backlog spooling). Add the same
   check in mptcp_move_skbs() to sync copied_seq before backlog
   processing when user holds the socket lock.
 - Patch 4: clamp max_seq to ack_seq in mptcp_data_queue_ofo() to
   prevent 64-bit after64() wrap-around when ack_seq > rcv_wnd_sent.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793361629.git.tanggeliang@kylinos.cn/

v18:
 - Re-add patch 2 "mptcp: drop the cant_coalesce CB field".
 - Patch 3: simplify TFO sync by relying on patch 2's __mptcp_sync_rcv_sequence()
   instead of complex copied_seq checks. Use copied_seq += mptcp_iasn(msk).
 - Patch 4: move subflow.c changes to patch 2, use map_seq64 for TFO init.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793349369.git.geliang@kernel.org/

v17:
 - Patch 2: add TFO synchronization: subflow_set_remote_key() preserves
   partial read progress by checking copied_seq against the initial TFO
   value before updating. Add WRITE_ONCE/READ_ONCE for ordering between
   BH (3rd ACK processing) and user-space readers. Add underflow guard
   (copied_seq >= map_seq) before offset subtraction in
   __mptcp_recvmsg_mskq() to protect MSG_PEEK traversals. Skip skbs
   entirely before copied_seq. Update subflow.c to set both map_seq
   (u32) and map_seq64 (u64) for TFO.
 - Patch 3: adapt to the new map_seq (u32) + map_seq64 (u64) layout
   introduced in patch 2.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793326853.git.tanggeliang@kylinos.cn/

v16:
 - Simplified the series to retain only the core changes required by both
   kTLS and sockmap: drop the CB offset field, add copied_seq tracking,
   and sync the MPTCP skb CB layout with the TCP one.
 - Dropped patch 2, leaving the fastopen code unchanged.
 - Moved patches 6-11 into a separate series "prepare interfaces for kTLS
   and sockmap support".
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793255126.git.tanggeliang@kylinos.cn/

v15:
 - Patch 2: drop mptcp_data_lock() in mptcp_inq_hint(), use READ_ONCE()
   and __test_and_clear_bit() instead. This issue is unrelated to this
   series and will be addressed in a separate patch later.
 - Patch 5: Guard mptcp_cleanup_rbuf() with read_copied > 0. Initialize/
   reset msk->read_copied. Drop redundant mptcp_recv_skb() in
   __mptcp_read_sock().
 - Patch 6: simplify memory ordering, drop smp_mb()/smp_rmb(), keep
   existing smp_mb__before_atomic(). Check SOCK_DONE inline.
 - Patch 9: drop tcp_sock_rate_check_app_limited() split, call
   tcp_rate_check_app_limited() directly as Paolo suggested. Add
   write_seq != snd_nxt early bail-out.
 - Patch 10: wrap assertion behind expect_all_features().
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1792718041.git.tanggeliang@kylinos.cn/

v14:
 - v13 can no longer be applied to the export branch, so it has been
   rebased here with the following corrections:
 - Patch 2: move locking in mptcp_inq_hint() to avoid races with ack_seq
   change from patch 3 to here. Update commit log.
 - Patch 3: add mptcp_data_lock() in __mptcp_move_skb() around copied_seq
   update. Remove local delta variable in __mptcp_ofo_queue(). Initialize
   msk->copied_seq to 0 in mptcp_propagate_state() fallback path.
 - Patch 4: add map_seq64 comparison in __mptcp_try_coalesce() for 64-bit
   sequence consistency.
 - New patch 11: rename mptcp_check_readable to mptcp_stream_is_readable,
   and align with TCP by calling sk_is_readable() for KTLS support.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1790734083.git.tanggeliang@kylinos.cn/

v13:
 - use smp_mb instead of smp_mb__before_atomic in mptcp_check_data_fin.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1788245079.git.tanggeliang@kylinos.cn/

v12:
 - Move sock_set_flag(SOCK_DONE) to after smp_mb__before_atomic() in
   mptcp_check_data_fin() and reuse the existing barrier rather than adding
   a new smp_wmb().
 - Sample SOCK_DONE before smp_rmb() in mptcp_inq_hint() and mptcp_inq(),
   so the smp_rmb() only orders SOCK_DONE read relative to the subsequent
   ack_seq/copied_seq reads.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1788171508.git.tanggeliang@kylinos.cn/

v11:
 - __mptcp_move_skb(): gate __mptcp_sync_rcv_sequence() on
   test_and_clear_bit(MPTCP_SYNC_SEQ) so the dummy mapping is only re-based
   after ack_seq = iasn.
 - a new patch to set and test SOCK_DONE.
 - use SOCK_DONE in mptcp_inq_hint() and mptcp_inq().
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1788099847.git.tanggeliang@kylinos.cn/

v10:
 - Drop "mptcp: use atomic64_t for locklessly accessed u64 fields"
   (v9 patch 1) and the squash to bpf_burst test (v9 patch 11).
   Series reduced from 11 to 9 patches.
 - Revert all atomic64_read()/atomic64_set() on msk->ack_seq back to
   plain reads and WRITE_ONCE() in patches 1-4. Remove the local u64
   ack_seq variables that only existed to hold atomic64_read() results.
 - mptcp_inq_hint: move test_bit(MPTCP_SYNC_SEQ) inside mptcp_data_lock()
   to close the race between the flag check and the ack_seq read. Revert
   atomic64_read() to plain msk->ack_seq under the lock.
 - mptcp_inq (patch 6): use READ_ONCE() for both ack_seq and copied_seq
   instead of atomic64_read(). Add comment for the FIN subtraction,
   mirroring tcp_inq(). Update commit log.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787888115.git.tanggeliang@kylinos.cn/

v9:
 - Extend atomic64_t conversion from only msk->ack_seq to all locklessly
   accessed u64 fields: write_seq, snd_nxt, snd_una, wnd_end,
   bytes_received, bytes_consumed.
 - Add mptcp_data_lock() in mptcp_inq_hint() to read ack_seq and
   copied_seq atomically.
 - Add read_copied field and drain it under mptcp_data_lock() in
   mptcp_read_complete() to fix the BH vs worker data race.
 - New patches: mptcp_inq/peek_len and sendmsg_locked proto_ops.
 - Squash to "selftests/bpf: Add bpf_burst scheduler & test".
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787827525.git.tanggeliang@kylinos.cn/

v8:
 - check MPTCP_SYNC_SEQ in mptcp_inq_hint:
       if (hint_val >= INT_MAX) {
               if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
                       return 0;
               return INT_MAX;
       }
 - rename mptcp_sock_rate_check_app_limited to
   mptcp_rate_check_app_limited.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787644449.git.tanggeliang@kylinos.cn/

v7:
 - Patch 1 is a new one to fix the existing issue Sashiko mentioned -
   using atomic64_t for msk->ack_seq.
 - Fix the map_subflow_seq setting in fallback mode in patch 4.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787537436.git.tanggeliang@kylinos.cn/

v6:
 - Memory ordering for MPTCP_SYNC_SEQ: Add smp_wmb() before set_bit() in
   subflow_set_remote_key() and smp_rmb() after test_and_clear_bit() in
   __mptcp_move_skb() and mptcp_release_cb() to prevent data races on
   weakly-ordered architectures, ensuring ack_seq updates are visible
   before the flag is set and readers see the updated sequence after
   observing the flag.
 - Lockdep nested locking: Use lock_sock_fast_nested(ssk) instead of
   lock_sock_fast(ssk) in mptcp_sock_rate_check_app_limited() to suppress
   false positive recursive locking warnings when acquiring subflow socket
   locks while holding the parent MPTCP socket lock.
 - Receive window advertisement order: Swap mptcp_rcv_space_adjust() and
   mptcp_cleanup_rbuf() in mptcp_read_complete() to expand the receive
   buffer before evaluating window updates, ensuring ACKs advertise the
   new (larger) window size instead of delaying the advertisement until
   the next cycle.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787446274.git.tanggeliang@kylinos.cn/

v5:
 - only patch 3 changed.
 - Fallback offset underflow: Initialize MPTCP sequence space to 0 in
   mptcp_propagate_state() when mp_opt == NULL, ensuring SKB's map_seq
   starts from 0 to match msk->copied_seq and prevent offset calculation
   underflow in fallback mode.
 - Stale peek_seq: Move peek_seq recomputation into the mptcp_move_skbs()
   continue path and add another recomputation after sk_wait_data(),
   ensuring peek_seq stays synchronized with msk->copied_seq updates from
   MPTCP_SYNC_SEQ processing in both paths.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787368526.git.tanggeliang@kylinos.cn/

v4:
 - mptcp_recvmsg: move the peek_seq recompute from after sk_wait_data()
   to before the mptcp_move_skbs() continue check.
   mptcp_move_skbs() -> __mptcp_move_skb() may shift the head skb's
   map_seq to the IASN frame via __mptcp_sync_rcv_sequence(); if
   'continue' then exits the loop, the recompute is skipped, and the
   next __mptcp_recvmsg_mskq() computes offset with peek_seq in the old
   frame minus map_seq in the new frame. The underflow makes the offset
   test fail and the skb is skipped (and mptcp_recv_skb() actually frees
   the TFO skb via mptcp_eat_recv_skb()).
 - Make all three MPTCP_SYNC_SEQ bit accesses atomic:
   - subflow_set_remote_key:   __set_bit            -> set_bit
   - __mptcp_move_skb:         __test_and_clear_bit -> test_and_clear_bit
   - mptcp_release_cb:         __test_and_clear_bit -> test_and_clear_bit
   The flag is set in BH under mptcp_data_lock() (msk slock held) and
   cleared in user context under lock_sock() slow path (only owned=1,
   slock not held). The non-atomic RMW on the two sides races, which can
   lose the IASN sync.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1787295147.git.tanggeliang@kylinos.cn/

v3:
 - Patch 2, handle "offset" in __mptcp_sync_rcv_sequence().
 - Patch 3, update __mptcp_move_skb() in response to Sashiko comments:

 	if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
		msk->copied_seq += mptcp_iasn(msk);

 - Patch 4, replace after64() to after() in mptcp_prune_ofo_queue(). The
   pre-existing issue raised by Sashiko regarding changing the type of
   ack_seq to atomic64_t is not addressed in this series.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1786445142.git.tanggeliang@kylinos.cn/

v2:
 - Patch 3, updated in response to Sashiko comments:

        if (unlikely(msk->rcvd_dummy_seq)) {
                msk->copied_seq += mptcp_iasn(msk);
                __mptcp_sync_rcv_sequence(sk);
                /* Release cb() would otherwise re-base copied_seq
                 * again.
                 */
                test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
        }

        /* Skip the already peeked data. */
        if (offset >= skb->len) {
                *last = skb;
                continue;
        }
 - Patch 5, replaced with Paolo's patch.
 - Patch 6, 7, new patches addressing app-limited conditions.
 - The patch "trim the duplicated skb head at receive enqueue" has been
   dropped, as it is no longer needed after Paolo updated the "mptcp:
   out-of-order queue pruning" series.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1786158416.git.tanggeliang@kylinos.cn/

v1:
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1785150300.git.tanggeliang@kylinos.cn/

The goal of this series is to reduce the differences between TCP and MPTCP
for TLS usage, in preparation for adding TLS over MPTCP support in the
future.

In previous versions [1], a struct tls_prot_ops was defined to represent
the interface differences between TCP and MPTCP, which contained the
following callbacks:

	struct sk_buff *(*recv_skb)(struct sock *sk, u32 *off);
	bool (*lock_is_held)(struct sock *sk);
	void (*read_done)(struct sock *sk, size_t len);
	u32 (*get_skb_seq)(struct sk_buff *skb);
	int (*skb_get_header)(const struct sk_buff *skb, int offset,
	                      void *to, int len);
	bool (*epollin_ready)(const struct sock *sk);
	void (*check_app_limited)(struct sock *sk);

In reality, some of these callbacks are unnecessary. This series aims to
eliminate the get_skb_seq(), skb_get_header(), and lock_is_held()
callbacks.

The first four patches come from Paolo's "mptcp: address stall under memory
pressure" series v5 [2], with only minor cleanup from my side.

They remove the CB offset field and sync the MPTCP skb CB layout with the
TCP one, so that we can obtain the TCP or MPTCP sequence number in a
unified way, e.g.:

	struct tls_skb_cb {
	    u32 seq;
	};

	#define TLS_SKB_CB(__skb) ((struct tls_skb_cb *)&((__skb)->cb[0]))

This eliminates the need for a separate get_skb_seq() callback.

Building on the removal of the CB offset field, I also added patch 5 that
trims the duplicated skb head at receive enqueue. With that in place, KTLS
can retrieve the record header via skb_copy_bits() directly, so there is no
longer any need for a dedicated MPTCP helper like mptcp_skb_get_header().
The skb_get_header() callback can thus be removed.

Patch 6 defers sk_data_ready to the worker, which avoids recursive locking
when TLS calls back into MPTCP under mptcp_data_lock(). With this change,
the lock_is_held() callback is no longer needed and can be removed.

[1]
https://patchwork.kernel.org/project/mptcp/cover/cover.1782123118.git.tanggeliang@kylinos.cn/
[2]
https://patchwork.kernel.org/project/mptcp/cover/cover.1778446731.git.pabeni@redhat.com/

Geliang Tang (2):
  mptcp: fix TFO map_seq sign extension in fastopen
  mptcp: defer read_sock cleanup to mptcp_worker

Paolo Abeni (4):
  mptcp: drop the mptcp_ooo_try_coalesce() helper
  mptcp: drop the cant_coalesce CB field
  mptcp: remove CB offset field
  mptcp: sync mptcp skb cb layout with tcp one

 net/mptcp/fastopen.c |  16 ++-
 net/mptcp/protocol.c | 262 +++++++++++++++++++++++++++----------------
 net/mptcp/protocol.h |  16 ++-
 net/mptcp/subflow.c  |  19 +++-
 4 files changed, 208 insertions(+), 105 deletions(-)

-- 
2.55.0

[PATCH mptcp-next v20 1/6] mptcp: drop the mptcp_ooo_try_coalesce() helper

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

It's used to save an additional comparison for in-order skbs, but is
also a barrier to remove CB offset. Remove the helper, let
__mptcp_try_coalesce() always perform the sequence check and remove
duplicate checks from the callers.

Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..528a72da646c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -167,7 +167,8 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 {
 	int limit = READ_ONCE(sk->sk_rcvbuf);
 
-	if (unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
+	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
+	    unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
 	    MPTCP_SKB_CB(from)->offset ||
 	    ((to->len + from->len) > (limit >> 3)) ||
 	    !skb_try_coalesce(to, from, fragstolen, delta))
@@ -200,15 +201,6 @@ static bool mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	return true;
 }
 
-static bool mptcp_ooo_try_coalesce(struct mptcp_sock *msk, struct sk_buff *to,
-				   struct sk_buff *from)
-{
-	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq)
-		return false;
-
-	return mptcp_try_coalesce((struct sock *)msk, to, from);
-}
-
 /* "inspired" by tcp_rcvbuf_grow(), main difference:
  * - mptcp does not maintain a msk-level window clamp
  * - returns true when  the receive buffer is actually updated
@@ -348,7 +340,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	/* with 2 subflows, adding at end of ooo queue is quite likely
 	 * Use of ooo_last_skb avoids the O(Log(N)) rbtree lookup.
 	 */
-	if (mptcp_ooo_try_coalesce(msk, msk->ooo_last_skb, skb)) {
+	if (mptcp_try_coalesce(sk, msk->ooo_last_skb, skb)) {
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOMERGE);
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
 		return;
@@ -394,7 +386,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
 				goto merge_right;
 			}
-		} else if (mptcp_ooo_try_coalesce(msk, skb1, skb)) {
+		} else if (mptcp_try_coalesce(sk, skb1, skb)) {
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOMERGE);
 			return;
 		}
@@ -777,8 +769,7 @@ static void __mptcp_add_backlog(struct sock *sk,
 	if (!list_empty(&msk->backlog_list))
 		tail = list_last_entry(&msk->backlog_list, struct sk_buff, list);
 
-	if (tail && MPTCP_SKB_CB(skb)->map_seq == MPTCP_SKB_CB(tail)->end_seq &&
-	    ssk == tail->sk &&
+	if (tail && ssk == tail->sk &&
 	    __mptcp_try_coalesce(sk, tail, skb, &fragstolen, &delta)) {
 		skb->truesize -= delta;
 		kfree_skb_partial(skb, fragstolen);
@@ -902,7 +893,7 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 
 		end_seq = MPTCP_SKB_CB(skb)->end_seq;
 		tail = skb_peek_tail(&sk->sk_receive_queue);
-		if (!tail || !mptcp_ooo_try_coalesce(msk, tail, skb)) {
+		if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
 			int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
 
 			/* skip overlapping data, if any */
-- 
2.55.0

[PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

Such field is used to ensure in-sequence processing in case of fastopen.
Instead let's perform synchronization of the fastopen skb sequence
when the IASN becomes available with the 3rd ack.

When the cant_coalesce field has been introduced, commit f03afb3aeb9d
("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()") noted that updating the
already queued skb for passive fastopen socket at 3rd ack time would be
difficult and race prone. The main point is that such update don't need
to be synchronously performed at 3rd ack time, but is sufficient to
perform it before the next segment is introduced into the msk.

To such extent, add __mptcp_sync_rcv_sequence() to update the TFO skb
map_seq and end_seq using the IASN stored in msk->tfo_iasn, and call it
from __mptcp_move_skb() before any new skb lands into the msk.

When the 3rd ack arrives while the msk is owned by user space, defer the
sync via MPTCP_SYNC_SEQ in cb_flags. subflow_set_remote_key() stores
tfo_iasn and either calls __mptcp_sync_rcv_sequence() directly (when the
socket is not locked) or sets MPTCP_SYNC_SEQ for deferred processing.

mptcp_move_skbs() and mptcp_release_cb() check MPTCP_SYNC_SEQ at each
iteration before spooling the backlog, using test_and_clear_bit() since
__mptcp_sync_rcv_sequence() can also be called without the socket lock
held. A rcvd_dummy_seq flag tracks whether the TFO skb still carries the
dummy mapping and needs synchronization.
---
 net/mptcp/fastopen.c |  6 ++++--
 net/mptcp/protocol.c | 33 +++++++++++++++++++++++++++++++--
 net/mptcp/protocol.h |  5 ++++-
 net/mptcp/subflow.c  |  8 ++++++++
 4 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index f717750906ff..e18b31028e7e 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -9,6 +9,7 @@
 void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
 					      struct request_sock *req)
 {
+	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
 	struct sk_buff *skb;
 	struct tcp_sock *tp;
@@ -49,15 +50,16 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	MPTCP_SKB_CB(skb)->end_seq = 0;
 	MPTCP_SKB_CB(skb)->offset = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
-	MPTCP_SKB_CB(skb)->cant_coalesce = 1;
 
 	mptcp_data_lock(sk);
 	DEBUG_NET_WARN_ON_ONCE(sock_owned_by_user_nocheck(sk));
 
+	msk = mptcp_sk(sk);
+	msk->rcvd_dummy_seq = true;
 	mptcp_borrow_fwdmem(sk, skb);
 	skb_set_owner_r(skb, sk);
 	__skb_queue_tail(&sk->sk_receive_queue, skb);
-	mptcp_sk(sk)->bytes_received += skb->len;
+	msk->bytes_received += skb->len;
 
 	sk->sk_data_ready(sk);
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 528a72da646c..f3b07b303f55 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	int limit = READ_ONCE(sk->sk_rcvbuf);
 
 	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
-	    unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
 	    MPTCP_SKB_CB(from)->offset ||
 	    ((to->len + from->len) > (limit >> 3)) ||
 	    !skb_try_coalesce(to, from, fragstolen, delta))
@@ -430,7 +429,6 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
 	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
 	MPTCP_SKB_CB(skb)->offset = offset;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
-	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
 
 	__skb_unlink(skb, &ssk->sk_receive_queue);
 
@@ -438,6 +436,24 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
 	skb_dst_drop(skb);
 }
 
+void __mptcp_sync_rcv_sequence(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sk_buff *skb;
+
+	if (likely(!msk->rcvd_dummy_seq))
+		return;
+
+	/* User space can have already received the TFO skb. */
+	msk->rcvd_dummy_seq = false;
+	skb = skb_peek_tail(&sk->sk_receive_queue);
+	if (!skb)
+		return;
+
+	MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
+	MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
+}
+
 static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 {
 	u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
@@ -446,6 +462,12 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 
 	mptcp_borrow_fwdmem(sk, skb);
 
+	/* Be sure to sync the eventual fastopen dummy mapping before any other
+	 * skb lands into the msk.
+	 */
+	if (unlikely(msk->rcvd_dummy_seq))
+		__mptcp_sync_rcv_sequence(sk);
+
 	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
 		/* in sequence */
 insert:
@@ -2375,6 +2397,8 @@ static bool mptcp_move_skbs(struct sock *sk)
 	u32 moved = 0;
 
 	mptcp_data_lock(sk);
+	if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+		__mptcp_sync_rcv_sequence(sk);
 	while (mptcp_can_spool_backlog(sk, &skbs)) {
 		mptcp_data_unlock(sk);
 		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
@@ -3680,6 +3704,8 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	msk->bytes_retrans = 0;
 	msk->rcvspace_init = 0;
 	msk->fastclosing = 0;
+	msk->tfo_iasn = 0;
+	msk->rcvd_dummy_seq = false;
 	mptcp_init_rtt_est(msk);
 
 	/* for fallback's sake */
@@ -3882,6 +3908,9 @@ static void mptcp_release_cb(struct sock *sk)
 		struct list_head join_list, skbs;
 		bool spool_bl;
 
+		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+			__mptcp_sync_rcv_sequence(sk);
+
 		spool_bl = mptcp_can_spool_backlog(sk, &skbs);
 		if (!flags && !spool_bl)
 			break;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..b4e49d719200 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -126,13 +126,13 @@
 #define MPTCP_FLUSH_JOIN_LIST	5
 #define MPTCP_SYNC_STATE	6
 #define MPTCP_SYNC_SNDBUF	7
+#define MPTCP_SYNC_SEQ		8
 
 struct mptcp_skb_cb {
 	u64 map_seq;
 	u64 end_seq;
 	u32 offset;
 	u8  has_rxtstamp;
-	u8  cant_coalesce;
 };
 
 #define MPTCP_SKB_CB(__skb)	((struct mptcp_skb_cb *)&((__skb)->cb[0]))
@@ -325,6 +325,7 @@ struct mptcp_sock {
 	u32		last_ack_recv;
 	unsigned long	timer_ival;
 	u32		token;
+	u64		tfo_iasn;
 	unsigned long	flags;
 	unsigned long	cb_flags;
 	bool		recovery;		/* closing subflow write queue reinjected */
@@ -336,6 +337,7 @@ struct mptcp_sock {
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
 	bool		allow_infinite_fallback;
+	bool		rcvd_dummy_seq;
 	u8		pending_state; /* A subflow asked to set this sk_state,
 					* protected by the msk data lock
 					*/
@@ -1193,6 +1195,7 @@ void mptcp_event_pm_listener(const struct sock *ssk,
 			     enum mptcp_event_type event);
 bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
 
+void __mptcp_sync_rcv_sequence(struct sock *sk);
 void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
 					      struct request_sock *req);
 int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..8a3ba490b577 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -515,6 +515,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 				   struct mptcp_subflow_context *subflow,
 				   const struct mptcp_options_received *mp_opt)
 {
+	struct sock *sk = (struct sock *)msk;
+
 	/* active MPC subflow will reach here multiple times:
 	 * at subflow_finish_connect() time and at 4th ack time
 	 */
@@ -533,6 +535,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 	WRITE_ONCE(msk->ack_seq, subflow->iasn);
 	WRITE_ONCE(msk->can_ack, true);
 	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
+	msk->tfo_iasn = subflow->iasn;
+
+	if (!sock_owned_by_user(sk))
+		__mptcp_sync_rcv_sequence(sk);
+	else
+		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
 }
 
 static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
-- 
2.55.0

[PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In mptcp_fastopen_subflow_synack_set_params(), the TFO skb's map_seq is
set to -skb->len to represent data before the IASN.

Since skb->len is u32, the negation -skb->len is computed in u32
arithmetic, producing a large positive value (e.g., -1420 becomes
4294965876). When assigned to map_seq (u64), this value is zero-extended
rather than sign-extended.

This causes incorrect behavior in sequence number comparisons throughout
the code. Functions like after64() and before64() may misorder TFO skbs,
and the out-of-order queue handling may misclassify TFO data.

Fix by casting to u64 before negation: -(u64)skb->len. This ensures
proper 64-bit two's complement representation and correct semantics for
all sequence number operations.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/fastopen.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index e18b31028e7e..79765cf55286 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -45,8 +45,10 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	subflow->ssn_offset += skb->len;
 	has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
 
-	/* Only the sequence delta is relevant */
-	MPTCP_SKB_CB(skb)->map_seq = -skb->len;
+	/* The TFO segment data sits before the IASN; before receiving
+	 * the remote key, IASN is assumed being 0.
+	 */
+	MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
 	MPTCP_SKB_CB(skb)->end_seq = 0;
 	MPTCP_SKB_CB(skb)->offset = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
-- 
2.55.0

[PATCH mptcp-next v20 4/6] mptcp: remove CB offset field

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

Instead, use a new msk-level field to track the bytes already consumed
inside each skb, carrying the amount of bytes already copied to
user-space, alike what TCP is already doing.

The newly introduce `copied_seq` field is always accessed under the msk
socket lock, delegating the synchronization with IASN to mptcp_move_skbs()
and the msk release CB, when the socket is owned by the user-space at
remote key reception time. Such synchronization preserves any partial
progress (copy) made on the TFO packet.

Prior to this patch, the TFO skb dummy mapping (map_seq = -(u32)skb->len,
zero-extended to u64) caused mptcp_inq_hint() to compute a wrapped
ack_seq - map_seq, so SIOCINQ and TCP_CM_INQ returned INT_MAX before the
MPC ACK.  This patch fixes that behaviour through both the -(u64)skb->len
sign fix and the new ack_seq - copied_seq hint.

Overall this simplifies a bit the __mptcp_recvmsg_mskq(), mptcp_inq_hint()
and the __mptcp_move_skb() code and will also make possible the next
patch.

In mptcp_init_skb(), map_seq is computed as mptcp_subflow_get_mapped_dsn()
minus offset. This is correct even when offset > 0 and the mapping is
out-of-order, because mptcp_subflow_get_mapped_dsn() returns the DSN for
the byte at tp->copied_seq (position 'offset' in the skb), and the
consumed prefix bytes are part of the same logical DSN range.

Initialize MPTCP sequence space to 0 in mptcp_propagate_state() when mp_opt
is NULL, ensuring SKB's map_seq starts from 0 to match msk->copied_seq and
prevent offset calculation underflow in fallback mode.  The writes are safe
without sock_owned_by_user() deferral because mptcp_propagate_state()
already holds mptcp_data_lock().

Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/fastopen.c |  2 +-
 net/mptcp/protocol.c | 93 +++++++++++++++++++++++---------------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  | 15 +++++--
 4 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 79765cf55286..78481c100e2a 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -50,7 +50,6 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	 */
 	MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
 	MPTCP_SKB_CB(skb)->end_seq = 0;
-	MPTCP_SKB_CB(skb)->offset = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
 
 	mptcp_data_lock(sk);
@@ -58,6 +57,7 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 
 	msk = mptcp_sk(sk);
 	msk->rcvd_dummy_seq = true;
+	msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
 	mptcp_borrow_fwdmem(sk, skb);
 	skb_set_owner_r(skb, sk);
 	__skb_queue_tail(&sk->sk_receive_queue, skb);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f3b07b303f55..3bd7d38ce78c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	int limit = READ_ONCE(sk->sk_rcvbuf);
 
 	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
-	    MPTCP_SKB_CB(from)->offset ||
 	    ((to->len + from->len) > (limit >> 3)) ||
 	    !skb_try_coalesce(to, from, fragstolen, delta))
 		return false;
@@ -415,8 +414,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	skb_set_owner_r(skb, sk);
 }
 
-static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
-			   int copy_len)
+static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
@@ -424,10 +422,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
 	/* the skb map_seq accounts for the skb offset:
 	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
 	 * value
+	 *
+	 * This is correct even when offset > 0 and mapping is out-of-order,
+	 * because consumed prefix bytes are part of the same logical DSN range.
 	 */
-	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
-	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
-	MPTCP_SKB_CB(skb)->offset = offset;
+	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
+				     offset;
+	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
 
 	__skb_unlink(skb, &ssk->sk_receive_queue);
@@ -451,7 +452,7 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
 		return;
 
 	MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
-	MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
+	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
 }
 
 static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
@@ -494,10 +495,6 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 	/* Partial packet */
 	if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
 		copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
-		MPTCP_SKB_CB(skb)->offset += msk->ack_seq -
-					     MPTCP_SKB_CB(skb)->map_seq;
-		MPTCP_SKB_CB(skb)->map_seq += msk->ack_seq -
-					      MPTCP_SKB_CB(skb)->map_seq;
 		goto insert;
 	}
 
@@ -855,7 +852,7 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
 		if (offset < skb->len) {
 			size_t len = skb->len - offset;
 
-			mptcp_init_skb(ssk, skb, offset, len);
+			mptcp_init_skb(ssk, skb, offset);
 
 			if (own_msk) {
 				mptcp_subflow_lend_fwdmem(subflow, skb);
@@ -918,12 +915,9 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 		if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
 			int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
 
-			/* skip overlapping data, if any */
 			pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
 				 MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
 				 delta);
-			MPTCP_SKB_CB(skb)->offset += delta;
-			MPTCP_SKB_CB(skb)->map_seq += delta;
 			__skb_queue_tail(&sk->sk_receive_queue, skb);
 		}
 		msk->bytes_received += end_seq - msk->ack_seq;
@@ -2207,33 +2201,24 @@ static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
 }
 
 static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
-				size_t len, int flags, int copied_total,
+				size_t len, int flags, u64 *seq,
 				struct scm_timestamping_internal *tss,
 				int *cmsg_flags, struct sk_buff **last)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct sk_buff *skb, *tmp;
-	int total_data_len = 0;
 	int copied = 0;
 
 	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
-		u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
+		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
 		u32 data_len = skb->len - offset;
 		u32 count;
 		int err;
 
-		if (flags & MSG_PEEK) {
-			/* skip already peeked skbs */
-			if (total_data_len + data_len <= copied_total) {
-				total_data_len += data_len;
-				*last = skb;
-				continue;
-			}
-
-			/* skip the already peeked data in the current skb */
-			delta = copied_total - total_data_len;
-			offset += delta;
-			data_len -= delta;
+		/* Skip the already peeked data. */
+		if (offset >= skb->len) {
+			*last = skb;
+			continue;
 		}
 
 		count = min_t(size_t, len - copied, data_len);
@@ -2252,14 +2237,12 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
 		}
 
 		copied += count;
+		*seq += count;
 
 		if (!(flags & MSG_PEEK)) {
 			msk->bytes_consumed += count;
-			if (count < data_len) {
-				MPTCP_SKB_CB(skb)->offset += count;
-				MPTCP_SKB_CB(skb)->map_seq += count;
+			if (count < data_len)
 				break;
-			}
 
 			mptcp_eat_recv_skb(sk, skb);
 		} else {
@@ -2397,9 +2380,15 @@ static bool mptcp_move_skbs(struct sock *sk)
 	u32 moved = 0;
 
 	mptcp_data_lock(sk);
-	if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
-		__mptcp_sync_rcv_sequence(sk);
-	while (mptcp_can_spool_backlog(sk, &skbs)) {
+	while (1) {
+		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+			msk->copied_seq += msk->tfo_iasn;
+			__mptcp_sync_rcv_sequence(sk);
+		}
+
+		if (!mptcp_can_spool_backlog(sk, &skbs))
+			break;
+
 		mptcp_data_unlock(sk);
 		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
 
@@ -2441,6 +2430,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct scm_timestamping_internal tss;
 	int copied = 0, cmsg_flags = 0;
+	u64 peek_seq, *seq;
 	int target;
 	long timeo;
 
@@ -2459,6 +2449,11 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 
 	len = min_t(size_t, len, INT_MAX);
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
+	seq = &msk->copied_seq;
+	if (flags & MSG_PEEK) {
+		peek_seq = msk->copied_seq;
+		seq = &peek_seq;
+	}
 
 	if (unlikely(msk->recvmsg_inq))
 		cmsg_flags = MPTCP_CMSG_INQ;
@@ -2468,7 +2463,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		int err, bytes_read;
 
 		bytes_read = __mptcp_recvmsg_mskq(sk, msg, len - copied, flags,
-						  copied, &tss, &cmsg_flags,
+						  seq, &tss, &cmsg_flags,
 						  &last);
 		if (unlikely(bytes_read < 0)) {
 			if (!copied)
@@ -2478,8 +2473,11 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 
 		copied += bytes_read;
 
-		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
+		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk)) {
+			if (flags & MSG_PEEK)
+				peek_seq = msk->copied_seq + copied;
 			continue;
+		}
 
 		/* only the MPTCP socket status is relevant here. The exit
 		 * conditions mirror closely tcp_recvmsg()
@@ -2522,6 +2520,10 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 			copied = copied ? : err;
 			goto out_err;
 		}
+
+		/* Recompute peek offset after eventual seq resync. */
+		if (flags & MSG_PEEK)
+			peek_seq = msk->copied_seq + copied;
 	}
 
 	mptcp_cleanup_rbuf(msk, copied);
@@ -3698,6 +3700,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	mptcp_pm_data_reset(msk);
 	mptcp_ca_reset(sk);
 	msk->bytes_consumed = 0;
+	msk->copied_seq = 0;
 	msk->bytes_acked = 0;
 	msk->bytes_received = 0;
 	msk->bytes_sent = 0;
@@ -3908,8 +3911,10 @@ static void mptcp_release_cb(struct sock *sk)
 		struct list_head join_list, skbs;
 		bool spool_bl;
 
-		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+			msk->copied_seq += msk->tfo_iasn;
 			__mptcp_sync_rcv_sequence(sk);
+		}
 
 		spool_bl = mptcp_can_spool_backlog(sk, &skbs);
 		if (!flags && !spool_bl)
@@ -4630,7 +4635,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 		mptcp_move_skbs(sk);
 
 	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
-		offset = MPTCP_SKB_CB(skb)->offset;
+		offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
 		if (offset < skb->len) {
 			*off = offset;
 			return skb;
@@ -4672,11 +4677,9 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 		copied += count;
 
 		msk->bytes_consumed += count;
-		if (count < data_len) {
-			MPTCP_SKB_CB(skb)->offset += count;
-			MPTCP_SKB_CB(skb)->map_seq += count;
+		msk->copied_seq += count;
+		if (count < data_len)
 			break;
-		}
 
 		mptcp_eat_recv_skb(sk, skb);
 		if (!desc->count)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index b4e49d719200..7878e6ac6c0b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -131,7 +131,6 @@
 struct mptcp_skb_cb {
 	u64 map_seq;
 	u64 end_seq;
-	u32 offset;
 	u8  has_rxtstamp;
 };
 
@@ -311,6 +310,7 @@ struct mptcp_sock {
 	u64		rcv_data_fin_seq;
 	u64		bytes_retrans;
 	u64		bytes_consumed;
+	u64		copied_seq;
 	int		snd_burst;
 	int		old_wspace;
 	u64		recovery_snd_nxt;	/* in recovery mode accept up to this seq;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8a3ba490b577..71c3b148cf04 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -537,10 +537,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
 	msk->tfo_iasn = subflow->iasn;
 
-	if (!sock_owned_by_user(sk))
+	if (!sock_owned_by_user(sk)) {
+		msk->copied_seq += subflow->iasn;
 		__mptcp_sync_rcv_sequence(sk);
-	else
+	} else {
 		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
+	}
 }
 
 static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
@@ -557,6 +559,13 @@ static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
 		WRITE_ONCE(msk->snd_una, subflow->idsn + 1);
 		WRITE_ONCE(msk->wnd_end, subflow->idsn + 1 + tcp_sk(ssk)->snd_wnd);
 		subflow_set_remote_key(msk, subflow, mp_opt);
+	} else {
+		/* Fallback (no MPTCP options at SYN-ACK) */
+		subflow->map_seq = 0;
+		subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq -
+					   subflow->ssn_offset;
+		WRITE_ONCE(msk->ack_seq, 0);
+		msk->copied_seq = 0;
 	}
 
 	if (!sock_owned_by_user(sk)) {
@@ -1493,7 +1502,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;
 	subflow->map_data_len = skb->len;
-	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
+	subflow->map_subflow_seq = TCP_SKB_CB(skb)->seq - subflow->ssn_offset;
 	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
 					      subflow->iasn +
 					      TCP_SKB_CB(skb)->seq -
-- 
2.55.0

[PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

The MPTCP protocol uses a significantly different CB layout WRT TCP, as it
includes different information and use 64 bits for the sequence numbers.

As the msk-level rcvbuf buffer size is limited by the core socket code the
INT_MAX; after validating the incoming skb vs the current receive window,
we can safely use 32 bits for MPTCP-level sequence number. This allow
updating the MPTCP CB layout so that fields with a corresponding TCP-level
data use the same area inside the CB itself.

The 32-bit sequence numbers are safe because all SKBs in the receive queue
and OFO queue have been validated against the receive window (max INT_MAX),
so their sequence numbers are guaranteed to be within 2^31 of the current
ack_seq or copied_seq. This ensures that 32-bit arithmetic (comparisons
and subtractions) produces correct results even when 64-bit values cross
the 2^32 boundary, as the relative distance remains small.

Add build time check to ensure the latter invariant.

Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/fastopen.c |  6 ++-
 net/mptcp/protocol.c | 94 +++++++++++++++++++++++++++-----------------
 net/mptcp/protocol.h |  7 +++-
 3 files changed, 68 insertions(+), 39 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 78481c100e2a..bd52e9528b29 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -48,8 +48,10 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	/* The TFO segment data sits before the IASN; before receiving
 	 * the remote key, IASN is assumed being 0.
 	 */
-	MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
+	MPTCP_SKB_CB(skb)->map_seq64 = -(u64)skb->len;
+	MPTCP_SKB_CB(skb)->map_seq = MPTCP_SKB_CB(skb)->map_seq64;
 	MPTCP_SKB_CB(skb)->end_seq = 0;
+	MPTCP_SKB_CB(skb)->flags = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
 
 	mptcp_data_lock(sk);
@@ -57,7 +59,7 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 
 	msk = mptcp_sk(sk);
 	msk->rcvd_dummy_seq = true;
-	msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
+	msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq64;
 	mptcp_borrow_fwdmem(sk, skb);
 	skb_set_owner_r(skb, sk);
 	__skb_queue_tail(&sk->sk_receive_queue, skb);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3bd7d38ce78c..3e10b06b7772 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,11 +168,13 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	int limit = READ_ONCE(sk->sk_rcvbuf);
 
 	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
+	    MPTCP_SKB_CB(from)->map_seq64 != MPTCP_SKB_CB(to)->map_seq64 +
+					     to->len ||
 	    ((to->len + from->len) > (limit >> 3)) ||
 	    !skb_try_coalesce(to, from, fragstolen, delta))
 		return false;
 
-	pr_debug("colesced seq %llx into %llx new len %d new end seq %llx\n",
+	pr_debug("colesced seq %x into %x new len %d new end seq %x\n",
 		 MPTCP_SKB_CB(from)->map_seq, MPTCP_SKB_CB(to)->map_seq,
 		 to->len, MPTCP_SKB_CB(from)->end_seq);
 	MPTCP_SKB_CB(to)->end_seq = MPTCP_SKB_CB(from)->end_seq;
@@ -254,8 +256,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
 		struct sk_buff *skb = rb_to_skb(node);
 
 		/* Stop pruning if the incoming skb would land in OoO tail. */
-		if (after64(MPTCP_SKB_CB(in_skb)->map_seq,
-			    MPTCP_SKB_CB(skb)->map_seq))
+		if (after(MPTCP_SKB_CB(in_skb)->map_seq,
+			  MPTCP_SKB_CB(skb)->map_seq))
 			break;
 
 		pruned = true;
@@ -301,16 +303,23 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 {
 	struct sock *sk = (struct sock *)msk;
 	struct rb_node **p, *parent;
-	u64 seq, end_seq, max_seq;
+	u64 end_seq, max_seq;
 	struct sk_buff *skb1;
+	u32 seq;
 
 	seq = MPTCP_SKB_CB(skb)->map_seq;
-	end_seq = MPTCP_SKB_CB(skb)->end_seq;
+	end_seq = MPTCP_SKB_CB(skb)->map_seq64 + skb->len;
 	max_seq = atomic64_read(&msk->rcv_wnd_sent);
+	if (after64(READ_ONCE(msk->ack_seq), max_seq))
+		max_seq = READ_ONCE(msk->ack_seq);
 
-	pr_debug("msk=%p seq=%llx limit=%llx empty=%d\n", msk, seq, max_seq,
+	pr_debug("msk=%p seq=%x limit=%llx empty=%d\n", msk, seq, max_seq,
 		 RB_EMPTY_ROOT(&msk->out_of_order_queue));
-	if (after64(end_seq, max_seq)) {
+	/* Use the full sequence space to perform the admission checks, to
+	 * protect vs possible wrap-arounds.
+	 */
+	if (after64(MPTCP_SKB_CB(skb)->map_seq64, max_seq) ||
+	    after64(end_seq, max_seq)) {
 		/* out of window */
 		mptcp_drop(sk, skb);
 		pr_debug("oow by %lld, rcv_wnd_sent %llu\n",
@@ -345,7 +354,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	}
 
 	/* Can avoid an rbtree lookup if we are adding skb after ooo_last_skb */
-	if (!before64(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
+	if (!before(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
 		parent = &msk->ooo_last_skb->rbnode;
 		p = &parent->rb_right;
@@ -357,18 +366,18 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	while (*p) {
 		parent = *p;
 		skb1 = rb_to_skb(parent);
-		if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+		if (before(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
 			p = &parent->rb_left;
 			continue;
 		}
-		if (before64(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
-			if (!after64(end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+		if (before(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+			if (!after((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
 				/* All the bits are present. Drop. */
 				mptcp_drop(sk, skb);
 				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
 				return;
 			}
-			if (after64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+			if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
 				/* partial overlap:
 				 *     |     skb      |
 				 *  |     skb1    |
@@ -399,7 +408,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 merge_right:
 	/* Remove other segments covered by skb. */
 	while ((skb1 = skb_rb_next(skb)) != NULL) {
-		if (before64(end_seq, MPTCP_SKB_CB(skb1)->end_seq))
+		if (before((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq))
 			break;
 		rb_erase(&skb1->rbnode, &msk->out_of_order_queue);
 		mptcp_drop(sk, skb1);
@@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
 	 *
 	 * This is correct even when offset > 0 and mapping is out-of-order,
 	 * because consumed prefix bytes are part of the same logical DSN range.
+	 *
+	 * Note that end seq number is only available in 32bits format.
 	 */
-	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
-				     offset;
+	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
+				       offset;
+	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
 	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
+	MPTCP_SKB_CB(skb)->flags = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
 
 	__skb_unlink(skb, &ssk->sk_receive_queue);
@@ -451,13 +464,14 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
 	if (!skb)
 		return;
 
-	MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
+	MPTCP_SKB_CB(skb)->map_seq64 = msk->tfo_iasn - skb->len;
+	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
 	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
 }
 
 static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 {
-	u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
+	u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct sk_buff *tail;
 
@@ -469,7 +483,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 	if (unlikely(msk->rcvd_dummy_seq))
 		__mptcp_sync_rcv_sequence(sk);
 
-	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
+	if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
 		/* in sequence */
 insert:
 		if (!mptcp_try_rmem_schedule(sk, skb)) {
@@ -487,14 +501,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 		skb_set_owner_r(skb, sk);
 		__skb_queue_tail(&sk->sk_receive_queue, skb);
 		return true;
-	} else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
+	} else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
 		mptcp_data_queue_ofo(msk, skb);
 		return false;
 	}
 
 	/* Partial packet */
-	if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
-		copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
+	if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
+		copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
 		goto insert;
 	}
 
@@ -889,39 +903,37 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
 	struct sk_buff *skb, *tail;
+	u32 seq_delta, ack_seq;
 	bool moved = false;
 	struct rb_node *p;
-	u64 end_seq;
 
 	p = rb_first(&msk->out_of_order_queue);
 	pr_debug("msk=%p empty=%d\n", msk, RB_EMPTY_ROOT(&msk->out_of_order_queue));
 	while (p) {
+		ack_seq = msk->ack_seq;
 		skb = rb_to_skb(p);
-		if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq))
+		if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
 			break;
 
 		p = rb_next(p);
 		rb_erase(&skb->rbnode, &msk->out_of_order_queue);
 
-		if (unlikely(!after64(MPTCP_SKB_CB(skb)->end_seq,
-				      msk->ack_seq))) {
+		if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
 			mptcp_drop(sk, skb);
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
 			continue;
 		}
 
-		end_seq = MPTCP_SKB_CB(skb)->end_seq;
+		seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;
 		tail = skb_peek_tail(&sk->sk_receive_queue);
 		if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
-			int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
-
-			pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
-				 MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
-				 delta);
+			pr_debug("uncoalesced seq=%x ack seq=%x delta=%d\n",
+				 MPTCP_SKB_CB(skb)->map_seq, ack_seq,
+				 ack_seq - MPTCP_SKB_CB(skb)->map_seq);
 			__skb_queue_tail(&sk->sk_receive_queue, skb);
 		}
-		msk->bytes_received += end_seq - msk->ack_seq;
-		WRITE_ONCE(msk->ack_seq, end_seq);
+		msk->bytes_received += seq_delta;
+		WRITE_ONCE(msk->ack_seq, msk->ack_seq + seq_delta);
 		moved = true;
 	}
 	return moved;
@@ -2210,7 +2222,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
 	int copied = 0;
 
 	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
-		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
+		u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;
 		u32 data_len = skb->len - offset;
 		u32 count;
 		int err;
@@ -4635,7 +4647,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 		mptcp_move_skbs(sk);
 
 	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
-		offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
+		offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
 		if (offset < skb->len) {
 			*off = offset;
 			return skb;
@@ -4885,11 +4897,23 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
 	return work_done;
 }
 
+#define CHK_CB_FIELD(mptcp_field, tcp_field)	\
+	({					\
+		BUILD_BUG_ON(offsetof(struct mptcp_skb_cb, mptcp_field) !=    \
+			     offsetof(struct tcp_skb_cb, tcp_field));	      \
+		BUILD_BUG_ON(offsetofend(struct mptcp_skb_cb, mptcp_field) != \
+			     offsetofend(struct tcp_skb_cb, tcp_field));      \
+	})
+
 void __init mptcp_proto_init(void)
 {
 	struct mptcp_delegated_action *delegated;
 	int cpu;
 
+	CHK_CB_FIELD(map_seq, seq);
+	CHK_CB_FIELD(end_seq, end_seq);
+	CHK_CB_FIELD(flags, tcp_flags);
+
 	mptcp_prot.h.hashinfo = tcp_prot.h.hashinfo;
 
 	if (percpu_counter_init(&mptcp_sockets_allocated, 0, GFP_KERNEL))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7878e6ac6c0b..fa54f70ab501 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -129,9 +129,12 @@
 #define MPTCP_SYNC_SEQ		8
 
 struct mptcp_skb_cb {
-	u64 map_seq;
-	u64 end_seq;
+	u32 map_seq;
+	u32 end_seq;
+	u32 unused;
+	u16 flags;
 	u8  has_rxtstamp;
+	u64 map_seq64;
 };
 
 #define MPTCP_SKB_CB(__skb)	((struct mptcp_skb_cb *)&((__skb)->cb[0]))
-- 
2.55.0

[PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

When MPTCP carries TLS, __mptcp_read_sock() may be reached from BH
context (via tls_strp_read_copyin() -> sk->sk_read_sock()), where
calling mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() is not safe
as both expect process context.

Conditionally defer cleanup to mptcp_worker(): when the caller doesn't
own the socket lock (BH context), accumulate consumed bytes in
msk->read_copied and signal the worker through MPTCP_WORK_READ_COMPLETE.
When the caller owns the lock (process context), perform inline cleanup
to avoid delaying window updates and autotuning.

Replace msk_owned_by_me() assertion with DEBUG_NET_WARN_ON_ONCE using
lockdep_sock_is_held() to verify proper locking for both process and
BH contexts.

Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()) and drop the redundant mptcp_recv_skb() call after
the main loop.

Initialize msk->read_copied in __mptcp_init_sock() and reset it in
mptcp_disconnect().

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++--------
 net/mptcp/protocol.h |  2 ++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3e10b06b7772..25f0d10373d2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3156,6 +3156,21 @@ static void mptcp_backlog_purge(struct sock *sk)
 	sk_mem_reclaim(sk);
 }
 
+static void mptcp_read_complete(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	int read_copied;
+
+	mptcp_data_lock(sk);
+	read_copied = msk->read_copied;
+	msk->read_copied = 0;
+	mptcp_data_unlock(sk);
+
+	mptcp_rcv_space_adjust(msk, read_copied);
+	if (read_copied > 0)
+		mptcp_cleanup_rbuf(msk, read_copied);
+}
+
 static void mptcp_do_fastclose(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow, *tmp;
@@ -3231,6 +3246,9 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
+	if (test_and_clear_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags))
+		mptcp_read_complete(sk);
+
 	fail_tout = msk->first ? READ_ONCE(mptcp_subflow_ctx(msk->first)->fail_tout) : 0;
 	if (fail_tout && time_after(jiffies, fail_tout))
 		mptcp_mp_fail_no_response(msk);
@@ -3274,6 +3292,7 @@ static void __mptcp_init_sock(struct sock *sk)
 	msk->last_data_sent = tcp_jiffies32;
 	msk->last_data_recv = tcp_jiffies32;
 	msk->last_ack_recv = tcp_jiffies32;
+	msk->read_copied = 0;
 
 	mptcp_pm_data_init(msk);
 	spin_lock_init(&msk->fallback_lock);
@@ -3721,6 +3740,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	msk->fastclosing = 0;
 	msk->tfo_iasn = 0;
 	msk->rcvd_dummy_seq = false;
+	msk->read_copied = 0;
 	mptcp_init_rtt_est(msk);
 
 	/* for fallback's sake */
@@ -4643,9 +4663,6 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 	struct sk_buff *skb;
 	u32 offset;
 
-	if (!list_empty(&msk->backlog_list))
-		mptcp_move_skbs(sk);
-
 	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
 		offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
 		if (offset < skb->len) {
@@ -4660,6 +4677,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 /*
  * Note:
  *	- It is assumed that the socket was locked by the caller.
+ *	- Can be invoked in BH scope.
  */
 static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 			     sk_read_actor_t recv_actor, bool noack)
@@ -4669,7 +4687,7 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 	int copied = 0;
 	u32 offset;
 
-	msk_owned_by_me(msk);
+	DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk));
 
 	if (sk->sk_state == TCP_LISTEN)
 		return -ENOTCONN;
@@ -4701,11 +4719,18 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 	if (noack)
 		goto out;
 
-	mptcp_rcv_space_adjust(msk, copied);
-
 	if (copied > 0) {
-		mptcp_recv_skb(sk, &offset);
-		mptcp_cleanup_rbuf(msk, copied);
+		if (sock_owned_by_user(sk)) {
+			/* Process context: inline cleanup */
+			mptcp_rcv_space_adjust(msk, copied);
+			mptcp_recv_skb(sk, &offset);
+			mptcp_cleanup_rbuf(msk, copied);
+		} else {
+			/* BH context: defer to worker */
+			msk->read_copied += copied;
+			set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
+			mptcp_schedule_work(sk);
+		}
 	}
 out:
 	return copied;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index fa54f70ab501..3580cf66f303 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -117,6 +117,7 @@
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
 #define MPTCP_RTX_ENABLED	4
+#define MPTCP_WORK_READ_COMPLETE 5
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
@@ -326,6 +327,7 @@ struct mptcp_sock {
 	u32		last_data_sent;
 	u32		last_data_recv;
 	u32		last_ack_recv;
+	int		read_copied;
 	unsigned long	timer_ival;
 	u32		token;
 	u64		tfo_iasn;
-- 
2.55.0

Re: [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is deferred
--

commit 198606f4de002c65c8846681be2fa1a33a1379e5
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: drop the cant_coalesce CB field

This commit removes the cant_coalesce field from struct mptcp_skb_cb and
synchronizes the passive Fast Open skb sequence numbers with the peer's
initial ACK sequence number once the 3rd ACK arrives. When the socket is
owned by user space at 3rd ACK arrival, synchronization is deferred via
the MPTCP_SYNC_SEQ flag in cb_flags.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..8a3ba490b5772 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -515,6 +515,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  				   struct mptcp_subflow_context *subflow,
>  				   const struct mptcp_options_received *mp_opt)
>  {
> +	struct sock *sk = (struct sock *)msk;
> +
>  	/* active MPC subflow will reach here multiple times:
>  	 * at subflow_finish_connect() time and at 4th ack time
>  	 */
> @@ -533,6 +535,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
> +	msk->tfo_iasn = subflow->iasn;
> +
> +	if (!sock_owned_by_user(sk))
> +		__mptcp_sync_rcv_sequence(sk);
> +	else
> +		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);

[Severity: Medium]
As reported by Sashiko on v15 [1], when the 3rd ACK arrives on a passive Fast
Open socket while user space holds the socket lock, subflow_set_remote_key()
immediately updates msk->ack_seq to subflow->iasn while deferring
__mptcp_sync_rcv_sequence() via MPTCP_SYNC_SEQ.

Until MPTCP_SYNC_SEQ is processed, the TFO skb in sk->sk_receive_queue still
carries its dummy sequence mapping (MPTCP_SKB_CB(skb)->map_seq = -skb->len
plus any consumed offset). If mptcp_inq_hint() is called under lock_sock(sk)
during that window, it subtracts the dummy map_seq from the newly updated
msk->ack_seq:

net/mptcp/protocol.c:mptcp_inq_hint() {
	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}
}

This can happen before release_sock(sk) runs mptcp_release_cb() in three
paths:

1. mptcp_recvmsg() -> __mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust()
   calls mptcp_inq_hint(sk) before mptcp_move_skbs(sk) is invoked, which can
   underflow copied -= mptcp_inq_hint(sk).

2. mptcp_recvmsg() skips mptcp_move_skbs(sk) when msk->backlog_list is empty
   (for example when the 3rd ACK carries no payload) and then calls
   mptcp_inq_hint(sk) for TCP_CM_INQ:

net/mptcp/protocol.c:mptcp_recvmsg() {
		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
			continue;
		...
		if (cmsg_flags & MPTCP_CMSG_INQ) {
			unsigned int inq = mptcp_inq_hint(sk);

			put_cmsg(msg, SOL_TCP, TCP_CM_INQ, sizeof(inq), &inq);
		}
}

3. In mptcp_ioctl() for SIOCINQ, if the 3rd ACK arrives in BH after
   mptcp_move_skbs(sk) drops mptcp_data_lock(sk) and before
   mptcp_inq_hint(sk) reads msk->ack_seq:

net/mptcp/protocol.c:mptcp_ioctl() {
		lock_sock(sk);
		if (mptcp_move_skbs(sk))
			mptcp_cleanup_rbuf(msk, 0);
		*karg = mptcp_inq_hint(sk);
		release_sock(sk);
}

Could this cause mptcp_inq_hint() to return INT_MAX (or a bogus value derived
from subflow->iasn) to user space for TCP_CM_INQ and SIOCINQ, and corrupt the
receive buffer space calculation in mptcp_rcv_space_adjust()?

[1] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=2

Re: [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: inaccurate commit message on before64()/after64() and OoO queue
--

commit c0adf4769c9ae204c304e167fe42012e5a15d3c6
Author: Geliang Tang <geliang@kernel.org>

mptcp: fix TFO map_seq sign extension in fastopen

This commit casts skb->len to u64 before negating it when initializing the
TFO skb's dummy map_seq in mptcp_fastopen_subflow_synack_set_params(), so
that it is properly sign-extended in 64-bit arithmetic.

>     mptcp: fix TFO map_seq sign extension in fastopen
>     
>     In mptcp_fastopen_subflow_synack_set_params(), the TFO skb's map_seq is
>     set to -skb->len to represent data before the IASN.
>     
>     Since skb->len is u32, the negation -skb->len is computed in u32
>     arithmetic, producing a large positive value (e.g., -1420 becomes
>     4294965876). When assigned to map_seq (u64), this value is zero-extended
>     rather than sign-extended.
>     
>     This causes incorrect behavior in sequence number comparisons throughout
>     the code. Functions like after64() and before64() may misorder TFO skbs,
>     and the out-of-order queue handling may misclassify TFO data.
>     
>     Fix by casting to u64 before negation: -(u64)skb->len. This ensures
>     proper 64-bit two's complement representation and correct semantics for
>     all sequence number operations.
>     
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Medium]
Is the commit message description of the impact on after64(), before64(), and
the out-of-order queue accurate?

In mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c, the
TFO skb is queued directly to the tail of the in-order receive queue:

	MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
	MPTCP_SKB_CB(skb)->end_seq = 0;
	MPTCP_SKB_CB(skb)->offset = 0;
	...
	__skb_queue_tail(&sk->sk_receive_queue, skb);

Because it goes directly onto sk->sk_receive_queue, it never passes through
__mptcp_move_skb(), mptcp_data_queue_ofo(), or msk->out_of_order_queue, and
its dummy map_seq is not compared with before64() or after64(). When
subsequent skbs arrive in __mptcp_move_skb(), __mptcp_sync_rcv_sequence()
updates the TFO skb's dummy map_seq and end_seq using msk->tfo_iasn before
any comparison or coalescing takes place, and __mptcp_try_coalesce() checks
MPTCP_SKB_CB(to)->end_seq rather than map_seq.

Instead, the missing sign extension affects mptcp_inq_hint() in
net/mptcp/protocol.c before the 3rd ACK arrives (while msk->ack_seq is 0):

	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}

Without the cast to u64, 0ULL - MPTCP_SKB_CB(skb)->map_seq underflows and
clamps to INT_MAX for SIOCINQ, TCP_CM_INQ, and mptcp_rcv_space_adjust(). It
is also needed by the subsequent commit 698eb6e05455 ("mptcp: remove CB
offset field"), which initializes msk->copied_seq from
MPTCP_SKB_CB(skb)->map_seq and relies on 64-bit two's complement wrapping
when adding msk->tfo_iasn.

Could the commit message be updated to describe the actual callers affected
by the unextended map_seq value?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=3

Re: [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: stale map_seq subtraction in mptcp_inq_hint() after removing CB offset
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero subflow offset
--

commit 698eb6e054556fc8ba45c463c10e26a894f29975
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit removes the offset field from struct mptcp_skb_cb and introduces
copied_seq in struct mptcp_sock to track consumed bytes across receive skbs.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f3b07b303f555..3bd7d38ce78c4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
>  	int limit = READ_ONCE(sk->sk_rcvbuf);
>  
>  	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
> -	    MPTCP_SKB_CB(from)->offset ||
>  	    ((to->len + from->len) > (limit >> 3)) ||
>  	    !skb_try_coalesce(to, from, fragstolen, delta))
>  		return false;
> @@ -415,8 +414,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	skb_set_owner_r(skb, sk);
>  }
>  
> -static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> -			   int copy_len)
> +static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
> @@ -424,10 +422,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	/* the skb map_seq accounts for the skb offset:
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
>  	 * value
> +	 *
> +	 * This is correct even when offset > 0 and mapping is out-of-order,
> +	 * because consumed prefix bytes are part of the same logical DSN range.
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> -	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> +				     offset;
> +	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;

[Severity: High]
As reported by Sashiko on v19 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() corrupt the receive
stream when a subflow skb has a non-zero offset at the start of a new DSS
mapping?

In __mptcp_move_skbs_from_subflow(), the head skb on ssk->sk_receive_queue can
have offset = tp->copied_seq - TCP_SKB_CB(skb)->seq > 0 while starting a new
DSS mapping M2 at tp->copied_seq.

For example, in subflow_check_data_avail() -> get_mapping_status(), a single
TCP skb can carry the tail of an earlier mapping M1 in skb->data[0..offset-1]
and a new mapping M2 in its SKB_EXT_MPTCP extension starting at offset:

net/mptcp/subflow.c:get_mapping_status() {
	...
		if (skb_is_fully_mapped(ssk, skb)) {
			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSNOMATCH);
			return MAPPING_INVALID;
		}

		/* will validate the next map after consuming the current one */
		goto validate_csum;
	...
}

When the remaining bytes of M1 have already been acknowledged at the MPTCP
level, mptcp_subflow_discard_data() advances tp->copied_seq by offset without
eating the skb and resets subflow->map_valid = 0. On the next loop iteration
in subflow_check_data_avail(), get_mapping_status() installs M2 starting at
tp->copied_seq (offset bytes into the skb). The same situation also occurs
when tcp_data_queue() or tcp_ofo_queue() queues a partially overlapping TCP
segment whose first offset bytes overlap the end of M1 and whose remaining
bytes start M2 at tp->copied_seq.

Because MPTCP stripes data across multiple subflows, M1 and M2 are contiguous
only in subflow sequence space, not in data sequence space when intervening
DSNs [mapped_dsn - offset, mapped_dsn) were sent on another subflow. Setting
MPTCP_SKB_CB(skb)->map_seq = mapped_dsn - offset assigns the DSN range
[mapped_dsn - offset, mapped_dsn) to the stale M1 prefix bytes in
skb->data[0..offset-1]:

1. When M2 is out-of-order at the MPTCP level (mapped_dsn > msk->ack_seq)
   because [msk->ack_seq, mapped_dsn) is in flight on another subflow, and
   mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb() sees
   MPTCP_SKB_CB(skb)->map_seq <= msk->ack_seq. It treats the skb as
   in-sequence (or partial in-sequence), advances msk->ack_seq across the
   unreceived gap [msk->ack_seq, mapped_dsn), delivers the stale M1 prefix
   bytes from skb->data[0..offset-1] to user space via __mptcp_recvmsg_mskq()
   and mptcp_recv_skb(), and drops the genuine data arriving on the other
   subflow as MPTCP_MIB_DUPDATA.

2. Because __mptcp_try_coalesce() no longer checks MPTCP_SKB_CB(from)->offset,
   whenever mapped_dsn - offset equals MPTCP_SKB_CB(to)->end_seq in
   __mptcp_add_backlog(), __mptcp_move_skb(), mptcp_data_queue_ofo(), or
   __mptcp_ofo_queue(), skb_try_coalesce() appends all from->len bytes
   starting at byte 0 (including the stale M1 prefix in from->data[0..offset-1])
   onto to.

3. When mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
   skb into msk->out_of_order_queue under the falsified start DSN
   mapped_dsn - offset, where it can coalesce with or replace legitimate
   out-of-order segments covering [mapped_dsn - offset, mapped_dsn) and be
   dequeued prematurely by __mptcp_ofo_queue().

[ ... ]

> @@ -2252,14 +2237,12 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>  		}
>  
>  		copied += count;
> +		*seq += count;
>  
>  		if (!(flags & MSG_PEEK)) {
>  			msk->bytes_consumed += count;
> -			if (count < data_len) {
> -				MPTCP_SKB_CB(skb)->offset += count;
> -				MPTCP_SKB_CB(skb)->map_seq += count;
> +			if (count < data_len)
>  				break;
> -			}
>  
>  			mptcp_eat_recv_skb(sk, skb);
>  		} else {

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8a3ba490b5772..71c3b148cf04a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -537,10 +537,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
>  	msk->tfo_iasn = subflow->iasn;
>  
> -	if (!sock_owned_by_user(sk))
> +	if (!sock_owned_by_user(sk)) {
> +		msk->copied_seq += subflow->iasn;
>  		__mptcp_sync_rcv_sequence(sk);
> -	else
> +	} else {
>  		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
> +	}
>  }

[Severity: High]
As reported by Sashiko on v15 [2], does mptcp_inq_hint() still compute the
unread byte count using MPTCP_SKB_CB(skb)->map_seq instead of msk->copied_seq?

The commit message notes that this change fixes and simplifies
mptcp_inq_hint() via the new ack_seq - copied_seq hint, but mptcp_inq_hint()
in net/mptcp/protocol.c still reads:

net/mptcp/protocol.c:mptcp_inq_hint() {
	const struct mptcp_sock *msk = mptcp_sk(sk);
	const struct sk_buff *skb;

	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}
	...
}

Because __mptcp_recvmsg_mskq(), __mptcp_read_sock(), __mptcp_move_skb(), and
__mptcp_ofo_queue() no longer advance MPTCP_SKB_CB(skb)->map_seq as bytes are
consumed or overlapped, and mptcp_init_skb() subtracts offset from map_seq,
MPTCP_SKB_CB(skb)->map_seq stays at the start of the head skb while
msk->copied_seq advances. Whenever the head skb in sk->sk_receive_queue is
partially read (count < data_len) or has a non-zero offset, mptcp_inq_hint()
over-counts unread bytes by msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq for
SIOCINQ in mptcp_ioctl(), TCP_CM_INQ in mptcp_recvmsg(), and receive buffer
auto-tuning in mptcp_rcv_space_adjust().

In addition, on a passive TFO socket when the peer's 3rd ACK arrives while
sock_owned_by_user(sk) is true, subflow_set_remote_key() updates msk->ack_seq
to subflow->iasn immediately while deferring msk->copied_seq += msk->tfo_iasn
and __mptcp_sync_rcv_sequence(sk) via MPTCP_SYNC_SEQ. If mptcp_inq_hint() runs
before MPTCP_SYNC_SEQ is processed (for example in __mptcp_recvmsg_mskq() ->
mptcp_rcv_space_adjust(), at out_err in mptcp_recvmsg() when msk->backlog_list
is empty, in __mptcp_read_sock() -> mptcp_rcv_space_adjust(), or in
mptcp_ioctl() if the 3rd ACK arrives after mptcp_move_skbs()), it subtracts
the pre-IASN sequence from the post-IASN msk->ack_seq and returns INT_MAX.

Furthermore, a later patch in this series (commit 0495d49fc957, "mptcp: sync
mptcp skb cb layout with tcp one") narrows MPTCP_SKB_CB(skb)->map_seq from u64
to u32, which will cause READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq
to return INT_MAX whenever the upper 32 bits of msk->ack_seq are non-zero.

Should mptcp_inq_hint() use msk->copied_seq (while accounting for any pending
MPTCP_SYNC_SEQ adjustment) instead of MPTCP_SKB_CB(skb)->map_seq?

[1] https://lore.kernel.org/r/1dac839fffd26114e35382b256fdee9923c0949b.1793361629.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=4

Re: [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Script error! ❓
- KVM Validation: normal (only selftest_mptcp_join): Script error! ❓
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Script error! ❓
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Unstable: 0 failed test(s):  ⚠️ 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/41609865581

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8b0ebaec7c2e
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1200005


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: subtracting offset in mptcp_init_skb() corrupts out-of-order data
- [High] mptcp: 64-bit ack_seq minus 32-bit map_seq overflow in mptcp_inq_hint()
--

commit 0495d49fc9571bbbbcc61fc3640dabeb8f98b52b
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: sync mptcp skb cb layout with tcp one

This commit aligns struct mptcp_skb_cb with struct tcp_skb_cb by narrowing
map_seq and end_seq to 32-bit sequence numbers and adding a 64-bit
map_seq64 field for receive window admission and coalescing checks.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3bd7d38ce78c4..3e10b06b7772d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  	 *
>  	 * This is correct even when offset > 0 and mapping is out-of-order,
>  	 * because consumed prefix bytes are part of the same logical DSN range.
> +	 *
> +	 * Note that end seq number is only available in 32bits format.
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> -				     offset;
> +	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
> +				       offset;
> +	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
>  	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
> +	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: Critical]
As reported by Sashiko on v19 [1], when offset > 0 in mptcp_init_skb() and
the active DSS mapping is out of order at the MPTCP level
(mptcp_subflow_get_mapped_dsn(subflow) > msk->ack_seq), can subtracting
offset from map_seq64 and map_seq without stripping the [0, offset) prefix
from skb corrupt the MPTCP receive stream?

On a subflow where data is multiplexed across multiple subflows,
consecutive DSS mappings do not have contiguous DSNs. If the leading
[0, offset) bytes of the head skb on ssk->sk_receive_queue belonged to a
previous DSS mapping (for example after mptcp_subflow_discard_data() in
subflow_check_data_avail() discards an already-received mapping and
advances to the next mapping in the same skb) or an overlapping TCP
retransmission, those prefix bytes are not part of the active mapping
starting at tp->copied_seq and are not covered by its DSS checksum in
validate_data_csum().

Setting map_seq64 = mapped_dsn - offset and end_seq = map_seq + skb->len
assigns the fabricated DSN range [mapped_dsn - offset, mapped_dsn) to the
unpulled [0, offset) prefix bytes:

- If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
  treats the skb as covering msk->ack_seq, queues or coalesces it into
  sk->sk_receive_queue, and advances msk->ack_seq across the unreceived
  gap. __mptcp_recvmsg_mskq() and mptcp_recv_skb() then compute a read
  offset of (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq < offset and
  copy the stale [0, offset) subflow bytes to user space.

- If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
  skb into msk->out_of_order_queue starting at mapped_dsn - offset, where
  __mptcp_try_coalesce() can coalesce the stale prefix into a preceding OFO
  skb, real segments arriving for [mapped_dsn - offset, mapped_dsn) are
  dropped as duplicate data, and __mptcp_ofo_queue() dequeues the skb once
  msk->ack_seq reaches mapped_dsn - offset.

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7878e6ac6c0b6..fa54f70ab501d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -129,9 +129,12 @@
>  #define MPTCP_SYNC_SEQ		8
>  
>  struct mptcp_skb_cb {
> -	u64 map_seq;
> -	u64 end_seq;
> +	u32 map_seq;
> +	u32 end_seq;
> +	u32 unused;
> +	u16 flags;
>  	u8  has_rxtstamp;
> +	u64 map_seq64;
>  };

[Severity: High]
Now that MPTCP_SKB_CB(skb)->map_seq is a u32, does mptcp_inq_hint()
overflow when subtracting it from the 64-bit msk->ack_seq?

In net/mptcp/protocol.c:mptcp_inq_hint():

	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}

Because READ_ONCE(msk->ack_seq) is a u64 and MPTCP_SKB_CB(skb)->map_seq is
zero-extended from u32 to u64, the upper 32 bits of msk->ack_seq remain in
hint_val (or the subtraction underflows across a 32-bit wrap), causing
hint_val >= INT_MAX to evaluate to true whenever sk->sk_receive_queue is
non-empty and msk->ack_seq has non-zero upper 32 bits.

In addition to returning INT_MAX for SIOCINQ in mptcp_ioctl() and
TCP_CM_INQ in mptcp_recvmsg(), mptcp_rcv_space_adjust() does:

	copied = msk->rcvq_space.copied;
	copied -= mptcp_inq_hint(sk);
	if (copied <= msk->rcvq_space.space)
		goto new_measure;

When mptcp_inq_hint() returns INT_MAX, copied becomes negative and jumps to
new_measure, preventing mptcp_rcvbuf_grow() from auto-tuning the MPTCP and
subflow receive buffers. Should mptcp_inq_hint() use 32-bit sequence
arithmetic (or subtract msk->copied_seq from msk->ack_seq) instead?

[1] https://lore.kernel.org/r/1c928472a89719130041c92cadd7e0fb6dbf552a.1793361629.git.tanggeliang@kylinos.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=5

Re: [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mptcp: broken mptcp_inq_hint() 32-bit map_seq subtraction and MSG_PEEK sync
- [High] mptcp: data corruption in mptcp_init_skb() when subtracting non-zero offset
- [Medium] mptcp: link failure in __mptcp_read_sock() when CONFIG_LOCKDEP=n
- [Low] mptcp: redundant mptcp_recv_skb() call retained in __mptcp_read_sock()
--

commit db7d508291f7183730bd0d3c5939151c2b4d01c3
Author: Geliang Tang <geliang@kernel.org>

mptcp: defer read_sock cleanup to mptcp_worker

This commit defers mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() from
__mptcp_read_sock() to mptcp_worker() via MPTCP_WORK_READ_COMPLETE when called
in BH context, while keeping cleanup inline when the user owns the socket
lock. It also updates the socket lock check in __mptcp_read_sock() and removes
the backlog flush from mptcp_recv_skb().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3e10b06b7772d..25f0d10373d22 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3156,6 +3156,21 @@ static void mptcp_backlog_purge(struct sock *sk)
>  	sk_mem_reclaim(sk);
>  }
>  
> +static void mptcp_read_complete(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int read_copied;
> +
> +	mptcp_data_lock(sk);
> +	read_copied = msk->read_copied;
> +	msk->read_copied = 0;
> +	mptcp_data_unlock(sk);
> +
> +	mptcp_rcv_space_adjust(msk, read_copied);

[Severity: High]
As reported by Sashiko on v19 [1], does mptcp_rcv_space_adjust() ->
mptcp_inq_hint() break receive buffer autotuning here (as well as SIOCINQ and
TCP_CM_INQ) due to subtracting the 32-bit MPTCP_SKB_CB(skb)->map_seq from the
64-bit msk->ack_seq?

In commit 698eb6e05455 ("mptcp: remove CB offset field"),
MPTCP_SKB_CB(skb)->map_seq stopped being advanced on partial reads, which
instead advance msk->copied_seq, and commit 0495d49fc957 ("mptcp: sync mptcp
skb cb layout with tcp one") then narrowed MPTCP_SKB_CB(skb)->map_seq from u64
to u32:

net/mptcp/protocol.h:
struct mptcp_skb_cb {
	u32 map_seq;
	u32 end_seq;
	u32 unused;
	u16 flags;
	u8  has_rxtstamp;
	u64 map_seq64;
};

However, mptcp_inq_hint() still subtracts MPTCP_SKB_CB(skb)->map_seq from
msk->ack_seq:

net/mptcp/protocol.c:mptcp_inq_hint() {
	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}
}

Because MPTCP_SKB_CB(skb)->map_seq is zero-extended from u32 to u64 while
msk->ack_seq is a 64-bit sequence number whose upper 32 bits are non-zero on
nearly all MPTCP connections (or 0 while a passive TFO packet has map_seq =
(u32)-skb->len), hint_val retains non-zero upper 32 bits and returns INT_MAX
whenever any skb is present in sk->sk_receive_queue. Even when the upper 32
bits of msk->ack_seq happen to be zero, MPTCP_SKB_CB(skb)->map_seq does not
account for bytes already consumed via msk->copied_seq.

In mptcp_rcv_space_adjust(), subtracting INT_MAX causes copied to become
negative and skip mptcp_rcvbuf_grow():

net/mptcp/protocol.c:mptcp_rcv_space_adjust() {
	copied = msk->rcvq_space.copied;
	copied -= mptcp_inq_hint(sk);
	if (copied <= msk->rcvq_space.space)
		goto new_measure;
}

Additionally, in mptcp_recvmsg(), peek_seq is only updated when
mptcp_move_skbs(sk) returns true:

net/mptcp/protocol.c:mptcp_recvmsg() {
		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk)) {
			if (flags & MSG_PEEK)
				peek_seq = msk->copied_seq + copied;
			continue;
		}
}

If the third ACK arrives with an out-of-order or duplicate segment while
lock_sock(sk) is held during MSG_PEEK, mptcp_move_skbs(sk) processes
MPTCP_SYNC_SEQ (adding msk->tfo_iasn to msk->copied_seq and updating the TFO
skb map_seq) but returns false, leaving peek_seq unsynchronized in the
pre-IASN sequence space. Should mptcp_inq_hint() subtract msk->copied_seq from
READ_ONCE(msk->ack_seq), and should mptcp_recvmsg() also resynchronize
peek_seq when MPTCP_SYNC_SEQ runs?

[ ... ]

> @@ -4643,9 +4663,6 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
>  	struct sk_buff *skb;
>  	u32 offset;
>  
> -	if (!list_empty(&msk->backlog_list))
> -		mptcp_move_skbs(sk);
> -
>  	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
>  		offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;

[Severity: High]
As reported by Sashiko on v19 [2], can subtracting a non-zero subflow offset
in mptcp_init_skb() corrupt data delivery when an out-of-order subflow skb has
a discarded prefix?

In subflow_check_data_avail(), when a TCP skb on ssk->sk_receive_queue
contains the tail of an already-acknowledged DSS mapping in bytes 0..offset
and the start of a new DSS mapping at offset (which get_mapping_status()
allows when !skb_is_fully_mapped(ssk, skb)), mptcp_subflow_discard_data()
advances tp->copied_seq by offset, clears subflow->map_valid, and loops to
activate the new DSS mapping on the same skb:

net/mptcp/subflow.c:subflow_check_data_avail() {
		if (unlikely(before64(ack_seq, old_ack))) {
			mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq);
			continue;
		}
}

When __mptcp_move_skbs_from_subflow() then initializes the skb via
mptcp_init_skb(), it subtracts offset from mptcp_subflow_get_mapped_dsn():

net/mptcp/protocol.c:mptcp_init_skb() {
	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
				       offset;
	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
}

Because MPTCP stripes mappings across subflows, the new mapping starting at
offset in skb can have an out-of-order DSN (mapped_dsn > msk->ack_seq) that is
not contiguous with the discarded bytes in 0..offset. Subtracting offset
assigns the unreceived DSN range [mapped_dsn - offset, mapped_dsn) to the
stale discarded prefix in 0..offset:

net/mptcp/protocol.c:__mptcp_move_skb() {
	if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
		/* in sequence */
insert:
...
	} else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
		mptcp_data_queue_ofo(msk, skb);
		return false;
	}

	/* Partial packet */
	if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
		copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
		goto insert;
	}
}

If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb() sees
map_seq64 <= msk->ack_seq, queues the out-of-order skb onto
sk->sk_receive_queue, and advances msk->ack_seq across the missing DSN gap,
causing mptcp_recv_skb() and __mptcp_recvmsg_mskq() to deliver the discarded
prefix bytes from 0..offset to user space. If mapped_dsn - offset >
msk->ack_seq, mptcp_data_queue_ofo() queues the skb into
msk->out_of_order_queue as covering [mapped_dsn - offset, mapped_dsn +
skb->len - offset), dropping legitimate segments for [mapped_dsn - offset,
mapped_dsn) as duplicates and delivering the stale prefix once msk->ack_seq
reaches mapped_dsn - offset.

> @@ -4660,6 +4677,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
>  /*
>   * Note:
>   *	- It is assumed that the socket was locked by the caller.
> + *	- Can be invoked in BH scope.
>   */
>  static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  			     sk_read_actor_t recv_actor, bool noack)
> @@ -4669,7 +4687,7 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	int copied = 0;
>  	u32 offset;
>  
> -	msk_owned_by_me(msk);
> +	DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk));

[Severity: Medium]
Does replacing msk_owned_by_me(msk) with
DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk)) cause a link failure when
CONFIG_DEBUG_NET=y and CONFIG_LOCKDEP=n?

In include/net/sock.h, lockdep_sock_is_held(sk) calls lockdep_is_held() on
both sk->sk_lock and sk->sk_lock.slock:

include/net/sock.h:lockdep_sock_is_held() {
	return lockdep_is_held(&sk->sk_lock) ||
	       lockdep_is_held(&sk->sk_lock.slock);
}

When CONFIG_LOCKDEP=n, include/linux/lockdep.h only declares lockdep_is_held()
as a dummy extern prototype with no definition, relying on dead-code
elimination. Because CONFIG_DEBUG_NET does not depend on CONFIG_LOCKDEP,
enabling CONFIG_DEBUG_NET=y with CONFIG_LOCKDEP=n expands
DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk)) to WARN_ON_ONCE() and leaves
an undefined reference to lockdep_is_held at link time.

Note that msk_owned_by_me(msk) calls sock_owned_by_me(sk):

include/net/sock.h:sock_owned_by_me() {
#ifdef CONFIG_LOCKDEP
	WARN_ON_ONCE(!lockdep_sock_is_held(sk) && debug_locks);
#endif
}

Since sock_owned_by_me(sk) already checks lockdep_sock_is_held(sk) for both
process and BH contexts while guarding on CONFIG_LOCKDEP and debug_locks, can
msk_owned_by_me(msk) be kept here as-is?

>  
>  	if (sk->sk_state == TCP_LISTEN)
>  		return -ENOTCONN;
> @@ -4701,11 +4719,18 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	if (noack)
>  		goto out;
>  
> -	mptcp_rcv_space_adjust(msk, copied);
> -
>  	if (copied > 0) {
> -		mptcp_recv_skb(sk, &offset);
> -		mptcp_cleanup_rbuf(msk, copied);
> +		if (sock_owned_by_user(sk)) {
> +			/* Process context: inline cleanup */
> +			mptcp_rcv_space_adjust(msk, copied);
> +			mptcp_recv_skb(sk, &offset);

[Severity: Low]
The commit message states:

  Also remove the backlog flush from mptcp_recv_skb() (covered by
  mptcp_release_cb()) and drop the redundant mptcp_recv_skb() call after
  the main loop.

Should this mptcp_recv_skb(sk, &offset) call in the sock_owned_by_user(sk)
branch be dropped to match the commit message, since the main loop already
calls mptcp_eat_recv_skb() on fully consumed skbs and mptcp_recv_skb() no
longer flushes msk->backlog_list?

[1] https://lore.kernel.org/r/90a300d74f44443759ebe14be70c07385267db5b.1793361629.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/90a300d74f44443759ebe14be70c07385267db5b.1793361629.git.tanggeliang@kylinos.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=6