[PATCH mptcp-next v15 00/11] Reduce the differences between TCP and MPTCP for TLS usage

[PATCH mptcp-next v15 00/11] Reduce the differences between TCP and MPTCP for TLS usage

[Geliang Tang]

From: Geliang Tang <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().

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/

Gang Yan (1):
  mptcp: update mptcp_check_readable

Geliang Tang (5):
  mptcp: align FIN handling with TCP via SOCK_DONE
  mptcp: implement peek_len for proto_ops
  mptcp: add sendmsg_locked to proto_ops
  mptcp: track app-limited state in mptcp_sendmsg
  selftests: mptcp: sockopt: check app_limited

Paolo Abeni (5):
  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
  mptcp: defer read_sock cleanup to mptcp_worker

 net/mptcp/fastopen.c                          |  17 +-
 net/mptcp/protocol.c                          | 365 ++++++++++++------
 net/mptcp/protocol.h                          |  21 +-
 net/mptcp/subflow.c                           |  23 ++
 .../selftests/net/mptcp/mptcp_sockopt.c       |   5 +
 5 files changed, 309 insertions(+), 122 deletions(-)

-- 
2.55.0

[PATCH mptcp-next v15 01/11] mptcp: drop the mptcp_ooo_try_coalesce() helper

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

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

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

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

[PATCH mptcp-next v15 02/11] 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 an explicit test in __mptcp_move_skb(). Performance
wise this trades a conditional in the fast path - in __mptcp_try_coalesce()
- with a similar one in __mptcp_move_skb() and a couple more in slow paths.

After this change the user-space will always observe consistent sequence
numbers in the receive queue, even in the TFO dummy mapping case.

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 | 46 +++++++++++++++++++++++++++++++++++++++++---
 net/mptcp/protocol.h |  4 +++-
 net/mptcp/subflow.c  | 10 ++++++++++
 4 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index f717750906ff..e18b31028e7e 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -9,6 +9,7 @@
 void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
 					      struct request_sock *req)
 {
+	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
 	struct sk_buff *skb;
 	struct tcp_sock *tp;
@@ -49,15 +50,16 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 	MPTCP_SKB_CB(skb)->end_seq = 0;
 	MPTCP_SKB_CB(skb)->offset = 0;
 	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
-	MPTCP_SKB_CB(skb)->cant_coalesce = 1;
 
 	mptcp_data_lock(sk);
 	DEBUG_NET_WARN_ON_ONCE(sock_owned_by_user_nocheck(sk));
 
+	msk = mptcp_sk(sk);
+	msk->rcvd_dummy_seq = true;
 	mptcp_borrow_fwdmem(sk, skb);
 	skb_set_owner_r(skb, sk);
 	__skb_queue_tail(&sk->sk_receive_queue, skb);
-	mptcp_sk(sk)->bytes_received += skb->len;
+	msk->bytes_received += skb->len;
 
 	sk->sk_data_ready(sk);
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 528a72da646c..96deaed109a2 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,27 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
 	skb_dst_drop(skb);
 }
 
+/* Caller must hold the msk data lock */
+void __mptcp_sync_rcv_sequence(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sk_buff *skb;
+	u32 offset;
+
+	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;
+
+	offset = MPTCP_SKB_CB(skb)->offset;
+	MPTCP_SKB_CB(skb)->map_seq = msk->ack_seq - skb->len + offset;
+	MPTCP_SKB_CB(skb)->end_seq = msk->ack_seq;
+}
+
 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 +465,17 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 
 	mptcp_borrow_fwdmem(sk, skb);
 
+	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+		/* Ensure we see the updated ack_seq after seeing the flag */
+		smp_rmb();
+
+		/* 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:
@@ -2394,11 +2424,14 @@ 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;
 
 	skb = skb_peek(&sk->sk_receive_queue);
 	if (skb) {
-		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
+		if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+			return skb->len - MPTCP_SKB_CB(skb)->offset;
 
+		hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
 		if (hint_val >= INT_MAX)
 			return INT_MAX;
 
@@ -3930,6 +3963,13 @@ static void mptcp_release_cb(struct sock *sk)
 			__mptcp_error_report(sk);
 		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
 			__mptcp_sync_sndbuf(sk);
+		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+			/* Ensure we see the updated ack_seq after seeing
+			 * the flag
+			 */
+			smp_rmb();
+			__mptcp_sync_rcv_sequence(sk);
+		}
 	}
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..609b572f8eb0 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]))
@@ -327,6 +327,7 @@ struct mptcp_sock {
 	u32		token;
 	unsigned long	flags;
 	unsigned long	cb_flags;
+	bool		rcvd_dummy_seq;
 	bool		recovery;		/* closing subflow write queue reinjected */
 	bool		can_ack;
 	bool		fully_established;
@@ -1193,6 +1194,7 @@ void mptcp_event_pm_listener(const struct sock *ssk,
 			     enum mptcp_event_type event);
 bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
 
+void __mptcp_sync_rcv_sequence(struct sock *sk);
 void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
 					      struct request_sock *req);
 int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..bf6ba235c268 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,14 @@ 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);
+
+	if (!sock_owned_by_user(sk)) {
+		__mptcp_sync_rcv_sequence(sk);
+	} else {
+		/* Ensure ack_seq is visible before setting the flag */
+		smp_wmb();
+		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 v15 03/11] mptcp: remove CB offset field

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

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

The newly introduce `copied_seq` field is always accessed under the msk
socket lock, delegating the synchronization with IASN to 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.

Note that the explicit synchronization in __mptcp_move_skb() is needed to
ensure that the TFO skb in the receive queue got its map_seq synched
before the next skb lands into the receive queue when spooling the backlog
at mptcp_release_cb() time, as the release CB synchronization will happen
later.

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

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

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

Note: mptcp_iasn() assumes ack_seq and bytes_received advance in lockstep.
If mptcp_check_data_fin() increments ack_seq for a DATA_FIN before the
deferred SYNC_SEQ sync runs, copied_seq ends up one byte past the real
IASN.  In practice this requires a DATA_FIN to arrive before the MPC third
ACK on a passive TFO socket, which cannot happen because the connection
is not yet established.

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 |   9 ++--
 net/mptcp/protocol.c | 107 +++++++++++++++++++------------------------
 net/mptcp/protocol.h |   8 +++-
 net/mptcp/subflow.c  |  13 ++++++
 4 files changed, 74 insertions(+), 63 deletions(-)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index e18b31028e7e..421a50a85547 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -45,10 +45,11 @@ 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;
 
 	mptcp_data_lock(sk);
@@ -56,6 +57,8 @@ 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->tfo_skb_len = skb->len;
 	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 96deaed109a2..f2840c8d9c82 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	int limit = READ_ONCE(sk->sk_rcvbuf);
 
 	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
-	    MPTCP_SKB_CB(from)->offset ||
 	    ((to->len + from->len) > (limit >> 3)) ||
 	    !skb_try_coalesce(to, from, fragstolen, delta))
 		return false;
@@ -415,8 +414,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	skb_set_owner_r(skb, sk);
 }
 
-static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
-			   int copy_len)
+static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
@@ -425,9 +423,9 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int 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);
@@ -441,7 +439,6 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct sk_buff *skb;
-	u32 offset;
 
 	if (likely(!msk->rcvd_dummy_seq))
 		return;
@@ -452,9 +449,8 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
 	if (!skb)
 		return;
 
-	offset = MPTCP_SKB_CB(skb)->offset;
-	MPTCP_SKB_CB(skb)->map_seq = msk->ack_seq - skb->len + offset;
-	MPTCP_SKB_CB(skb)->end_seq = msk->ack_seq;
+	MPTCP_SKB_CB(skb)->map_seq = mptcp_iasn(msk) - skb->len;
+	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)
@@ -468,6 +464,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
 		/* Ensure we see the updated ack_seq after seeing the flag */
 		smp_rmb();
+		msk->copied_seq += mptcp_iasn(msk);
 
 		/* Be sure to sync the eventual fastopen dummy mapping before
 		 * any other skb lands into the msk.
@@ -502,10 +499,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;
 	}
 
@@ -863,7 +856,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);
@@ -924,14 +917,9 @@ 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_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;
+				 (int)(msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq));
 			__skb_queue_tail(&sk->sk_receive_queue, skb);
 		}
 		msk->bytes_received += end_seq - msk->ack_seq;
@@ -2215,33 +2203,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);
@@ -2260,14 +2239,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 {
@@ -2426,22 +2403,20 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
 	const struct sk_buff *skb;
 	u64 hint_val;
 
-	skb = skb_peek(&sk->sk_receive_queue);
-	if (skb) {
-		if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
-			return skb->len - MPTCP_SKB_CB(skb)->offset;
-
-		hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
-		if (hint_val >= INT_MAX)
-			return INT_MAX;
-
-		return (unsigned int)hint_val;
+	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+		skb = skb_peek(&sk->sk_receive_queue);
+		return skb ? skb->len : 0;
 	}
 
-	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
+	hint_val = READ_ONCE(msk->ack_seq) - msk->copied_seq;
+	if (hint_val >= INT_MAX)
+		return INT_MAX;
+
+	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,
@@ -2450,6 +2425,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;
 
@@ -2468,6 +2444,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;
@@ -2477,7 +2458,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)
@@ -2487,8 +2468,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()
@@ -2531,6 +2515,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);
@@ -3713,11 +3701,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	msk->bytes_retrans = 0;
 	msk->rcvspace_init = 0;
 	msk->fastclosing = 0;
+	msk->tfo_skb_len = 0;
 	mptcp_init_rtt_est(msk);
 
 	/* for fallback's sake */
 	WRITE_ONCE(msk->ack_seq, 0);
 	atomic64_set(&msk->rcv_wnd_sent, 0);
+	msk->copied_seq = 0;
 
 	WRITE_ONCE(sk->sk_shutdown, 0);
 	sk_error_report(sk);
@@ -3968,6 +3958,7 @@ static void mptcp_release_cb(struct sock *sk)
 			 * the flag
 			 */
 			smp_rmb();
+			msk->copied_seq += mptcp_iasn(msk);
 			__mptcp_sync_rcv_sequence(sk);
 		}
 	}
@@ -4641,7 +4632,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;
@@ -4683,11 +4674,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 609b572f8eb0..1cb7989b6fb0 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;
 };
 
@@ -306,6 +305,7 @@ struct mptcp_sock {
 	u64		bytes_sent;
 	u64		snd_nxt;
 	u64		bytes_received;
+	u64		copied_seq;
 	u64		ack_seq;
 	atomic64_t	rcv_wnd_sent;
 	u64		rcv_data_fin_seq;
@@ -325,6 +325,7 @@ struct mptcp_sock {
 	u32		last_ack_recv;
 	unsigned long	timer_ival;
 	u32		token;
+	u32		tfo_skb_len;
 	unsigned long	flags;
 	unsigned long	cb_flags;
 	bool		rcvd_dummy_seq;
@@ -884,6 +885,11 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
 int mptcp_sched_get_send(struct mptcp_sock *msk);
 int mptcp_sched_get_retrans(struct mptcp_sock *msk);
 
+static inline u64 mptcp_iasn(const struct mptcp_sock *msk)
+{
+	return msk->ack_seq - msk->bytes_received + msk->tfo_skb_len;
+}
+
 static inline u64 mptcp_data_avail(const struct mptcp_sock *msk)
 {
 	return READ_ONCE(msk->bytes_received) - READ_ONCE(msk->bytes_consumed);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index bf6ba235c268..ebba77f05a42 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -537,6 +537,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
 
 	if (!sock_owned_by_user(sk)) {
+		/* User space could have already read partially the TFO skb */
+		msk->copied_seq += subflow->iasn;
 		__mptcp_sync_rcv_sequence(sk);
 	} else {
 		/* Ensure ack_seq is visible before setting the flag */
@@ -559,6 +561,17 @@ static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
 		WRITE_ONCE(msk->snd_una, subflow->idsn + 1);
 		WRITE_ONCE(msk->wnd_end, subflow->idsn + 1 + tcp_sk(ssk)->snd_wnd);
 		subflow_set_remote_key(msk, subflow, mp_opt);
+	} else {
+		/* Fallback (no MPTCP options at SYN-ACK): initialise the
+		 * sequence space to 0 so that skb map_seq starts from 0
+		 * to match msk->copied_seq.  Safe without deferral because
+		 * mptcp_data_lock() is already held.
+		 */
+		subflow->map_seq = 0;
+		subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq -
+					   subflow->ssn_offset;
+		WRITE_ONCE(msk->ack_seq, 0);
+		msk->copied_seq = 0;
 	}
 
 	if (!sock_owned_by_user(sk)) {
-- 
2.55.0

[PATCH mptcp-next v15 04/11] mptcp: sync mptcp skb cb layout with tcp one

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

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

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

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

Add build time check to ensure the latter invariant.

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

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 421a50a85547..0d339dd454f9 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;
 	msk->tfo_skb_len = skb->len;
 	mptcp_borrow_fwdmem(sk, skb);
 	skb_set_owner_r(skb, sk);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f2840c8d9c82..01eb7fb314e7 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,21 @@ 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);
 
-	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 +352,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 +364,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(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 +406,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);
@@ -421,11 +428,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);
@@ -449,13 +458,14 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
 	if (!skb)
 		return;
 
-	MPTCP_SKB_CB(skb)->map_seq = mptcp_iasn(msk) - skb->len;
+	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_iasn(msk) - 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;
 
@@ -473,7 +483,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 			__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)) {
@@ -491,14 +501,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
 		skb_set_owner_r(skb, sk);
 		__skb_queue_tail(&sk->sk_receive_queue, skb);
 		return true;
-	} else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
+	} else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
 		mptcp_data_queue_ofo(msk, skb);
 		return false;
 	}
 
 	/* Partial packet */
-	if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
-		copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
+	if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
+		copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
 		goto insert;
 	}
 
@@ -893,37 +903,37 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
 	struct sk_buff *skb, *tail;
+	u32 seq_delta, ack_seq;
 	bool moved = false;
 	struct rb_node *p;
-	u64 end_seq;
 
 	p = rb_first(&msk->out_of_order_queue);
 	pr_debug("msk=%p empty=%d\n", msk, RB_EMPTY_ROOT(&msk->out_of_order_queue));
 	while (p) {
+		ack_seq = msk->ack_seq;
 		skb = rb_to_skb(p);
-		if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq))
+		if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
 			break;
 
 		p = rb_next(p);
 		rb_erase(&skb->rbnode, &msk->out_of_order_queue);
 
-		if (unlikely(!after64(MPTCP_SKB_CB(skb)->end_seq,
-				      msk->ack_seq))) {
+		if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
 			mptcp_drop(sk, skb);
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
 			continue;
 		}
 
-		end_seq = MPTCP_SKB_CB(skb)->end_seq;
+		seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;
 		tail = skb_peek_tail(&sk->sk_receive_queue);
 		if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
-			pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
-				 MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
-				 (int)(msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq));
+			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;
@@ -2212,7 +2222,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
 	int copied = 0;
 
 	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
-		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
+		u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;
 		u32 data_len = skb->len - offset;
 		u32 count;
 		int err;
@@ -4632,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 = 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;
@@ -4882,11 +4892,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 1cb7989b6fb0..fc39fd733860 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 v15 05/11] mptcp: defer read_sock cleanup to mptcp_worker

[Geliang Tang]

From: Paolo Abeni <pabeni@redhat.com>

This is preparatory work for an upcoming kTLS-over-MPTCP series. When
MPTCP carries TLS, __mptcp_read_sock() will be reached from BH context
(via tls_strp_read_copyin() -> sk->sk_read_sock()), where calling
mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() inline is not safe:
both expect process context.

Move both calls to mptcp_worker(): __mptcp_read_sock() accumulates the
consumed byte count in msk->read_copied and signals the worker through
a new MPTCP_WORK_READ_COMPLETE bit. The worker drains the accumulator
under mptcp_data_lock() and runs the rcvbuf autotuning and RDB update
in process context. To prevent integer overflow when read_copied wraps
to negative values, mptcp_read_complete() only calls mptcp_cleanup_rbuf()
when read_copied > 0.

Since the worker now owns the post-read housekeeping, remove the backlog
flush from mptcp_recv_skb() (it ran mptcp_move_skbs() which is already
covered by mptcp_release_cb() when the worker calls release_sock()) and
drop the msk_owned_by_me() assertion from __mptcp_read_sock() (the BH
caller does not own the msk lock). Also drop the now-redundant
mptcp_recv_skb() call after the main loop in __mptcp_read_sock(): its
return value and offset output were unused, and fully-consumed SKBs are
already eaten by mptcp_eat_recv_skb() within the loop.

Initialize msk->read_copied in __mptcp_init_sock() and reset it in
mptcp_disconnect() alongside the other per-connection counters, so a
socket does not carry a stale byte count across connections.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 01eb7fb314e7..3c3ad8991799 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3149,6 +3149,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;
@@ -3224,6 +3239,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);
@@ -3267,6 +3285,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);
@@ -3709,6 +3728,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	msk->bytes_received = 0;
 	msk->bytes_sent = 0;
 	msk->bytes_retrans = 0;
+	msk->read_copied = 0;
 	msk->rcvspace_init = 0;
 	msk->fastclosing = 0;
 	msk->tfo_skb_len = 0;
@@ -4638,9 +4658,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) {
@@ -4655,6 +4672,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)
@@ -4664,8 +4682,6 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 	int copied = 0;
 	u32 offset;
 
-	msk_owned_by_me(msk);
-
 	if (sk->sk_state == TCP_LISTEN)
 		return -ENOTCONN;
 	while ((skb = mptcp_recv_skb(sk, &offset)) != NULL) {
@@ -4696,11 +4712,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 	if (noack)
 		goto out;
 
-	mptcp_rcv_space_adjust(msk, copied);
-
+	/* The backlog flushing is only needed when some data is actually
+	 * moved and will take place in mptcp_release_cb() via
+	 * release_sock() in the worker.
+	 */
 	if (copied > 0) {
-		mptcp_recv_skb(sk, &offset);
-		mptcp_cleanup_rbuf(msk, copied);
+		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 fc39fd733860..a061609f6152 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;
 	u32		tfo_skb_len;
-- 
2.55.0

[PATCH mptcp-next v15 06/11] mptcp: align FIN handling with TCP via SOCK_DONE

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

TCP uses SOCK_DONE to mark FIN reception because RCV_SHUTDOWN alone is
ambiguous: it is also set by RST (via SHUTDOWN_MASK) without an ack_seq
increment. Mirror tcp_fin()/tcp_disconnect() in MPTCP:

- Set SOCK_DONE in mptcp_check_data_fin() next to RCV_SHUTDOWN.
- Use SOCK_DONE in mptcp_inq_hint() instead of TCP_CLOSE/RCV_SHUTDOWN,
  mirroring tcp_inq_hint().
- Check SOCK_DONE in mptcp_recvmsg() before sock_error() for clean-close
  path.
- Reset SOCK_DONE in mptcp_disconnect().

All SOCK_DONE access runs under msk socket ownership; existing
smp_mb__before_atomic() orders RCV_SHUTDOWN before state transitions.

Co-developed-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3c3ad8991799..7b0005d47bea 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -740,6 +740,7 @@ static void mptcp_check_data_fin(struct sock *sk)
 
 		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
 		smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
+		sock_set_flag(sk, SOCK_DONE);
 
 		switch (sk->sk_state) {
 		case TCP_ESTABLISHED:
@@ -2423,7 +2424,7 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
 		return INT_MAX;
 
 	if (!hint_val &&
-	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
+	    sock_flag(sk, SOCK_DONE))
 		return 1;
 
 	return (unsigned int)hint_val;
@@ -2495,6 +2496,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 			    !timeo)
 				break;
 		} else {
+			if (sock_flag(sk, SOCK_DONE))
+				break;
+
 			copied = sock_error(sk);
 			if (copied)
 				break;
@@ -3740,6 +3744,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	msk->copied_seq = 0;
 
 	WRITE_ONCE(sk->sk_shutdown, 0);
+	sock_reset_flag(sk, SOCK_DONE);
 	sk_error_report(sk);
 	return 0;
 }
-- 
2.55.0

[PATCH mptcp-next v15 07/11] mptcp: implement peek_len for proto_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add mptcp_inq() to compute the number of readable bytes at the MPTCP
level. It derives the count from ack_seq - copied_seq, returns 0 while
the connection is still handshaking (TCP_SYN_SENT/TCP_SYN_RECV), and
subtracts 1 once a FIN has been received, mirroring tcp_inq().

The subtraction is needed because MPTCP's ack_seq is incremented by 1
in mptcp_check_datafin() when the DATA_FIN is processed, consuming a
sequence number without carrying any payload data. Without subtracting 1,
peek_len would over-report by one byte after DATA_FIN.

READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
ack_seq and copied_seq because mptcp_inq() will be called from TLS
contexts with the socket lock held - taking the spinlock would deadlock.
On 32-bit systems a torn read can produce a wrapped subtraction (close to
2^64), which is caught by the INT_MAX clamp.

Wire mptcp_inq() into mptcp_peek_len() and assign .peek_len in both
mptcp_stream_ops and mptcp_v6_stream_ops. This is preparatory work for
an upcoming kTLS-over-MPTCP series: no in-tree consumer currently
reaches ->peek_len on an MPTCP socket (the only caller, strp_peek_len(),
cannot attach strparser to MPTCP; the existing TLS receive path calls
tcp_inq() directly rather than going through ->peek_len).

Note: mptcp_inq() intentionally differs from mptcp_inq_hint() when the
calculated value is 0 and SOCK_DONE is set. mptcp_inq_hint() returns 1
in this case to signal EOF to ioctl/cmsg users (mirroring tcp_inq_hint()),
while mptcp_inq() returns 0 because peek_len should report the actual
readable byte count (mirroring tcp_inq()).

Co-developed-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7b0005d47bea..ad11a96b83eb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4848,6 +4848,43 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 	return ret;
 }
 
+static int mptcp_inq(struct sock *sk)
+{
+	const struct mptcp_sock *msk = mptcp_sk(sk);
+	int answ;
+
+	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
+		answ = 0;
+	} else if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+		answ = 0;
+	} else {
+		bool sock_done = sock_flag(sk, SOCK_DONE);
+		u64 hint_val;
+
+		/* Ensure we see updated ack_seq and copied_seq after
+		 * checking MPTCP_SYNC_SEQ flag.
+		 */
+		smp_rmb();
+
+		hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
+		if (hint_val >= INT_MAX)
+			hint_val = INT_MAX;
+
+		answ = (unsigned int)hint_val;
+
+		/* Subtract 1, if FIN was received. Mirror tcp_inq() */
+		if (answ && sock_done)
+			answ--;
+	}
+
+	return answ;
+}
+
+static int mptcp_peek_len(struct socket *sock)
+{
+	return mptcp_inq(sock->sk);
+}
+
 static const struct proto_ops mptcp_stream_ops = {
 	.family		   = PF_INET,
 	.owner		   = THIS_MODULE,
@@ -4870,6 +4907,7 @@ static const struct proto_ops mptcp_stream_ops = {
 	.set_rcvlowat	   = mptcp_set_rcvlowat,
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
+	.peek_len	   = mptcp_peek_len,
 };
 
 static struct inet_protosw mptcp_protosw = {
@@ -4994,6 +5032,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
 	.set_rcvlowat	   = mptcp_set_rcvlowat,
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
+	.peek_len	   = mptcp_peek_len,
 };
 
 static struct proto mptcp_v6_prot;
-- 
2.55.0

[PATCH mptcp-next v15 08/11] mptcp: add sendmsg_locked to proto_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

MPTCP currently provides a standard sendmsg() implementation which
acquires and releases the socket lock internally. However, certain
upper layers need to call the sendmsg method while the socket lock
is already held.

Split the existing mptcp_sendmsg() into mptcp_sendmsg_locked() which
assumes the caller holds the socket lock, and a tiny wrapper
mptcp_sendmsg() that acquires the lock and calls the locked version.

Expose .sendmsg_locked in both mptcp_stream_ops and mptcp_v6_stream_ops.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ad11a96b83eb..086de4f3100e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2058,7 +2058,7 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
 	}
 }
 
-static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
+static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct page_frag *pfrag;
@@ -2070,8 +2070,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
 			  MSG_FASTOPEN | MSG_EOR;
 
-	lock_sock(sk);
-
 	mptcp_rps_record_subflows(msk);
 
 	if (unlikely(inet_test_bit(DEFER_CONNECT, sk) ||
@@ -2187,7 +2185,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	}
 
 out:
-	release_sock(sk);
 	return copied;
 
 do_error:
@@ -2200,6 +2197,17 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	goto out;
 }
 
+static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
+{
+	int ret;
+
+	lock_sock(sk);
+	ret = mptcp_sendmsg_locked(sk, msg, len);
+	release_sock(sk);
+
+	return ret;
+}
+
 static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied);
 
 static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
@@ -4908,6 +4916,7 @@ static const struct proto_ops mptcp_stream_ops = {
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
 	.peek_len	   = mptcp_peek_len,
+	.sendmsg_locked	   = mptcp_sendmsg_locked,
 };
 
 static struct inet_protosw mptcp_protosw = {
@@ -5033,6 +5042,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
 	.peek_len	   = mptcp_peek_len,
+	.sendmsg_locked	   = mptcp_sendmsg_locked,
 };
 
 static struct proto mptcp_v6_prot;
-- 
2.55.0

[PATCH mptcp-next v15 09/11] mptcp: track app-limited state in mptcp_sendmsg

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add mptcp_rate_check_app_limited() that walks every subflow of the
mptcp_sock and runs tcp_rate_check_app_limited() under each subflow's
socket lock. The helper bails out early when MPTCP still has unsent data
(msk->write_seq != msk->snd_nxt), because in that case the subflows may
look idle only because mptcp_sendmsg_frag() has not pushed data down yet
- e.g. due to a constrained MPTCP receive window - and marking them
app-limited would be wrong.

Invoke mptcp_rate_check_app_limited() from mptcp_sendmsg_locked() right
after the send-side setup, so the delivery-rate app_limited state stays
in sync with what the application actually has to send across each
subflow.

With this in place, subflow rate samples taken while the MPTCP connection
is genuinely application-limited are flagged as such, allowing congestion
controllers like BBR to ignore them when estimating bandwidth.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 086de4f3100e..7870b9be76c7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2058,6 +2058,24 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
 	}
 }
 
+static void mptcp_rate_check_app_limited(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct mptcp_subflow_context *subflow;
+
+	if (msk->write_seq != msk->snd_nxt)
+		return;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		bool slow;
+
+		slow = lock_sock_fast_nested(ssk);
+		tcp_rate_check_app_limited(ssk);
+		unlock_sock_fast(ssk, slow);
+	}
+}
+
 static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -2086,6 +2104,9 @@ static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
+	/* is sending application-limited? */
+	mptcp_rate_check_app_limited(sk);
+
 	if ((1 << sk->sk_state) & ~(TCPF_ESTABLISHED | TCPF_CLOSE_WAIT)) {
 		ret = sk_stream_wait_connect(sk, &timeo);
 		if (ret)
-- 
2.55.0

[PATCH mptcp-next v15 10/11] selftests: mptcp: sockopt: check app_limited

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

connect_one_server() in mptcp_sockopt exchanges only a few packets between
the client and server, then closes the socket. After such a small transfer
the application has nothing further to send, so the connection is, by
definition, application-limited.

Extend the TCP_INFO readback at the end of the function to assert
s.tcp_info.tcpi_delivery_rate_app_limited == 1.

Without the preceding commit, mptcp_sendmsg() never updates the per-subflow
app-limited state, and this field stays at 0 - the assertion would fail.
With it in place, the value is forced to 1, turning this into a regression
guard for the subflow-side application-limited accounting.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b8158a1198eb..f7a45b93cbe0 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -635,6 +635,11 @@ static void connect_one_server(int fd, int pipefd)
 	do_getsockopts(&s, fd, total, total);
 
 	assert(s.mptcpi_rcv_delta == (uint64_t)total);
+
+	/* app-limited accounting requires a recent kernel */
+	if (expect_all_features())
+		assert(s.tcp_info.tcpi_delivery_rate_app_limited == 1);
+
 	close(fd);
 }
 
-- 
2.55.0

[PATCH mptcp-next v15 11/11] mptcp: update mptcp_check_readable

[Geliang Tang]

From: Gang Yan <yangang@kylinos.cn>

This patch makes mptcp_check_readable() aligned with TCP, and renames it to
mptcp_stream_is_readable(). It will be used in the case of KTLS, because
'prot' will be modified, tls_sw_sock_is_readable() is expected to be called
from prot->sock_is_readable().

Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/protocol.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7870b9be76c7..b224b53c0888 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3550,9 +3550,11 @@ void __mptcp_unaccepted_force_close(struct sock *sk)
 	__mptcp_destroy_sock(sk);
 }
 
-static __poll_t mptcp_check_readable(struct sock *sk)
+static bool mptcp_stream_is_readable(struct sock *sk)
 {
-	return mptcp_epollin_ready(sk) ? EPOLLIN | EPOLLRDNORM : 0;
+	if (mptcp_epollin_ready(sk))
+		return true;
+	return sk_is_readable(sk);
 }
 
 static void mptcp_check_listen_stop(struct sock *sk)
@@ -4666,7 +4668,8 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
 		mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
 
 	if (state != TCP_SYN_SENT && state != TCP_SYN_RECV) {
-		mask |= mptcp_check_readable(sk);
+		if (mptcp_stream_is_readable(sk))
+			mask |= EPOLLIN | EPOLLRDNORM;
 		if (shutdown & SEND_SHUTDOWN)
 			mask |= EPOLLOUT | EPOLLWRNORM;
 		else
-- 
2.55.0

Re: [PATCH mptcp-next v15 07/11] mptcp: implement peek_len for proto_ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: race and TFO byte loss on MPTCP_SYNC_SEQ in mptcp_inq()
- [High] mptcp: mptcp_inq() clamps wrapped u64 underflow to INT_MAX instead of 0
--

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

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() and mptcp_peek_len() to report the number of
readable bytes at the MPTCP socket level via proto_ops.peek_len for both IPv4
and IPv6 MPTCP stream operations.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7b0005d47bead..ad11a96b83eba 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4848,6 +4848,43 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_inq(struct sock *sk)
> +{
> +	const struct mptcp_sock *msk = mptcp_sk(sk);
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
> +		answ = 0;
> +	} else if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		answ = 0;
> +	} else {
> +		bool sock_done = sock_flag(sk, SOCK_DONE);
> +		u64 hint_val;
> +
> +		/* Ensure we see updated ack_seq and copied_seq after
> +		 * checking MPTCP_SYNC_SEQ flag.
> +		 */
> +		smp_rmb();

[Severity: High]
Could the ordering of the MPTCP_SYNC_SEQ check and the sequence number reads
here race with subflow_set_remote_key() and mptcp_release_cb(), and also hide
queued TFO data?

On a passive MPTCP connection created before the third ACK arrives (such as a
passive TFO socket initialized via mptcp_sk_clone_init() and
mptcp_fastopen_subflow_synack_set_params()), mptcp_sk_clone_init() sets
sk->sk_state to TCP_ESTABLISHED while msk->ack_seq is 0 and msk->copied_seq
is -(u64)skb->len (or 0).

When the third ACK arrives in BH context while a reader holds the socket
lock, subflow_set_remote_key() writes msk->ack_seq before setting
MPTCP_SYNC_SEQ:

net/mptcp/subflow.c:subflow_set_remote_key() {
    ...
	WRITE_ONCE(msk->ack_seq, subflow->iasn);
	WRITE_ONCE(msk->can_ack, true);
	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);

	if (!sock_owned_by_user(sk)) {
		/* User space could have already read partially the TFO skb */
		msk->copied_seq += subflow->iasn;
		__mptcp_sync_rcv_sequence(sk);
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
    ...
}

And when the flag is cleared in mptcp_release_cb() (or __mptcp_move_skb()),
MPTCP_SYNC_SEQ is cleared before msk->copied_seq is updated:

net/mptcp/protocol.c:mptcp_release_cb() {
    ...
		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
			/* Ensure we see the updated ack_seq after seeing
			 * the flag
			 */
			smp_rmb();
			msk->copied_seq += mptcp_iasn(msk);
			__mptcp_sync_rcv_sequence(sk);
		}
    ...
}

Because mptcp_inq() tests MPTCP_SYNC_SEQ before reading msk->ack_seq and
msk->copied_seq, can it observe MPTCP_SYNC_SEQ == 0 and then read the new
msk->ack_seq (subflow->iasn) alongside the pre-IASN msk->copied_seq, causing
ack_seq - copied_seq to evaluate to subflow->iasn and clamp to INT_MAX?

In addition, when MPTCP_SYNC_SEQ is set on a passive TFO socket, returning 0
unconditionally ignores unread TFO payload already queued in
sk->sk_receive_queue, whereas mptcp_inq_hint() inspects the receive queue:

net/mptcp/protocol.c:mptcp_inq_hint() {
    ...
	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
		skb = skb_peek(&sk->sk_receive_queue);
		return skb ? skb->len : 0;
	}
    ...
}

> +
> +		hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +		if (hint_val >= INT_MAX)
> +			hint_val = INT_MAX;
> +
> +		answ = (unsigned int)hint_val;

[Severity: High]
What happens on 32-bit systems when a torn read of msk->ack_seq or
msk->copied_seq causes the unsigned subtraction to wrap below zero?

The commit message notes that on 32-bit systems a torn read can produce a
wrapped subtraction close to 2^64 which is caught by the INT_MAX clamp.
However, since hint_val is an unsigned u64, a wrapped negative difference
(near U64_MAX) satisfies hint_val >= INT_MAX and gets clamped to INT_MAX
rather than 0.

Would clamping a wrapped underflow to INT_MAX cause mptcp_peek_len() to
report 2 GiB of readable bytes when no data is queued?

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

Re: [PATCH mptcp-next v15 06/11] mptcp: align FIN handling with TCP via SOCK_DONE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: SOCK_DONE set on fallback TCP RST via subflow_is_done() masks sk_err
--

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

mptcp: align FIN handling with TCP via SOCK_DONE

This commit aligns MPTCP FIN reception handling with TCP by setting SOCK_DONE
in mptcp_check_data_fin(), checking it in mptcp_inq_hint() and mptcp_recvmsg()
before sock_error(), and resetting it in mptcp_disconnect().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3c3ad8991799a..7b0005d47bead 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -740,6 +740,7 @@ static void mptcp_check_data_fin(struct sock *sk)
>  
>  		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
>  		smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
> +		sock_set_flag(sk, SOCK_DONE);
>  
>  		switch (sk->sk_state) {
>  		case TCP_ESTABLISHED:
> @@ -2423,7 +2424,7 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
>  		return INT_MAX;
>  
>  	if (!hint_val &&
> -	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> +	    sock_flag(sk, SOCK_DONE))
>  		return 1;
>  
>  	return (unsigned int)hint_val;
> @@ -2495,6 +2496,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  			    !timeo)
>  				break;
>  		} else {
> +			if (sock_flag(sk, SOCK_DONE))
> +				break;
> +
>  			copied = sock_error(sk);
>  			if (copied)
>  				break;

[Severity: High]
Can checking sock_flag(sk, SOCK_DONE) before sock_error(sk) in
mptcp_recvmsg() mask a connection reset error on a fallback MPTCP connection
when the peer aborts with a TCP reset?

When a peer sends a TCP reset on a fallback subflow (msk->first),
tcp_reset() -> tcp_done_with_error() -> tcp_done() sets sk_err, sk_state,
and sk_shutdown on the subflow socket without setting SOCK_DONE, and invokes
subflow_state_change():

net/mptcp/subflow.c:subflow_state_change() {
	...
	if (mptcp_subflow_data_available(sk))
		mptcp_data_ready(parent, sk);
	subflow_sched_work_if_closed(mptcp_sk(parent), sk);
	...
}

In subflow_sched_work_if_closed(), subflow_is_done() checks:

net/mptcp/subflow.c:subflow_is_done() {
	return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
}

Because subflow_is_done(ssk) returns true on a TCP reset,
subflow_sched_work_if_closed() generates a synthetic data fin:

net/mptcp/subflow.c:subflow_sched_work_if_closed() {
	...
	if (__mptcp_check_fallback(msk) && subflow_is_done(ssk) &&
	    msk->first == ssk &&
	    mptcp_update_rcv_data_fin(msk, subflow->map_seq +
				      subflow->map_data_len, true))
		mptcp_schedule_work(sk);
	...
}

If the parent msk is owned by user context when subflow_error_report(ssk)
subsequently runs (or if mptcp_worker acquires the msk socket lock before
subflow_error_report() acquires mptcp_data_lock), error and state propagation
to the parent msk is deferred via MPTCP_ERROR_REPORT in msk->cb_flags,
leaving sk->sk_state in TCP_ESTABLISHED.

Before MPTCP_ERROR_REPORT is processed at the end of mptcp_release_cb(),
mptcp_check_data_fin() can run while sk->sk_state is still TCP_ESTABLISHED
(for example via __mptcp_move_skbs() at the start of mptcp_release_cb(),
mptcp_move_skbs() in mptcp_recvmsg() or mptcp_ioctl(), or in mptcp_worker()):

net/mptcp/protocol.c:mptcp_release_cb() {
	for (;;) {
		...
		if (spool_bl && __mptcp_move_skbs(sk, &skbs, &moved)) {
			/* notify ack seq update */
			mptcp_cleanup_rbuf(msk, 0);
			sk->sk_data_ready(sk);
		}
		...
	}
	if (flags) {
		...
		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
			__mptcp_error_report(sk);
	}
	...
}

When mptcp_check_data_fin() runs in that window, it increments msk->ack_seq
and sets SOCK_DONE on the parent msk. Once __mptcp_error_report() then
propagates sk->sk_err, wouldn't mptcp_recvmsg() hit the new
sock_flag(sk, SOCK_DONE) check before sock_error(sk) and return 0 instead of
the connection reset error, while mptcp_inq_hint() returns 1 instead of 0?

Should subflow_is_done() also check sock_flag(sk, SOCK_DONE) (or check for
the absence of sk->sk_err) so that an abortive reset on a fallback subflow
does not synthesize a clean data fin?

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

Re: [PATCH mptcp-next v15 05/11] mptcp: defer read_sock cleanup to mptcp_worker

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: msk->read_copied overflow in __mptcp_read_sock() skips rbuf cleanup
--

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

mptcp: defer read_sock cleanup to mptcp_worker

This commit defers mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() from
__mptcp_read_sock() to mptcp_worker() by accumulating consumed bytes in
msk->read_copied and scheduling work with MPTCP_WORK_READ_COMPLETE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 01eb7fb314e76..3c3ad8991799a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3149,6 +3149,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);
> +}

[ ... ]

> @@ -4696,11 +4712,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	if (noack)
>  		goto out;
>  
> -	mptcp_rcv_space_adjust(msk, copied);
> -
> +	/* The backlog flushing is only needed when some data is actually
> +	 * moved and will take place in mptcp_release_cb() via
> +	 * release_sock() in the worker.
> +	 */
>  	if (copied > 0) {
> -		mptcp_recv_skb(sk, &offset);
> -		mptcp_cleanup_rbuf(msk, copied);
> +		msk->read_copied += copied;
> +		set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
> +		mptcp_schedule_work(sk);
>  	}

[Severity: Medium]
Can accumulating copied into msk->read_copied in __mptcp_read_sock() without
saturating cause mptcp_read_complete() to skip receive buffer cleanup and
window updates when msk->read_copied wraps?

In net/mptcp/protocol.h, struct mptcp_sock defines read_copied as a signed
int. If multiple calls to __mptcp_read_sock() (for example via
mptcp_splice_read() or BH-context sk->sk_read_sock()) consume enough data
before mptcp_worker() runs mptcp_read_complete() that msk->read_copied
overflows and wraps to zero or a negative value, mptcp_read_complete() resets
msk->read_copied to 0 and skips mptcp_cleanup_rbuf(msk, read_copied).

At the same time, mptcp_rcv_space_adjust() in net/mptcp/protocol.c also
returns early when copied <= 0, and when given a large positive accumulated
read_copied it adds into another signed int without saturation:

net/mptcp/protocol.c:mptcp_rcv_space_adjust() {
	...
	if (copied <= 0)
		return;

	if (!msk->rcvspace_init)
		mptcp_rcv_space_init(msk, msk->first);

	msk->rcvq_space.copied += copied;
	...
}

Skipping mptcp_cleanup_rbuf() after draining the receive queue prevents
subflows from calling tcp_cleanup_rbuf() to send window-update ACKs, which can
stall the connection if the receive window was exhausted, while also skipping
receive buffer autotuning.

Would it make sense to use saturating addition (or a wider type) when
accumulating msk->read_copied and msk->rcvq_space.copied so thatread_copied
remains positive when mptcp_read_complete() drains it?

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

Re: [PATCH mptcp-next v15 02/11] mptcp: drop the cant_coalesce CB field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mptcp: race between subflow_set_remote_key() and mptcp_inq_hint()
- [Medium] mptcp: unlocked test_and_clear_bit() on cb_flags in __mptcp_move_skb()
- [Medium] mptcp: off-by-one TFO sequence sync if mptcp_check_data_fin() runs first
--

commit 95b8582af133b9f96581ec94b3c12f504f8d4881
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: drop the cant_coalesce CB field

This commit removes the cant_coalesce field from mptcp_skb_cb and instead
synchronizes the passive fastopen skb sequence numbers with the initial ack
sequence number once the third ack arrives. Synchronization is deferred via
MPTCP_SYNC_SEQ in cb_flags when the msk is owned by user space.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..96deaed109a2f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -438,6 +436,27 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }
>  
> +/* Caller must hold the msk data lock */
> +void __mptcp_sync_rcv_sequence(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sk_buff *skb;
> +	u32 offset;
> +
> +	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;
> +
> +	offset = MPTCP_SKB_CB(skb)->offset;
> +	MPTCP_SKB_CB(skb)->map_seq = msk->ack_seq - skb->len + offset;
> +	MPTCP_SKB_CB(skb)->end_seq = msk->ack_seq;
> +}
> +
>  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 +465,17 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		/* Ensure we see the updated ack_seq after seeing the flag */
> +		smp_rmb();
> +
> +		/* 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);
> +	}

[Severity: Medium]
Can this unlocked test_and_clear_bit() on msk->cb_flags race with non-atomic
modifications to msk->cb_flags on other CPUs?

When mptcp_move_skbs() or mptcp_release_cb() spools skbs from
msk->backlog_list, it drops mptcp_data_lock(sk) while sock_owned_by_user(sk)
remains true before calling __mptcp_move_skbs() -> __mptcp_move_skb():

net/mptcp/protocol.c:mptcp_move_skbs() {
	mptcp_data_lock(sk);
	while (mptcp_can_spool_backlog(sk, &skbs)) {
		mptcp_data_unlock(sk);
		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);

		mptcp_data_lock(sk);
	}
}

As a result, __mptcp_move_skb() calls __mptcp_sync_rcv_sequence(sk) without
holding mptcp_data_lock(sk) (contrary to the comment above
__mptcp_sync_rcv_sequence()), and executes an atomic test_and_clear_bit() on
msk->cb_flags without mptcp_data_lock(sk).

Meanwhile, softirq and timer paths holding mptcp_data_lock(sk) see
sock_owned_by_user(sk) == true and update other flags in the same
msk->cb_flags word using non-atomic operations (__set_bit() or |=), such as
in __mptcp_data_acked(), __mptcp_check_push(), mptcp_propagate_state(),
mptcp_retransmit_timer(), mptcp_subflow_process_delegated(),
mptcp_finish_join(), subflow_error_report(), and
mptcp_check_and_set_pending():

net/mptcp/protocol.c:__mptcp_data_acked() {
	if (!sock_owned_by_user(sk))
		__mptcp_clean_una(sk);
	else
		__set_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->cb_flags);
}

If a softirq or timer handler loads msk->cb_flags while MPTCP_SYNC_SEQ is
set, and __mptcp_move_skb() clears MPTCP_SYNC_SEQ before that handler stores
back the modified msk->cb_flags word, MPTCP_SYNC_SEQ will be restored to 1.

In this commit, a resurrected MPTCP_SYNC_SEQ bit causes mptcp_inq_hint() to
return only the first skb's remaining length rather than the full receive
queue length. In the later commit in this series ("mptcp: remove CB offset
field"), clearing the resurrected MPTCP_SYNC_SEQ bit a second time adds
mptcp_iasn(msk) to msk->copied_seq twice, corrupting msk->copied_seq.

> @@ -2394,11 +2424,14 @@ 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;
>  
>  	skb = skb_peek(&sk->sk_receive_queue);
>  	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +		if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +			return skb->len - MPTCP_SKB_CB(skb)->offset;
>  
> +		hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
>  		if (hint_val >= INT_MAX)
>  			return INT_MAX;

[Severity: Medium]
Could mptcp_inq_hint() observe MPTCP_SYNC_SEQ == 0 together with the updated
msk->ack_seq == subflow->iasn while the fastopen skb still has its dummy
map_seq?

On a passive fastopen socket, mptcp_fastopen_subflow_synack_set_params()
queues the skb onto sk->sk_receive_queue with MPTCP_SKB_CB(skb)->map_seq =
-skb->len while msk->ack_seq is 0.

When the third ack arrives in softirq context while user space holds
lock_sock(sk) (for instance during mptcp_ioctl(), mptcp_recvmsg(), or
mptcp_rcv_space_adjust()), subflow_set_remote_key() updates msk->ack_seq
first and sets MPTCP_SYNC_SEQ afterwards:

net/mptcp/subflow.c:subflow_set_remote_key() {
	WRITE_ONCE(msk->ack_seq, subflow->iasn);
	WRITE_ONCE(msk->can_ack, true);
	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);

	if (!sock_owned_by_user(sk)) {
		__mptcp_sync_rcv_sequence(sk);
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
}

Because mptcp_inq_hint() runs under lock_sock(sk) without mptcp_data_lock(sk)
and checks test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) before reading
msk->ack_seq, it can test MPTCP_SYNC_SEQ before set_bit() executes and then
read msk->ack_seq after WRITE_ONCE(msk->ack_seq, subflow->iasn) executes.

In that window, MPTCP_SKB_CB(skb)->map_seq is still -skb->len (plus offset),
so hint_val becomes subflow->iasn + skb->len - offset, causing
mptcp_inq_hint() to return a bogus value (clamped to INT_MAX or leaking the
lower bits of subflow->iasn) and corrupting the receive buffer autotuning
calculation in mptcp_rcv_space_adjust().

> @@ -3930,6 +3963,13 @@ static void mptcp_release_cb(struct sock *sk)
>  			__mptcp_error_report(sk);
>  		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
>  			__mptcp_sync_sndbuf(sk);
> +		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +			/* Ensure we see the updated ack_seq after seeing
> +			 * the flag
> +			 */
> +			smp_rmb();
> +			__mptcp_sync_rcv_sequence(sk);
> +		}
>  	}
>  }

[Severity: Medium]
What happens if mptcp_worker() runs mptcp_check_data_fin(sk) before
mptcp_release_cb(sk) processes MPTCP_SYNC_SEQ?

Suppose a passive fastopen socket has queued the fastopen skb with dummy
sequence numbers and msk->rcvd_dummy_seq = true, and mptcp_worker() is
running under lock_sock(sk). If softirq processes the third ack (carrying
MP_CAPABLE) followed by a zero-payload data_fin (with data_seq ==
subflow->iasn):

1. subflow_set_remote_key() sets msk->ack_seq = subflow->iasn and sets
   MPTCP_SYNC_SEQ in msk->cb_flags because sock_owned_by_user(sk) is true.

2. mptcp_incoming_options() or get_mapping_status() calls
   mptcp_update_rcv_data_fin(), setting msk->rcv_data_fin_seq = subflow->iasn
   and msk->rcv_data_fin = 1 without queuing an skb to msk->backlog_list.

3. Before release_sock(sk) calls mptcp_release_cb(sk), mptcp_worker() calls
   mptcp_check_data_fin(sk), which sees msk->ack_seq == msk->rcv_data_fin_seq
   and increments msk->ack_seq to subflow->iasn + 1:

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);
		WRITE_ONCE(msk->rcv_data_fin, 0);
}

4. When release_sock(sk) then invokes mptcp_release_cb(sk) ->
   __mptcp_sync_rcv_sequence(sk), msk->ack_seq is already subflow->iasn + 1.

Wouldn't __mptcp_sync_rcv_sequence() (and mptcp_iasn(msk) in the subsequent
commit "mptcp: remove CB offset field") then compute the fastopen sequence
off by 1 byte, causing mptcp_inq_hint() (and mptcp_inq() in commit "mptcp:
implement peek_len for proto_ops") to under-report the pending receive queue
data by 1 byte?

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

Re: [PATCH mptcp-next v15 04/11] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] mptcp: subtracting offset in mptcp_init_skb() corrupts OoO and coalescing
- [High] mptcp: double offset subtraction in mptcp_init_skb() on TCP fallback
- [High] mptcp: unlocked cb_flags and sequence sync in __mptcp_move_skb()
- [Medium] mptcp: race and TFO byte overcount in mptcp_inq_hint()
- [Medium] mptcp: missing rcvd_dummy_seq reset in mptcp_disconnect()
--

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

mptcp: sync mptcp skb cb layout with tcp one

This series removes the cant_coalesce and offset fields from struct
mptcp_skb_cb, tracks consumed receive bytes via msk->copied_seq, and
aligns the 32-bit sequence number and flags layout of struct mptcp_skb_cb
with struct tcp_skb_cb while keeping the 64-bit sequence number in
map_seq64.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..01eb7fb314e76 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -167,13 +167,14 @@ 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) ||
> -	    MPTCP_SKB_CB(from)->offset ||
> +	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;

[ ... ]

> @@ -424,21 +421,21 @@ 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;
>  
>  	/* 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);
> -	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_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]
When __mptcp_move_skbs_from_subflow() passes a non-zero offset to
mptcp_init_skb() (for example, after mptcp_subflow_discard_data() advances
tp->copied_seq past an already-received mapping in the prefix of a
multi-mapping skb, or when a partial-overlap subflow segment has
TCP_SKB_CB(skb)->seq < tp->copied_seq), the valid mapped payload starts at
skb->data + offset with data sequence number dsn =
mptcp_subflow_get_mapped_dsn(subflow), while skb->data[0 .. offset - 1]
contains discarded or unrelated subflow bytes.

By shifting map_seq64 and map_seq backwards to dsn - offset and removing
the offset check from __mptcp_try_coalesce(), what prevents the following
regressions when offset > 0?

1. If a preceding skb in backlog_list, out_of_order_queue, or
sk_receive_queue ends at dsn - offset, __mptcp_try_coalesce() sees
MPTCP_SKB_CB(from)->map_seq == MPTCP_SKB_CB(to)->end_seq and
MPTCP_SKB_CB(from)->map_seq64 == MPTCP_SKB_CB(to)->map_seq64 + to->len, and
skb_try_coalesce() appends the entire from buffer (including the leading
bytes in from->data[0 .. offset - 1]) onto to.

2. If the mapping at skb->data + offset is out-of-order (dsn >
msk->ack_seq) with dsn - offset <= msk->ack_seq, __mptcp_move_skb() sees
MPTCP_SKB_CB(skb)->map_seq64 <= msk->ack_seq and treats the out-of-order
skb as in-sequence or a partial packet, queuing it to sk->sk_receive_queue
and advancing msk->ack_seq so __mptcp_recvmsg_mskq() copies stale prefix
bytes to user space in place of [msk->ack_seq, dsn).

3. If dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the skb
into msk->out_of_order_queue at map_seq = (u32)(dsn - offset), where it can
drop or overwrite legitimate out-of-order segments in [dsn - offset, dsn)
as MPTCP_MIB_DUPDATA, and __mptcp_ofo_queue() will dequeue it as soon as
msk->ack_seq reaches dsn - offset.

[Severity: High]
Also in mptcp_init_skb(), doesn't subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) subtract offset twice when the
connection is in fallback mode?

In net/mptcp/subflow.c:subflow_check_data_avail():

	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_seq = __mptcp_expand_seq(subflow->map_seq,
					      subflow->iasn +
					      TCP_SKB_CB(skb)->seq -
					      subflow->ssn_offset - 1);

Because subflow->map_subflow_seq is based on tcp_sk(ssk)->copied_seq,
mptcp_subflow_get_map_offset(subflow) is 0, while subflow->map_seq is set
from TCP_SKB_CB(skb)->seq (which is tcp_sk(ssk)->copied_seq - offset).
Thus mptcp_subflow_get_mapped_dsn(subflow) is already msk->ack_seq -
offset.

When mptcp_init_skb() subtracts offset again, map_seq64 becomes
msk->ack_seq - 2 * offset and end_seq becomes (u32)msk->ack_seq + skb->len
- 2 * offset. In __mptcp_move_skb(), won't this drop the skb as
MPTCP_MIB_DUPDATA when skb->len <= 2 * offset, or compute copy_len =
skb->len - 2 * offset and skip 2 * offset bytes in __mptcp_recvmsg_mskq()
while leaving msk->ack_seq behind tp->copied_seq?

>  	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
> -	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>  
>  	__skb_unlink(skb, &ssk->sk_receive_queue);
>  
> @@ -446,15 +443,47 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }
>  
> +/* Caller must hold the msk data lock */
> +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_seq64 = mptcp_iasn(msk) - 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;
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> -	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		/* Ensure we see the updated ack_seq after seeing the flag */
> +		smp_rmb();
> +		msk->copied_seq += mptcp_iasn(msk);
> +
> +		/* 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);
> +	}

[Severity: High]
Is it safe for __mptcp_move_skb() to run test_and_clear_bit(MPTCP_SYNC_SEQ,
&msk->cb_flags) and call __mptcp_sync_rcv_sequence(sk) without holding
mptcp_data_lock(sk)?

When __mptcp_move_skb() is called from mptcp_move_skbs() or
mptcp_release_cb() -> __mptcp_move_skbs(), sk->sk_lock.slock has been
dropped while sock_owned_by_user(sk) is true:

net/mptcp/protocol.c:mptcp_move_skbs() {
	mptcp_data_lock(sk);
	while (mptcp_can_spool_backlog(sk, &skbs)) {
		mptcp_data_unlock(sk);
		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);

		mptcp_data_lock(sk);
	}
}

At the same time, other paths holding mptcp_data_lock(sk) while
sock_owned_by_user(sk) is true update msk->cb_flags using non-atomic
__set_bit() or |= operations, such as __mptcp_data_acked(),
__mptcp_check_push(), and mptcp_propagate_state():

net/mptcp/protocol.c:__mptcp_data_acked() {
	if (!sock_owned_by_user(sk))
		__mptcp_clean_una(sk);
	else
		__set_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->cb_flags);
}

If a concurrent softirq path loads msk->cb_flags (while MPTCP_SYNC_SEQ is
set) for a non-atomic __set_bit() and stores the word back after
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) in __mptcp_move_skb()
clears it, won't MPTCP_SYNC_SEQ be re-set and cause a subsequent
__mptcp_move_skb() or mptcp_release_cb() call to add mptcp_iasn(msk) to
msk->copied_seq a second time? Calling __mptcp_sync_rcv_sequence(sk) here
without mptcp_data_lock(sk) also violates the lock requirement noted above
__mptcp_sync_rcv_sequence().

[ ... ]

> @@ -2403,21 +2411,22 @@ 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;
>  
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> -
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> -
> -		return (unsigned int)hint_val;
> +	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		skb = skb_peek(&sk->sk_receive_queue);
> +		return skb ? skb->len : 0;
>  	}
>  
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	hint_val = READ_ONCE(msk->ack_seq) - msk->copied_seq;

[Severity: Medium]
Can mptcp_inq_hint() return an inflated byte count in two cases here?

First, in subflow_set_remote_key() when sock_owned_by_user(sk) is true,
msk->ack_seq is written before MPTCP_SYNC_SEQ is set:

net/mptcp/subflow.c:subflow_set_remote_key() {
	WRITE_ONCE(msk->ack_seq, subflow->iasn);
	WRITE_ONCE(msk->can_ack, true);
	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);

	if (!sock_owned_by_user(sk)) {
		...
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
}

Because mptcp_inq_hint() tests MPTCP_SYNC_SEQ first and then reads
READ_ONCE(msk->ack_seq) without holding mptcp_data_lock(sk), it can observe
MPTCP_SYNC_SEQ == 0 and then read the updated msk->ack_seq == subflow->iasn
before msk->copied_seq has been synced, causing READ_ONCE(msk->ack_seq) -
msk->copied_seq to wrap or clamp to INT_MAX.

Second, when test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) is true and user
space has already partially read from the fastopen skb (advancing
msk->copied_seq), returning skb->len does not subtract the already-copied
offset ((u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq).

[ ... ]

> @@ -3689,11 +3711,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  	msk->bytes_retrans = 0;
>  	msk->rcvspace_init = 0;
>  	msk->fastclosing = 0;
> +	msk->tfo_skb_len = 0;
>  	mptcp_init_rtt_est(msk);
>  
>  	/* for fallback's sake */
>  	WRITE_ONCE(msk->ack_seq, 0);
>  	atomic64_set(&msk->rcv_wnd_sent, 0);
> +	msk->copied_seq = 0;

[Severity: Medium]
Should mptcp_disconnect() also reset msk->rcvd_dummy_seq = false (and clear
MPTCP_SYNC_SEQ)?

If a passive fastopen socket sets msk->rcvd_dummy_seq = true in
mptcp_fastopen_subflow_synack_set_params() and is then disconnected via
mptcp_disconnect() before __mptcp_sync_rcv_sequence() runs,
msk->rcvd_dummy_seq remains true and a subsequent connection on the same
socket can invoke __mptcp_sync_rcv_sequence() and overwrite the tail skb's
sequence numbers in sk->sk_receive_queue.

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

Re: [PATCH mptcp-next v15 00/11] Reduce the differences between TCP and MPTCP for TLS usage

[MPTCP CI]

Hi Gang,

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/39888761240

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


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

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

For more details:

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


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

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

Re: [PATCH mptcp-next v15 07/11] mptcp: implement peek_len for proto_ops

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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).

- [Medium] mptcp_inq() in net/mptcp/protocol.c returns 0 whenever
  MPTCP_SYNC_SEQ is set, without looking at sk_receive_queue.
- [Medium] In mptcp_inq() (net/mptcp/protocol.c), the order
  test_bit(MPTCP_SYNC_SEQ) -> smp_rmb() -> READ_ONCE(ack_seq) -…
- [Medium] The commit message says a torn 64-bit read of
  ack_seq/copied_seq on 32-bit systems "is caught by the INT_MAX clamp".
- [Low] The commit message for "mptcp: implement peek_len for proto_ops"
  contains two factual errors.

--

mptcp: implement peek_len for proto_ops

This adds mptcp_inq(), which computes the number of readable bytes at the
MPTCP level from ack_seq - copied_seq. It is wired into a new
mptcp_peek_len() callback for both the IPv4 and IPv6 MPTCP proto_ops. The
commit describes this as preparatory work for an upcoming kTLS-over-MPTCP
series.

> This is preparatory work for
> an upcoming kTLS-over-MPTCP series: no in-tree consumer currently
> reaches ->peek_len on an MPTCP socket (the only caller, strp_peek_len(),
> cannot attach strparser to MPTCP; the existing TLS receive path calls
> tcp_inq() directly rather than going through ->peek_len).

[Severity: Low]
Is "the only caller" accurate? sock_has_rx_data() in drivers/vhost/net.c
also calls ->peek_len:

	if (sock->ops->peek_len)
		return sock->ops->peek_len(sock);

vhost-net only accepts packet/tun/tap sockets, so the conclusion that MPTCP
is not reached still holds. Only the wording is off.

> Note: mptcp_inq() intentionally differs from mptcp_inq_hint() when the
> calculated value is 0 and SOCK_DONE is set.

Can that state actually happen? mptcp_check_data_fin() bumps ack_seq and
sets SOCK_DONE together:

	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
		...
		sock_set_flag(sk, SOCK_DONE);

copied_seq only advances by the number of data bytes in recvmsg and
read_sock, so it never moves past the DATA_FIN. After a DATA_FIN,
ack_seq - copied_seq is therefore always at least 1.

That would make the !hint_val && SOCK_DONE branch in mptcp_inq_hint()
unreachable. The real difference between the two helpers would then be
N + 1 vs N for N unread bytes. Could the note be updated to say that?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7b0005d47bead..ad11a96b83eba 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4848,6 +4848,43 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_inq(struct sock *sk)
> +{
> +	const struct mptcp_sock *msk = mptcp_sk(sk);
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
> +		answ = 0;
> +	} else if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		answ = 0;

[Severity: Medium]
Can this branch under-report readable data on a passive Fast Open (TFO)
socket?

For a passive TFO msk, mptcp_sk_clone_init() has already moved the msk to
TCP_ESTABLISHED, so the SYN_SENT/SYN_RECV check above doesn't filter it.
mptcp_fastopen_subflow_synack_set_params() has also queued the SYN payload
on sk_receive_queue with a dummy mapping.

If the MPC third ACK is processed while the user owns the msk,
check_fully_established()->__mptcp_subflow_fully_established()->
subflow_set_remote_key() takes the deferred path:

net/mptcp/subflow.c:subflow_set_remote_key() {
    ...
	WRITE_ONCE(msk->ack_seq, subflow->iasn);
    ...
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
}

The flag stays set until mptcp_release_cb() or __mptcp_move_skb() clears
it. During that time, a ->peek_len caller holding lock_sock() gets 0 here.
recvmsg() or read_sock() can still read the queued TFO bytes in that window.

The existing mptcp_inq_hint() handles the same state by looking at the
queue:

	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
		skb = skb_peek(&sk->sk_receive_queue);
		return skb ? skb->len : 0;
	}

A peek_len consumer such as strp_data_ready() treats this value as a gate:

	if (strp_peek_len(strp) < strp->need_bytes)
		return;

Suppose the whole message arrived in the SYN data and nothing else follows.
Could the reader stall in that case? There is no in-tree MPTCP ->peek_len
consumer yet, so this would show up with the planned kTLS user.

The commit message doesn't mention this case as a difference from
mptcp_inq_hint(). Should this branch do what mptcp_inq_hint() does?

> +	} else {
> +		bool sock_done = sock_flag(sk, SOCK_DONE);
> +		u64 hint_val;
> +
> +		/* Ensure we see updated ack_seq and copied_seq after
> +		 * checking MPTCP_SYNC_SEQ flag.
> +		 */
> +		smp_rmb();
> +
> +		hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);

[Severity: Medium]
Does this smp_rmb() give the guarantee the comment describes?

In subflow_set_remote_key(), the writer stores ack_seq, then does smp_wmb()
and set_bit(MPTCP_SYNC_SEQ). When the msk is user-owned, it defers the
copied_seq rebase. That pairing only orders accesses for a reader that sees
the flag set, which is what __mptcp_move_skb() does:

	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
		/* Ensure we see the updated ack_seq after seeing the flag */
		smp_rmb();
		msk->copied_seq += mptcp_iasn(msk);

Here the barrier is only reached after the flag was seen clear, and a clear
flag says nothing about ack_seq.

Take a passive TFO msk that is ESTABLISHED via mptcp_sk_clone_init(), with
ack_seq 0 and copied_seq set to minus the TFO length, owned by a lock_sock()
holder:

CPU0                                CPU1 (BH, MPC third ACK)
mptcp_inq()                         check_fully_established()
  test_bit(MPTCP_SYNC_SEQ) == 0
                                      subflow_set_remote_key()
                                        WRITE_ONCE(msk->ack_seq, iasn)
                                        smp_wmb()
                                        set_bit(MPTCP_SYNC_SEQ)
  smp_rmb()
  READ_ONCE(msk->ack_seq)     -> iasn
  READ_ONCE(msk->copied_seq)  -> not yet rebased

hint_val then becomes iasn minus the old copied_seq. With a hash-derived
64-bit iasn, that is almost always >= INT_MAX, so INT_MAX would be returned
as the readable byte count.

Would re-checking the flag after the loads close this window?

The commit message also says:

  READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
  ack_seq and copied_seq because mptcp_inq() will be called from TLS
  contexts with the socket lock held - taking the spinlock would deadlock.

A lock_sock() owner doesn't hold sk_lock.slock, so taking
mptcp_data_lock() there would not deadlock. Could the data lock be taken
when the caller owns the socket?

> +		if (hint_val >= INT_MAX)
> +			hint_val = INT_MAX;

[Severity: Medium]
The commit message says:

  On 32-bit systems a torn read can produce a wrapped subtraction (close to
  2^64), which is caught by the INT_MAX clamp.

Is "caught" the right word here? The clamp doesn't reject a torn value. It
turns it into INT_MAX, which mptcp_inq() then returns as the readable byte
count.

A torn read leaves the difference off by about 2^32, in either direction.
It is either a small real value plus 2^32, or a negative difference that
wraps near 2^64. Both cases end up as INT_MAX.

In the lock_sock() owner context, subflow_set_remote_key() is still a
concurrent u64 writer of ack_seq:

	WRITE_ONCE(msk->ack_seq, subflow->iasn);

On 32-bit, READ_ONCE() and WRITE_ONCE() of a u64 are two 32-bit accesses.
That would let ->peek_len report about 2GB readable on a socket holding
little or no data.

tcp_inq(), which this is meant to mirror, doesn't have this problem because
TCP sequence numbers are 32-bit. Would a retry, a fallback to 0, or taking
the data lock in owner context fit better here?

> +
> +		answ = (unsigned int)hint_val;
> +
> +		/* Subtract 1, if FIN was received. Mirror tcp_inq() */
> +		if (answ && sock_done)
> +			answ--;
> +	}
> +
> +	return answ;
> +}

[ ... ]

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