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

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

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v21:
 - patch 1: new patch. Fix mptcp_inq_hint() to use mptcp_data_avail()
   which computes bytes_received - bytes_consumed instead of
   ack_seq - map_seq. This fixes INT_MAX overflow on passive TFO
   connections before the MPC 3rd ACK arrives.
 - patch 2: new patch. Add selftest to check TFO SIOCINQ doesn't return
   INT_MAX after accept().
 - patch 4: new patch. Fix fallback dummy mapping anchor in
   subflow_check_data_avail() by anchoring map_subflow_seq at
   TCP_SKB_CB(skb)->seq instead of copied_seq. This ensures
   mptcp_subflow_get_mapped_dsn() returns the correct DSN at copied_seq.
 - patch 7: added mapping boundary detection in mptcp_init_skb(). When
   offset > 0 and skb starts before current mapping, prefix bytes are
   removed via skb_pull() to prevent data corruption. Removed redundant
   fallback initialization in mptcp_propagate_state().
 - patch 9: kept msk_owned_by_me() assertion (reverted the
   DEBUG_NET_WARN_ON_ONCE replacement from v20).

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.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793695518.git.tanggeliang@kylinos.cn/

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 (5):
  mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint
  selftests: mptcp: connect: check TFO SIOCINQ overflow
  mptcp: fix TFO map_seq sign extension in fastopen
  mptcp: fix fallback dummy mapping anchor
  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                          | 291 +++++++++++-------
 net/mptcp/protocol.h                          |  16 +-
 net/mptcp/subflow.c                           |  12 +-
 .../selftests/net/mptcp/mptcp_connect.c       |  14 +
 5 files changed, 232 insertions(+), 117 deletions(-)

-- 
2.55.0

[PATCH mptcp-net v21 1/9] mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Fix mptcp_inq_hint() to compute available bytes using mptcp_data_avail()
(bytes_received - bytes_consumed) instead of ack_seq minus the head skb's
map_seq.

On passive TFO connections, the TFO skb has map_seq set to -skb->len
(representing data before IASN). When the third ACK arrives,
subflow_set_remote_key() sets msk->ack_seq to subflow->iasn. Before
commit f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()"),
that function adjusted the TFO skb's map_seq. Since then, mptcp_inq_hint()
computes ack_seq - map_seq = iasn + len, which is almost always >= INT_MAX,
causing the function to return INT_MAX.

As a result, SIOCINQ/FIONREAD through mptcp_ioctl() and the TCP_CM_INQ
cmsg in mptcp_recvmsg() report INT_MAX instead of the actual byte count,
and mptcp_rcv_space_adjust() subtracts INT_MAX from copied. This lasts
until the TFO skb is fully consumed.

Using mptcp_data_avail() avoids this regression by tracking bytes directly
without depending on map_seq.

The EOF hint behavior (returning 1 when socket is closed with no data)
is preserved.

Fixes: f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..fc4ffa326317 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2402,22 +2402,16 @@ static bool mptcp_move_skbs(struct sock *sk)
 static unsigned int mptcp_inq_hint(const struct sock *sk)
 {
 	const struct mptcp_sock *msk = mptcp_sk(sk);
-	const struct sk_buff *skb;
+	u64 hint_val = mptcp_data_avail(msk);
 
-	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;
 
-		if (hint_val >= INT_MAX)
-			return INT_MAX;
-
-		return (unsigned int)hint_val;
-	}
-
-	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
+	if (!hint_val &&
+	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
 		return 1;
 
-	return 0;
+	return (unsigned int)hint_val;
 }
 
 static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
-- 
2.55.0

[PATCH mptcp-next v21 2/9] selftests: mptcp: connect: check TFO SIOCINQ overflow

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add a SIOCINQ check in mptcp_connect's server mode when the MPTFO socket
option is enabled to detect TFO SIOCINQ overflow regressions.

When a TFO connection is accepted, the test waits 10ms for the third ACK
to be processed, then checks if SIOCINQ returns INT_MAX. This detects the
regression where mptcp_inq_hint() incorrectly returns INT_MAX for passive
TFO connections after the third ACK sets msk->ack_seq to subflow->iasn.

The check runs when the MPTFO sockopt is set and uses ioctl(SIOCINQ) to
verify the available byte count. If SIOCINQ returns INT_MAX, the test
fails.

This test verifies the fix in the preceding commit "mptcp: fix TFO SIOCINQ
overflow in mptcp_inq_hint".

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..e30a6900d01f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1207,6 +1207,20 @@ int main_loop_s(int listensock)
 		check_sockaddr(pf, &ss, salen);
 		check_getpeername(remotesock, &ss, salen);
 
+		if (cfg_sockopt_types.mptfo) {
+			int avail;
+
+			/* Wait for third ACK to be processed */
+			usleep(10000);  /* 10ms */
+
+			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
+			    avail == INT_MAX) {
+				fprintf(stderr, "TFO SIOCINQ overflow\n");
+				close(remotesock);
+				return 1;
+			}
+		}
+
 		if (cfg_input) {
 			fd = open(cfg_input, O_RDONLY);
 			if (fd < 0)
-- 
2.55.0

[PATCH mptcp-net v21 3/9] 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 mptcp_inq_hint() to return INT_MAX before the MPC 3rd ACK
arrives. On a passive TFO msk, msk->ack_seq is still 0 until the 3rd
ACK. Without the cast to u64, the computation 0ULL - (u32)(-len)
underflows and clamps to INT_MAX. This incorrect value is returned to
user space through SIOCINQ/FIONREAD in mptcp_ioctl(), the TCP_CM_INQ
cmsg in mptcp_recvmsg(), and also affects receive buffer auto-tuning in
mptcp_rcv_space_adjust().

Fix by casting to u64 before negation: -(u64)skb->len. This ensures
proper 64-bit two's complement representation, so the hint correctly
returns skb->len before the 3rd ACK.

Fixes: 36b122baf6a8 ("mptcp: add subflow_v(4,6)_send_synack()")
Co-developed-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
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 f717750906ff..914d52960dc6 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -44,8 +44,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-net v21 4/9] mptcp: fix fallback dummy mapping anchor

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Commit 2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation")
anchored map_seq at the DSN of skb->seq but left map_subflow_seq
anchored at copied_seq. This caused mptcp_subflow_get_mapped_dsn() to
return the DSN at skb->seq instead of copied_seq.

When TCP queues a partially overlapping segment (seq < rcv_nxt), the skb
starts before copied_seq, giving offset > 0. mptcp_init_skb() then sets
map_seq to the wrong DSN, causing __mptcp_move_skb() to skip data or
drop the skb, leaving a hole in the stream.

Fix by anchoring map_subflow_seq at TCP_SKB_CB(skb)->seq, ensuring
mptcp_subflow_get_mapped_dsn() returns the correct DSN at copied_seq.

Fixes: 2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..73417740074f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1485,7 +1485,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 v21 5/9] 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 fc4ffa326317..71f5b0209192 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 v21 6/9] 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
the cb_flags are protected by the msk data lock. A rcvd_dummy_seq flag
tracks whether the TFO skb still carries the dummy mapping and needs
synchronization.

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 | 39 ++++++++++++++++++++++++++++++++++++---
 net/mptcp/protocol.h |  5 ++++-
 net/mptcp/subflow.c  |  8 ++++++++
 4 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 914d52960dc6..79765cf55286 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;
@@ -51,15 +52,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 71f5b0209192..dd612e9b2b26 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,7 +2397,13 @@ static bool mptcp_move_skbs(struct sock *sk)
 	u32 moved = 0;
 
 	mptcp_data_lock(sk);
-	while (mptcp_can_spool_backlog(sk, &skbs)) {
+	while (1) {
+		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+			__mptcp_sync_rcv_sequence(sk);
+
+		if (!mptcp_can_spool_backlog(sk, &skbs))
+			break;
+
 		mptcp_data_unlock(sk);
 		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
 
@@ -3674,6 +3702,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 */
@@ -3876,6 +3906,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 73417740074f..bf9a75118ac9 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 v21 7/9] 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 introduced `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.

mptcp_inq_hint() now uses mptcp_data_avail() which computes
bytes_received - bytes_consumed, avoiding the previous ack_seq - map_seq
computation that could overflow when map_seq was a dummy negative value.

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. A mapping boundary check detects when the skb spans multiple
DSS mappings (offset > 0 and skb starts before the current mapping). In
that case, the consumed prefix bytes belong to an old mapping and are
removed via skb_pull() before setting map_seq.

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 | 98 +++++++++++++++++++++++++-------------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  |  6 ++-
 4 files changed, 61 insertions(+), 47 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 dd612e9b2b26..0666bbfdf2cb 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,19 +414,34 @@ 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;
 
+	/* Detect mapping boundary: skb spans multiple mappings */
+	if (unlikely(offset > 0 &&
+		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
+		     subflow->map_subflow_seq)) {
+		pr_debug("spans mappings seq=%u map_subflow_seq=%u offset=%d ssn_offset=%u\n",
+			 TCP_SKB_CB(skb)->seq, subflow->map_subflow_seq,
+			 offset, subflow->ssn_offset);
+
+		/* prefix bytes belong to old mapping, remove them and start
+		 * from current mapping
+		 */
+		WARN_ON_ONCE(offset >= skb->len);
+		skb_pull(skb, offset);
+		offset = 0;
+	}
+
 	/* the skb map_seq accounts for the skb offset:
 	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
 	 * value
 	 */
-	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 +465,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 +508,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 +865,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 +928,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 +2214,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 +2250,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 {
@@ -2398,8 +2394,10 @@ static bool mptcp_move_skbs(struct sock *sk)
 
 	mptcp_data_lock(sk);
 	while (1) {
-		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);
+		}
 
 		if (!mptcp_can_spool_backlog(sk, &skbs))
 			break;
@@ -2439,6 +2437,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;
 
@@ -2457,6 +2456,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;
@@ -2466,7 +2470,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)
@@ -2476,8 +2480,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()
@@ -2520,6 +2527,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);
@@ -3696,6 +3707,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;
@@ -3906,8 +3918,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)
@@ -4628,7 +4642,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;
@@ -4670,11 +4684,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 bf9a75118ac9..daf14d69e529 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,
-- 
2.55.0

[PATCH mptcp-next v21 8/9] 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 uses 64 bits for the sequence numbers.

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

The CB layout changes map_seq and end_seq from u64 to u32, adds a new
map_seq64 field to track the full 64-bit DSN, and adds a flags field.
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 a build time check to ensure the CB layout matches TCP's layout for
the shared fields.

In mptcp_init_skb(), map_seq64 is computed as mptcp_subflow_get_mapped_dsn()
minus offset, and map_seq is the lower 32 bits. A mapping boundary check
detects when the skb spans multiple DSS mappings (offset > 0 and skb starts
before the current mapping). In that case, the consumed prefix bytes belong
to an old mapping and are removed via skb_pull() before setting map_seq.

The receive path now uses 32-bit before()/after() helpers for most sequence
comparisons, while map_seq64 is used for window validation and coalescing
checks. __mptcp_try_coalesce() gains a map_seq64 contiguity check to ensure
skbs are truly adjacent in the 64-bit DSN space.

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, 67 insertions(+), 40 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 0666bbfdf2cb..705b28219883 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);
@@ -437,11 +446,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
+	 * value; 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);
@@ -464,13 +475,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;
 
@@ -482,7 +494,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)) {
@@ -500,14 +512,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;
 	}
 
@@ -902,39 +914,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;
@@ -2223,7 +2233,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;
@@ -4642,7 +4652,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;
@@ -4892,11 +4902,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 v21 9/9] mptcp: defer read_sock cleanup to mptcp_worker

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch prepares __mptcp_read_sock() for future TLS-over-MPTCP support,
where it may be reached from BH context (via tls_strp_read_copyin() ->
sk->sk_read_sock()). In that case, calling mptcp_rcv_space_adjust() and
mptcp_cleanup_rbuf() is not safe as both expect process context.

Note: The BH call path does not exist in the current tree. This is
groundwork for the upcoming TLS-over-MPTCP series. Currently, the only
callers are mptcp_splice_read() and svc_tcp_read_msg(), both of which
hold lock_sock() and run in 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.

Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()). The mptcp_recv_skb() call remains in the
sock_owned_by_user() branch for inline cleanup.

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 | 39 ++++++++++++++++++++++++++++++++-------
 net/mptcp/protocol.h |  2 ++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 705b28219883..b15100a67672 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3161,6 +3161,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;
@@ -3236,6 +3251,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);
@@ -3279,6 +3297,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);
@@ -3726,6 +3745,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 */
@@ -4648,9 +4668,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) {
@@ -4665,6 +4682,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)
@@ -4706,11 +4724,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 v21 7/9] mptcp: remove CB offset field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: kernel BUG in mptcp_init_skb() when pulling nonlinear skb
- [High] mptcp: 32-bit sequence wrap bug in mptcp_init_skb() boundary check
--

commit 1950bbe60e98f6cc91ceb0674f4d63a6b8018ce9
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit removes the offset field from struct mptcp_skb_cb and replaces it
with a new copied_seq field in struct mptcp_sock to track bytes already
consumed inside each skb. It also adds a mapping boundary check in
mptcp_init_skb() to strip already-consumed prefix bytes belonging to a
previous DSS mapping via skb_pull().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dd612e9b2b262..0666bbfdf2cb8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -415,19 +414,34 @@ 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;
>  
> +	/* Detect mapping boundary: skb spans multiple mappings */
> +	if (unlikely(offset > 0 &&
> +		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
> +		     subflow->map_subflow_seq)) {

[Severity: High]
As reported by Sashiko on v20 [1], should this comparison use before()
instead of raw unsigned < for 32-bit modular relative subflow sequence
numbers, matching validate_mapping() in net/mptcp/subflow.c?

net/mptcp/subflow.c:validate_mapping() {
	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {
		...
}

TCP_SKB_CB(skb)->seq, subflow->ssn_offset, and subflow->map_subflow_seq are
all u32 values. Using unsigned < causes regressions in two scenarios:

First, on passive TFO subflows, mptcp_fastopen_subflow_synack_set_params()
in net/mptcp/fastopen.c advances subflow->ssn_offset past the TFO SYN data:

	tp->copied_seq += skb->len;
	subflow->ssn_offset += skb->len;

If a peer sends an overlapping segment starting in the TFO SYN data range
where TCP_SKB_CB(skb)->seq < subflow->ssn_offset < TCP_SKB_CB(skb)->end_seq,
or when relative subflow sequence numbers wrap around 2^32 across two
mappings, TCP_SKB_CB(skb)->seq - subflow->ssn_offset wraps near U32_MAX while
subflow->map_subflow_seq is small. The unsigned < check evaluates to false,
leaving the old prefix bytes in skb and setting MPTCP_SKB_CB(skb)->map_seq to
mptcp_subflow_get_mapped_dsn(subflow) - offset, which corrupts the receive
stream.

Second, if a single DSS mapping starts near U32_MAX and wraps past 0, an skb
within that same mapping whose relative sequence number has wrapped past 0
with offset > 0 will satisfy unsigned < against subflow->map_subflow_seq and
falsely trigger the boundary branch.

> +		pr_debug("spans mappings seq=%u map_subflow_seq=%u offset=%d ssn_offset=%u\n",
> +			 TCP_SKB_CB(skb)->seq, subflow->map_subflow_seq,
> +			 offset, subflow->ssn_offset);
> +
> +		/* prefix bytes belong to old mapping, remove them and start
> +		 * from current mapping
> +		 */
> +		WARN_ON_ONCE(offset >= skb->len);
> +		skb_pull(skb, offset);
> +		offset = 0;
> +	}

[Severity: Critical]
Can calling skb_pull(skb, offset) in __mptcp_move_skbs_from_subflow() ->
mptcp_init_skb() trigger a kernel BUG() when the subflow receive skb is
non-linear?

When TCP queues incoming segments onto ssk->sk_receive_queue in
tcp_data_queue(), the TCP header has already been pulled via __skb_pull().
For non-linear skbs where payload bytes reside in skb_shinfo(skb)->frags[]
(such as packets sent by mptcp_sendmsg_frag() over loopback or veth, GRO
packets, page_pool RX packets, or segments coalesced via tcp_try_coalesce()),
skb_headlen(skb) is 0 or smaller than offset.

Calling skb_pull() invokes __skb_pull() in include/linux/skbuff.h:

static inline void *__skb_pull(struct sk_buff *skb, unsigned int len)
{
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
#if defined(CONFIG_DEBUG_NET)
		skb->len += len;
		pr_err("__skb_pull(len=%u)\n", len);
		skb_dump(KERN_ERR, skb, false);
#endif
		BUG();
	}
	return skb->data += len;
}

When offset > skb_headlen(skb), skb->len becomes smaller than skb->data_len
and __skb_pull() hits BUG(). Would pskb_pull() or __pskb_pull_tail() be
needed here, or can the offset be handled without pulling paged data?

[1] https://lore.kernel.org/r/866f86ad2ea894fc21af7546ed0cf6f8b9d816de.1793695518.git.tanggeliang@kylinos.cn

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

Re: [PATCH mptcp-next v21 8/9] 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: skb_pull() BUG() on nonlinear skb and bad offset in mptcp_init_skb()
- [High] mptcp: wrap-unsafe mapping boundary check in mptcp_init_skb()
--

commit 9d35387c93b4a664b7d58d9f115a8985e1df6730
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: sync mptcp skb cb layout with tcp one

This commit updates struct mptcp_skb_cb to use 32-bit map_seq and end_seq
fields aligned with struct tcp_skb_cb, while adding a 64-bit map_seq64 field
for window validation and coalescing checks.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0666bbfdf2cb8..705b28219883f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -437,11 +446,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
> +	 * value; 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;

[Severity: Critical]
As reported by Sashiko on v20 [1], in mptcp_init_skb():

	/* Detect mapping boundary: skb spans multiple mappings */
	if (unlikely(offset > 0 &&
		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
		     subflow->map_subflow_seq)) {
		...
		WARN_ON_ONCE(offset >= skb->len);
		skb_pull(skb, offset);
		offset = 0;
	}

	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;

can calling skb_pull(skb, offset) here trigger a BUG() in __skb_pull() when
the incoming subflow skb is non-linear?

In tcp_data_queue(), the TCP header has already been pulled via __skb_pull().
When the TCP payload resides in page fragments (for example, skbs generated by
mptcp_sendmsg_frag() where only MAX_TCP_HEADER linear space is allocated, GRO
packets, page_pool buffers, or segments coalesced by tcp_try_coalesce()),
skb_headlen(skb) can be 0 or smaller than offset. Because skb_pull() calls
__skb_pull():

	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}

pulling offset without pskb_pull() or __pskb_trim_head() reduces skb->len
below skb->data_len and panics the kernel whenever offset > skb_headlen(skb).

Additionally, when offset > 0 within a single DSS mapping (so the mapping
boundary branch is not taken), mptcp_init_skb() leaves the [0, offset) prefix
attached and subtracts offset from map_seq64. Since validate_data_csum() only
validates data starting at tp->copied_seq, if this skb is queued out of order
in __mptcp_move_skb() -> mptcp_data_queue_ofo():

		if (before(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
			if (!after((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
				...
			}
			if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
				...
			} else {
				rb_replace_node(&skb1->rbnode, &skb->rbnode,
						&msk->out_of_order_queue);
				mptcp_drop(sk, skb1);
				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
				goto merge_right;
			}
		}

won't the offset-shifted map_seq cause mptcp_data_queue_ofo() to replace an
already-validated skb1 at mapped_dsn - offset with the unvalidated [0, offset)
prefix, or prevent mptcp_try_coalesce() when skb1 starts earlier?

[Severity: High]
Also in mptcp_init_skb(), the mapping boundary check compares 32-bit relative
subflow sequence numbers using unsigned <:

	if (unlikely(offset > 0 &&
		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
		     subflow->map_subflow_seq)) {

Unlike validate_mapping() in net/mptcp/subflow.c, which uses before():

	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {

what happens when the 32-bit relative subflow sequence number wraps around
2^32 (or when TCP_SKB_CB(skb)->seq precedes subflow->ssn_offset, such as a
segment overlapping TFO SYN data)?

If the skb starts before the wrap in an old mapping (for example,
TCP_SKB_CB(skb)->seq - subflow->ssn_offset == 0xfffffe00) and the current
mapping starts after the wrap (for example, subflow->map_subflow_seq ==
0x00000100), unsigned < evaluates to false. Wouldn't mptcp_init_skb() then
skip removing the old mapping's prefix bytes and subtract offset from
mptcp_subflow_get_mapped_dsn(subflow), attributing stale prefix bytes from
the old mapping to the new mapping's DSN range [mapped_dsn - offset,
mapped_dsn)? Conversely, if a single mapping straddles the 2^32 wrap boundary,
wouldn't an skb starting after the wrap with offset > 0 falsely satisfy
unsigned < and strip valid prefix bytes?

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

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

Re: [PATCH mptcp-next v21 0/9] 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): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/42007402371

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


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-net v21 1/9] mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In net/mptcp/protocol.c, mptcp_inq_hint() now returns
  mptcp_data_avail() (bytes_received - bytes_consumed) instead of…

--

mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

This changes mptcp_inq_hint() so it computes the hint from
mptcp_data_avail() (bytes_received - bytes_consumed) and no longer uses
ack_seq minus the head skb's map_seq. The goal is to stop passive TFO
connections from reporting INT_MAX through SIOCINQ/FIONREAD and
TCP_CM_INQ. The EOF hint of 1 is kept for the empty-queue case.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..fc4ffa3263176 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2402,22 +2402,16 @@ static bool mptcp_move_skbs(struct sock *sk)
>  static unsigned int mptcp_inq_hint(const struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
> -	const struct sk_buff *skb;
> +	u64 hint_val = mptcp_data_avail(msk);

[Severity: Low]
This change also affects non-TFO MPTCP sockets once a DATA_FIN has been
received. Was that intended?

When an in-sequence DATA_FIN is processed, mptcp_check_data_fin() bumps
ack_seq, but bytes_received stays the same:

net/mptcp/protocol.c:mptcp_check_data_fin() {
    ...
	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
    ...
}

Elsewhere, __mptcp_move_skb() and __mptcp_ofo_queue() advance
bytes_received and ack_seq together. Partial reads also advanced the head
skb's map_seq. So with N > 0 bytes still unread after a DATA_FIN, the old
ack_seq - map_seq gave N + 1, and mptcp_data_avail() now gives N.

That change shows up in SIOCINQ/FIONREAD from mptcp_ioctl(), in the
TCP_CM_INQ cmsg from mptcp_recvmsg(), and in the value that
mptcp_rcv_space_adjust() subtracts from copied.

The new value doesn't line up with TCP on both interfaces. On TCP,
tcp_inq() subtracts 1 when FIN was received, so SIOCINQ reports N, and the
new MPTCP SIOCINQ now matches that. tcp_inq_hint() uses
rcv_nxt - copied_seq and does not subtract the FIN, so TCP_CM_INQ reports
N + 1. The old MPTCP TCP_CM_INQ matched TCP here, and the new one does not.

The commit message says:

    The EOF hint behavior (returning 1 when socket is closed with no data)
    is preserved.

That is true for the empty-queue case. Could the commit message also
mention the N + 1 to N change for non-TFO connections after DATA_FIN, and
whether it is intended? This matters for stable backports of this Fixes:
patch.

>  
> -	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;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn

Re: [PATCH mptcp-next v21 2/9] selftests: mptcp: connect: check TFO SIOCINQ overflow

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new SIOCINQ regression check in main_loop_s()
  (tools/testing/selftests/net/mptcp/mptcp_connect.c) depends on timing,…

--

selftests: mptcp: connect: check TFO SIOCINQ overflow

This adds a SIOCINQ check to main_loop_s() in mptcp_connect for runs where
the MPTFO sockopt is set. After accept(), the server sleeps 10ms so the
third ACK can be processed. The test fails if ioctl(SIOCINQ) returns
INT_MAX.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..e30a6900d01f5 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1207,6 +1207,20 @@ int main_loop_s(int listensock)
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
>  
> +		if (cfg_sockopt_types.mptfo) {
> +			int avail;
> +
> +			/* Wait for third ACK to be processed */
> +			usleep(10000);  /* 10ms */
> +
> +			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
> +			    avail == INT_MAX) {

[Severity: Medium]
Is a fixed 10ms sleep enough to make this check reliable?

With passive TFO, the child socket is put on the accept queue while the
SYN is being handled. That means accept() can return before the MPC third
ACK has been processed. msk->ack_seq only changes later, in softirq:

__mptcp_subflow_fully_established()
    subflow_set_remote_key()
        WRITE_ONCE(msk->ack_seq, subflow->iasn);
        WRITE_ONCE(msk->can_ack, true);

At this commit, mptcp_fastopen_subflow_synack_set_params() still does:

    MPTCP_SKB_CB(skb)->map_seq = -skb->len;

So the old mptcp_inq_hint() returns INT_MAX even before the third ACK, and
timing does not matter yet.

Later in the series, "mptcp: fix TFO map_seq sign extension in fastopen"
changes this to -(u64)skb->len. After that, the old ack_seq - map_seq
formula gives the correct value (skb->len) while ack_seq is still 0. A
reintroduced mptcp_inq_hint() regression would then only show up if the
third ACK has been processed before ioctl() runs. No later patch in the
series changes this test.

On a loaded CI host, or on a debug kernel with KASAN, lockdep or
kmemleak, can the third ACK arrive after the 10ms window? If so, the check
reads a correct value and passes on a broken kernel.

Would it be more deterministic to poll getsockopt(SOL_MPTCP, MPTCP_INFO)
until MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED is set, with a bounded timeout
that fails loudly on expiry? mptcp_diag_fill_info() sets that flag from
msk->can_ack, which is written right after ack_seq.

Two smaller points about the same check:

It only rejects avail == INT_MAX. It does not check that ioctl()
succeeded, or that avail matches the size of the SYN payload.

Nothing confirms that passive TFO with SYN data actually happened.
run_tests_mptfo() in mptcp_connect.sh runs each address family twice:

    run_tests_lo "$ns1" "$ns2" 10.0.1.1 0 "-o MPTFO"
    run_tests_lo "$ns1" "$ns2" 10.0.1.1 0 "-o MPTFO"

The first run only fetches the TFO cookie and sends no SYN data, so the
new check passes trivially on that run.

> +				fprintf(stderr, "TFO SIOCINQ overflow\n");
> +				close(remotesock);
> +				return 1;
> +			}
> +		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn

Re: [PATCH mptcp-net v21 3/9] mptcp: fix TFO map_seq sign extension in fastopen

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message of "mptcp: fix TFO map_seq sign extension
  in fastopen" describes a user-visible bug that cannot happen in the…

--

mptcp: fix TFO map_seq sign extension in fastopen

This commit changes mptcp_fastopen_subflow_synack_set_params() in
net/mptcp/fastopen.c so the TFO skb's map_seq is set to -(u64)skb->len
instead of -skb->len. The value then becomes a proper 64-bit negative
offset from an assumed IASN of 0. The commit message says this fixes
mptcp_inq_hint() returning INT_MAX before the MPC 3rd ACK.

> This causes mptcp_inq_hint() to return INT_MAX before the MPC 3rd ACK
> arrives. On a passive TFO msk, msk->ack_seq is still 0 until the 3rd
> ACK. Without the cast to u64, the computation 0ULL - (u32)(-len)
> underflows and clamps to INT_MAX. This incorrect value is returned to
> user space through SIOCINQ/FIONREAD in mptcp_ioctl(), the TCP_CM_INQ
> cmsg in mptcp_recvmsg(), and also affects receive buffer auto-tuning in
> mptcp_rcv_space_adjust().

[Severity: Medium]
Does this description still match the tree this patch applies to?

An earlier patch in this series, "mptcp: fix TFO SIOCINQ overflow in
mptcp_inq_hint", already rewrote mptcp_inq_hint() in net/mptcp/protocol.c:

static unsigned int mptcp_inq_hint(const struct sock *sk)
{
	const struct mptcp_sock *msk = mptcp_sk(sk);
	u64 hint_val = mptcp_data_avail(msk);
	...

That function no longer reads msk->ack_seq or the head skb's map_seq.
The 0ULL - (u32)(-len) calculation described above is no longer in the
code.

The other readers of the TFO skb's map_seq don't depend on its absolute
value either. The skb has cant_coalesce set, so __mptcp_try_coalesce()
rejects it before it compares map_seq or end_seq:

	if (unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||

The only remaining accesses are map_seq += count on partial reads and
pr_debug() output.

The cast is harmless. It looks like it only becomes necessary later in
the series, once msk->copied_seq is seeded from map_seq64 and tfo_iasn
is added to it.

Could the commit message describe that dependency instead of the
SIOCINQ/TCP_CM_INQ symptom, which the earlier patch already fixed?

> Fix by casting to u64 before negation: -(u64)skb->len. This ensures
> proper 64-bit two's complement representation, so the hint correctly
> returns skb->len before the 3rd ACK.

The only tag here is Fixes: 36b122baf6a8. Could this mislead stable
backports into treating this patch as the fix for SIOCINQ returning
INT_MAX?

Take a tree that has f03afb3aeb9d but not the mptcp_inq_hint() rewrite.
There, this patch alone would only cover the window before the 3rd ACK.

After the 3rd ACK, ack_seq becomes iasn while the TFO skb's map_seq stays
at -(u64)len. So ack_seq - map_seq = iasn + len, which would still clamp
to INT_MAX.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn

Re: [PATCH mptcp-next v21 7/9] mptcp: remove CB offset field

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] A remote peer can crash the receiver: mptcp_init_skb()
  (net/mptcp/protocol.c) calls skb_pull() on a possibly non-linear…
- [Medium] The new boundary check in mptcp_init_skb() compares subflow-
  relative 32-bit sequence numbers with a plain unsigned '<', which is…
- [Medium] The commit message describes a change this commit does not
  make.

--

mptcp: remove CB offset field

This patch drops the per-skb offset field from the MPTCP skb control
block. It tracks consumed receive bytes with a new msk->copied_seq field,
the way TCP does. mptcp_init_skb() now computes map_seq as the mapped DSN
minus the offset, and uses skb_pull() to remove prefix bytes when an skb
spans multiple DSS mappings.

> mptcp_inq_hint() now uses mptcp_data_avail() which computes
> bytes_received - bytes_consumed, avoiding the previous ack_seq - map_seq
> computation that could overflow when map_seq was a dummy negative value.
>
> 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.

[Severity: Medium]
Is this paragraph still accurate? The diff doesn't touch mptcp_inq_hint().
At the parent commit ("mptcp: drop the cant_coalesce CB field") it already
reads:

static unsigned int mptcp_inq_hint(const struct sock *sk)
{
	const struct mptcp_sock *msk = mptcp_sk(sk);
	u64 hint_val = mptcp_data_avail(msk);

That change came from the earlier "mptcp: fix TFO SIOCINQ overflow in
mptcp_inq_hint" patch in this series, which has its own Fixes: tag. This
text looks left over from before that fix was split out.

The message also leaves out the real dependency. This patch no longer
advances the head skb's map_seq past consumed bytes, so the old
ack_seq - map_seq hint would over-report. That means this patch depends
on the earlier fix.

Could the commit message drop the mptcp_inq_hint() claims and mention
the dependency instead?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dd612e9b2b262..0666bbfdf2cb8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -415,19 +414,34 @@ 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;
>  
> +	/* Detect mapping boundary: skb spans multiple mappings */
> +	if (unlikely(offset > 0 &&
> +		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
> +		     subflow->map_subflow_seq)) {

[Severity: Medium]
Should this comparison use before()? Relative subflow sequence numbers
wrap after 4 GiB on one subflow. validate_mapping() compares the same
fields in a wrap-safe way:

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {

With a plain u32 '<', this check can go wrong in two ways near the wrap
point.

In the first case, the skb starts before the wrap (relative seq
0xFFFFFFF0) and the new mapping starts after it (map_subflow_seq 0x10).
The test is false, so the prefix is kept. map_seq then becomes
mptcp_subflow_get_mapped_dsn() - offset, which gives the old-mapping bytes
DSNs from the current mapping.

If that range is above msk->ack_seq, mptcp_data_queue_ofo() can accept
those bytes for a DSN range that isn't filled yet. They can then reach
userspace in place of the real data.

In the second case, the mapping starts before the wrap and the skb starts
inside it after the wrap. A partially overlapping retransmit from an
honest peer (offset > 0) then wrongly takes the skb_pull() path below.

The condition is the same at the end of the series.

> +		pr_debug("spans mappings seq=%u map_subflow_seq=%u offset=%d ssn_offset=%u\n",
> +			 TCP_SKB_CB(skb)->seq, subflow->map_subflow_seq,
> +			 offset, subflow->ssn_offset);
> +
> +		/* prefix bytes belong to old mapping, remove them and start
> +		 * from current mapping
> +		 */
> +		WARN_ON_ONCE(offset >= skb->len);
> +		skb_pull(skb, offset);

[Severity: Critical]
Can this skb_pull() hit the BUG() in __skb_pull() when the subflow skb is
non-linear?

__mptcp_move_skbs_from_subflow() computes offset as
tp->copied_seq - TCP_SKB_CB(skb)->seq. That is a count of payload bytes
and has no relation to skb_headlen(). skb_pull() only checks
len > skb->len, and then:

include/linux/skbuff.h:__skb_pull() {
	...
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
	...
		BUG();
	}
	...
}

TCP rx skbs usually carry their payload in page frags. This is true for
loopback, veth, GRO frags, header-split and page-pool drivers, virtio_net
mergeable buffers, and skbs merged by tcp_try_coalesce(). After the TCP
header pull, the linear part holds few or no payload bytes.

The WARN_ON_ONCE() above never fires, because the caller already ensures
offset < skb->len.

It looks like a remote peer could trigger this on an established
connection. The peer sends a segment with seq = rcv_nxt - N and length
N + M, carrying a DSS mapping with subflow_seq = rel(rcv_nxt) and
data_len = M:

tcp_data_queue()
  queues the partial packet untrimmed
get_mapping_status()->validate_mapping()
  accepts the mapping, since ssn == map_subflow_seq
subflow_data_ready()->mptcp_data_ready()->...
  __mptcp_move_skbs_from_subflow()
    mptcp_init_skb(ssk, skb, N)
      skb_pull(skb, N)  <- BUG() in softirq for a paged skb

There is a second way in. mptcp_subflow_discard_data() can leave
copied_seq in the middle of an skb at the end of a duplicate mapping.
The next mapping then starts inside that skb with offset > 0.

The old code never changed the skb boundaries; it kept the prefix in
MPTCP_SKB_CB(skb)->offset. So this regression appears to be new with this
patch. The same code is still there at the end of the series, after
"mptcp: sync mptcp skb cb layout with tcp one".

Switching to pskb_pull() alone may not be enough, since it can fail on
allocation in BH and would need an error path. Would a paged-safe head
trim work here? Another option is to track the prefix without changing
the skb.

> +		offset = 0;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn

Re: [PATCH mptcp-next v21 8/9] mptcp: sync mptcp skb cb layout with tcp one

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] A remote peer can trigger a kernel BUG() in mptcp_init_skb()
  (net/mptcp/protocol.c:443) by sending crafted segments.
- [High] The mapping-boundary check in mptcp_init_skb()
  (net/mptcp/protocol.c:432-434) compares wrapping u32 relative subflow…
- [Low] The commit message of "mptcp: sync mptcp skb cb layout with tcp
  one" describes a change this diff does not make: "A mapping boundary…

--

mptcp: sync mptcp skb cb layout with tcp one

This commit changes the MPTCP skb control block layout. map_seq and end_seq
become u32 and sit at the same offsets as the matching tcp_skb_cb fields. A
map_seq64 field is added to carry the full 64-bit DSN, along with a flags
field and a build-time layout check.

The receive path now uses 32-bit before()/after() for most comparisons. It
keeps map_seq64 for window admission and coalescing.

> In mptcp_init_skb(), map_seq64 is computed as mptcp_subflow_get_mapped_dsn()
> minus offset, and map_seq is the lower 32 bits. A mapping boundary check
> detects when the skb spans multiple DSS mappings (offset > 0 and skb starts
> before the current mapping). In that case, the consumed prefix bytes belong
> to an old mapping and are removed via skb_pull() before setting map_seq.

[Severity: Low]
Is this paragraph accurate for this patch?

The mapping boundary check, the pr_debug("spans mappings ...") and the
skb_pull(skb, offset) in mptcp_init_skb() are unchanged context in this diff.
They were added by the preceding commit "mptcp: remove CB offset field",
whose changelog has almost the same paragraph.

In this diff, mptcp_init_skb() only gains the map_seq64, map_seq and flags
assignments and the comment update.

Could this paragraph be dropped, or limited to the map_seq64/map_seq split?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0666bbfdf2cb8..705b28219883f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -437,11 +446,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
> +	 * value; 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;

[Severity: Critical]
This computation depends on the skb_pull() just above it. That code came from
"mptcp: remove CB offset field" and is not part of this diff, but the
changelog here describes it:

net/mptcp/protocol.c:mptcp_init_skb() {
    ...
	if (unlikely(offset > 0 &&
		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
		     subflow->map_subflow_seq)) {
		...
		WARN_ON_ONCE(offset >= skb->len);
		skb_pull(skb, offset);
		offset = 0;
	}
    ...
}

Can a remote peer reach the BUG() in __skb_pull() through this path?
skb_pull() only handles the linear area, and __skb_pull() does:

	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}

After TCP pulls its header, the payload of an rx skb is often in page frags
(header split, GRO, loopback/veth). So skb_headlen() can be smaller than
offset. The caller, __mptcp_move_skbs_from_subflow(), only checks offset
against skb->len.

One possible sequence:

1. The peer sends subflow data [0,1000) with mapping M1. MPTCP consumes it,
   so copied_seq = rcv_nxt = 1000.

2. The peer sends a segment with subflow seq 500 and length 1500. Its DSS
   mapping starts at relative ssn 1000.

3. tcp_data_queue() takes the "Partial packet, seq < rcv_next < end_seq"
   branch and queues the skb without trimming it.

4. validate_mapping() checks copied_seq against the mapping but not
   TCP_SKB_CB(skb)->seq, so it accepts the mapping.

5. __mptcp_move_skbs_from_subflow() computes offset = 500 and calls
   mptcp_init_skb(). The check 500 < 1000 is true, so skb_pull(skb, 500)
   runs on an skb whose headlen is below 500.

This code is still the same at the end of the series ("mptcp: defer
read_sock cleanup to mptcp_worker").

Would pskb_pull() with a NULL check, a frag-aware head trim, or keeping the
offset instead of pulling be safer here?

[Severity: High]
Separately, the same boundary check uses a plain '<' on wrapping u32
relative subflow sequence numbers:

	TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
	subflow->map_subflow_seq

validate_mapping() compares the same values with the wrap-safe helper:

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {

Does this check go wrong once the relative subflow sequence wraps past 2^32
(4 GiB on one subflow)? The map_seq64 and map_seq values set here are only
correct if the check decides properly whether the prefix belongs to the
current mapping.

First case: the mapping starts at rel 0x50 and an overlapping skb starts at
rel 0xFFFFFF00. Then 0xFFFFFF00 < 0x50 is false. The old-mapping prefix is
kept, and map_seq64 = mapped_dsn - offset gives those bytes DSNs they do not
carry.

If that skb goes to the msk OFO queue, mptcp_data_queue_ofo() can drop the
real data for that range as MPTCP_MIB_DUPDATA. Alternatively,
__mptcp_ofo_queue() can later deliver the wrong prefix bytes to the reader.

Second case: the mapping starts at rel 0xFFFFFF00 and spans the wrap, and a
partially overlapping skb inside it starts at rel 0x10. Then
0x10 < 0xFFFFFF00 is true. skb_pull() runs on an skb that needs no pull and
can hit the __skb_pull() BUG() described above, even with a well-behaved
peer.

Should this be before(TCP_SKB_CB(skb)->seq - subflow->ssn_offset,
subflow->map_subflow_seq) instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn

Re: [PATCH mptcp-next v21 9/9] mptcp: defer read_sock cleanup to mptcp_worker

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch removes the backlog flush (`if
  (!list_empty(&msk->backlog_list)) mptcp_move_skbs(sk);`) from…
- [Low] Nothing in the tree as submitted can reach the new deferred-
  cleanup machinery: msk->read_copied, MPTCP_WORK_READ_COMPLETE,…
- [Low] The inline and deferred post-read cleanup sequences in
  net/mptcp/protocol.c now differ, and the extra step in the inline one…

--

mptcp: defer read_sock cleanup to mptcp_worker

This commit prepares __mptcp_read_sock() for a future BH caller. When the
socket is not owned by the user, it defers mptcp_rcv_space_adjust() and
mptcp_cleanup_rbuf() to mptcp_worker() and records the consumed bytes in
msk->read_copied. It also removes the backlog flush from mptcp_recv_skb().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 705b28219883f..b15100a67672d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4648,9 +4668,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) {

[Severity: Medium]
Does removing this flush change behaviour for the existing process context
callers?

The commit message says the flush is "covered by mptcp_release_cb()".
That only happens at release_sock() time, though. Both in-tree callers
hold lock_sock() for the whole read:

net/sunrpc/svcsock.c:svc_tcp_recvfrom() {
    ...
	lock_sock(sock->sk);
	len = sock->ops->read_sock(sock->sk, &desc, svc_tcp_recv_actor);
    ...
}

While the msk is owned, subflow data that arrives through
mptcp_data_ready() goes onto msk->backlog_list, not sk_receive_queue.

Before this patch, mptcp_recv_skb() moved that data into the receive
queue, and the read loop consumed it in the same call. Now the data stays
in the backlog until mptcp_release_cb() spools it and calls
sk_data_ready(), so the reader needs another pass.

For svc_tcp_recvfrom(), that can mean a partial record and another trip
through XPT_DATA.

Can a non-blocking mptcp_splice_read() with an empty receive queue now
return -EAGAIN while data is waiting in msk->backlog_list?

net/mptcp/protocol.c:mptcp_splice_read() {
    ...
			if (!timeo) {
				ret = -EAGAIN;
				break;
			}
    ...
}

mptcp_recvmsg() still flushes the backlog inside its loop:

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;
		}
    ...
}

So recvmsg and read_sock now behave differently on the same socket.

Only the future BH caller needs the flush removed, because it would
already hold mptcp_data_lock(), which mptcp_move_skbs() takes. Could the
flush be kept for the sock_owned_by_user() case?

[ ... ]

> @@ -4706,11 +4724,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]
Now that the backlog flush is gone from mptcp_recv_skb(), can this call
ever eat an skb?

The read loop already calls mptcp_eat_recv_skb() on every fully consumed
skb. It only exits early when count <= 0 or count < data_len, and in both
cases the head skb still has offset < len. Otherwise, the queue is either
empty or the next skb starts at copied_seq.

sk_receive_queue cannot grow while the msk is owned, so this call looks
like a no-op.

The commit message says:

    The mptcp_recv_skb() call remains in the sock_owned_by_user() branch
    for inline cleanup.

mptcp_read_complete() leaves out this step, so the inline and deferred
cleanup sequences now differ. Should this call be dropped so the two
paths match?

> +			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);

[Severity: Low]
Can anything in the tree reach this branch?

Every current caller of mptcp_read_sock() holds lock_sock(), so
sock_owned_by_user() is always true for them. Those callers are
mptcp_splice_read() and svc_tcp_recvfrom().

The other sock->ops->read_sock users never get an MPTCP socket:

  - KCM rejects sk_protocol != IPPROTO_TCP.
  - The sockmap strparser requires sk_is_tcp().
  - nvme-tcp only creates IPPROTO_TCP sockets.

That leaves msk->read_copied, MPTCP_WORK_READ_COMPLETE and
mptcp_read_complete() with no user, and nothing in the tree can exercise
them. Would it make sense to carry this patch in the TLS-over-MPTCP series
that adds the BH caller?

The commit message also names two functions that don't seem to exist:

    where it may be reached from BH context (via tls_strp_read_copyin() ->
    sk->sk_read_sock()).

There is no sk_read_sock in the tree. tls_strp_read_copyin() calls
tcp_read_sock() directly:

net/tls/tls_strp.c:tls_strp_read_copyin() {
    ...
	/* sk should be locked here, so okay to do read_sock */
	tcp_read_sock(strp->sk, &desc, tls_strp_copyin);
    ...
}

The second one is in this sentence:

    Currently, the only callers are mptcp_splice_read() and
    svc_tcp_read_msg()

There is no svc_tcp_read_msg() in net/sunrpc. Is svc_tcp_recvfrom() the
caller that was meant?

> +		}
>  	}
>  out:
>  	return copied;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793870710.git.tanggeliang%40kylinos.cn