MPTCP Linux Development
 help / color / mirror / Atom feed
* [PATCH mptcp-next v16 0/4] Reduce the differences between TCP and MPTCP for TLS usage
@ 2026-10-29  6:47 Geliang Tang
  2026-10-29  6:47 ` [PATCH mptcp-next v16 1/4] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Geliang Tang @ 2026-10-29  6:47 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

From: Geliang Tang <tanggeliang@kylinos.cn>

v16:
 - Simplified the series to retain only the core changes required by both
   kTLS and sockmap: drop the CB offset field, add copied_seq tracking,
   and sync the MPTCP skb CB layout with the TCP one.
 - Dropped patch 2, leaving the fastopen code unchanged.
 - Moved patches 6-11 into a separate series "prepare interfaces for kTLS
   and sockmap support".

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

	struct tls_skb_cb {
	    u32 seq;
	};

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

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

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

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

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

Paolo Abeni (4):
  mptcp: drop the mptcp_ooo_try_coalesce() helper
  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 |  14 +++-
 net/mptcp/protocol.c | 184 ++++++++++++++++++++++++-------------------
 net/mptcp/protocol.h |  11 ++-
 net/mptcp/subflow.c  |  16 +++-
 4 files changed, 137 insertions(+), 88 deletions(-)

-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v16 2/4] mptcp: remove CB offset field
@ 2026-10-30 20:42 netdev-bot+sashiko
  0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 20:42 UTC (permalink / raw)
  To: geliang; +Cc: mptcp, pabeni, kuba

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

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

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

- [Critical] On the passive (server) TFO path, subflow_set_remote_key()
  overwrites msk->copied_seq: it sets `msk->copied_seq = subflow->iasn;`…
- [Critical] subflow_set_remote_key() now calls
  skb_peek(&sk->sk_receive_queue) on the msk, writes…
- [Critical] mptcp: stream corruption from offset subtraction in
  mptcp_init_skb()
- [High] mptcp_inq_hint() still reports unread bytes as
  READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(head)->map_seq.
- [High] MSG_PEEK cannot see data beyond the head skb.
- [Medium] The commit message describes three mechanisms that are not in
  the code.
- [Medium] The new skip branch in __mptcp_recvmsg_mskq() (`if (offset >=
  skb->len) { *last = skb; continue; }`) leaves the skb linked and…
- [Low] The commit message says it fixes a user-visible bug: before the
  MPC ACK, the TFO skb's `map_seq = -skb->len` (32-bit negation zero-…

--

mptcp: remove CB offset field

This patch removes the per-skb offset field from the MPTCP skb control
block and tracks read progress in a new msk->copied_seq field, similar to
TCP. It also changes the TFO skb dummy mapping and rebases it on IASN
when the remote key is received.

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

[ ... ]

> MPC ACK.  This patch fixes that behaviour through both the -(u64)skb->len
> sign fix and the new ack_seq - copied_seq hint.

[Severity: Medium]
Do these paragraphs match the code?

The message says copied_seq "is always accessed under the msk socket
lock" and that the IASN sync is deferred to the release CB when user
space owns the socket. mptcp_release_cb() has no copied_seq or IASN
handling at all. subflow_set_remote_key() writes msk->copied_seq and the
head skb's map_seq/end_seq directly. It holds only the msk data spinlock
and never checks sock_owned_by_user().

The __mptcp_move_skb() hunk only removes the offset/map_seq trimming on
the partial packet path. No TFO map_seq sync is added there.

mptcp_inq_hint() is not changed by this patch and still computes:

	u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

The later patches in the series don't seem to add any of these pieces.
Could the commit message describe what the code actually does?

> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index f717750906ffa..04b1038b5200d 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c

[ ... ]

> @@ -44,20 +45,23 @@ 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;

[Severity: Low]
This isn't a bug, but this one-line change fixes a user-visible problem.
The old -skb->len was a 32-bit negation zero-extended into the u64
map_seq, so SIOCINQ and TCP_CM_INQ returned INT_MAX on TFO server
sockets before the MPC ACK.

The old line was added by commit 36b122baf6a8 ("mptcp: add
subflow_v(4,6)_send_synack()"). Should this fix be split out with a
Fixes: tag so stable trees pick it up? As it stands, it is mixed into a
refactor that prepares for the next patch.

>  	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->copied_seq = MPTCP_SKB_CB(skb)->map_seq;

[ ... ]

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..f89ed94b668d9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -2195,11 +2186,17 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>  	int copied = 0;
>  
>  	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> -		u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
> +		u32 delta, offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
>  		u32 data_len = skb->len - offset;
>  		u32 count;
>  		int err;
>  
> +		/* Skip skbs that are entirely before copied_seq */
> +		if (offset >= skb->len) {
> +			*last = skb;
> +			continue;
> +		}

[Severity: High]
Can MSG_PEEK still see anything past the head skb?

MSG_PEEK never advances copied_seq. For the second and later skbs,
map_seq is ahead of copied_seq, so the u32 subtraction wraps:

    skb1: map_seq = M, len L1, k bytes read, copied_seq = M + k
    skb2: map_seq = M + L1
    offset = (u32)(M + k - (M + L1)) = 2^32 - (L1 - k) >= skb2->len

As a result, skb2 and every skb after it are skipped with *last = skb.
Before this patch, non-head skbs had a CB offset of 0, so a peek walked
the whole queue. The comment above the check also doesn't fit this case,
because these skbs start after copied_seq.

Now take MSG_PEEK | MSG_WAITALL, or SO_RCVLOWAT larger than the head
skb's unread data. On the next pass of mptcp_recvmsg(), the head skb is
counted as already peeked and every other skb is skipped. It then calls
sk_wait_data(sk, &timeo, last) with last pointing at the queue tail.

Would this block until timeout or a signal even though enough data is
queued? Newly arriving skbs would be skipped the same way.

[Severity: Medium]
This branch leaves the skb linked and charged to the msk receive memory.
mptcp_recv_skb() frees the skb on the same condition:

net/mptcp/protocol.c:mptcp_recv_skb() {
    ...
		offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
		if (offset < skb->len) {
			*off = offset;
			return skb;
		}
		mptcp_eat_recv_skb(sk, skb);
    ...
}

The offset is an unsigned difference, so neither path can tell "already
consumed" apart from "copied_seq is behind map_seq". Suppose copied_seq
and map_seq get out of step, for example through the
subflow_set_remote_key() race described below.

Would the recvmsg path then keep every later skb queued until close,
stalling the connection once rcvbuf fills? And would the read_sock/splice
path silently drop data that was never delivered?

[ ... ]

> @@ -2233,11 +2230,9 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>  
>  		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;
> +			msk->copied_seq += count;
> +			if (count < data_len)
>  				break;
> -			}

[Severity: High]
Without the map_seq += count, is mptcp_inq_hint() still correct? It is
unchanged and still computes:

net/mptcp/protocol.c:mptcp_inq_hint() {
    ...
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
    ...
}

That formula only worked because partial reads here and in
__mptcp_read_sock() advanced the head skb's map_seq. It also relied on
__mptcp_move_skb() and __mptcp_ofo_queue() trimming map_seq for
overlapping data.

This patch removes all of those updates, and mptcp_init_skb() now sets
map_seq to mapped_dsn - offset. The head skb's map_seq therefore stays at
the skb start, and the hint is too large by copied_seq - head->map_seq.
For example, after reading 1 byte of a 64KB head skb, the hint still
reports 64KB.

This affects SIOCINQ/FIONREAD in mptcp_ioctl() and the TCP_CM_INQ cmsg
in mptcp_recvmsg(). It also affects mptcp_rcv_space_adjust(), which
does:

	copied -= mptcp_inq_hint(sk);

so rcvbuf autotuning would underestimate how much was consumed.

The mptcp_inq selftest in tools/testing/selftests/net/mptcp/mptcp_inq.c
reads one byte and then checks:

	assert((size_t)tcp_inq == (expect_len - 1));

Wouldn't this assert now fail? Should mptcp_inq_hint() be based on
msk->copied_seq instead of the head skb's map_seq?

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..5e553adb9397e 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -533,6 +533,19 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
> +	msk->copied_seq = subflow->iasn;
> +
> +	if (subflow->is_mptfo) {
> +		struct sock *sk = (struct sock *)msk;
> +		struct sk_buff *skb;
> +
> +		skb = skb_peek(&sk->sk_receive_queue);

[Severity: Critical]
Is it safe to touch the msk receive queue and msk->copied_seq here
without checking whether user space owns the msk?

This runs in softirq when the MPC ACK arrives, holding only the msk data
spinlock:

tcp_v4_rcv()
  ...
    mptcp_incoming_options()
      check_fully_established()
        mptcp_data_lock(msk)
        __mptcp_subflow_fully_established()
          subflow_set_remote_key()
            skb_peek(&sk->sk_receive_queue)
            MPTCP_SKB_CB(skb)->map_seq = ...

Elsewhere, MPTCP only touches sk_receive_queue from BH when the msk is
not owned. For example, __mptcp_move_skbs_from_subflow() falls back to
the backlog when own_msk is false.

A reader in mptcp_recvmsg() or __mptcp_read_sock() owns the msk through
lock_sock(), which does not keep slock held. While it owns the msk, the
reader does msk->copied_seq += count and calls mptcp_eat_recv_skb().
That function unlinks the skb and hands it to skb_attempt_defer_free()
without the data lock.

Can the softirq peek the TFO skb just before the reader unlinks and frees
it, and then write into the freed skb's cb[]?

The reader's plain u64 read-modify-write of copied_seq also races with
the store here. One update can be lost, and on 32-bit the value can
tear. If the reader's stale value wins, copied_seq stays in the pre-IASN
space while map_seq is in the IASN space, and every later
(u32)copied_seq - map_seq offset is wrong. __mptcp_recvmsg_mskq() would
then skip those skbs forever, and mptcp_recv_skb() would eat them
without delivering them.

Before this patch, subflow_set_remote_key() did not touch the msk receive
queue or any state the reader owns.

> +		if (skb) {
> +			MPTCP_SKB_CB(skb)->map_seq = subflow->iasn - skb->len;
> +			MPTCP_SKB_CB(skb)->end_seq = subflow->iasn;
> +			msk->copied_seq -= skb->len;

[Severity: Critical]
Does this discard any partial read of the TFO skb?

mptcp_fastopen_subflow_synack_set_params() queues the TFO skb with
map_seq = copied_seq = -(u64)len and calls sk_data_ready(). The accepted
msk can be read before the MPC ACK arrives, since mptcp_recvmsg() only
rejects TCP_LISTEN.

If the application reads k < len bytes first, __mptcp_recvmsg_mskq()
sets copied_seq to -len + k and leaves the skb queued. When the MPC ACK
arrives:

check_fully_established()
  __mptcp_subflow_fully_established()
    subflow_set_remote_key()
      msk->copied_seq = iasn - len
      MPTCP_SKB_CB(skb)->map_seq = iasn - len

The next read computes offset 0, so bytes [0, k) would reach user space
a second time, and msk->bytes_consumed would count them twice. Before
this patch, the read progress lived in MPTCP_SKB_CB(skb)->offset, which
this path did not touch. The remote client decides when to send the MPC
ACK, so it can reliably open this window.

This also contradicts the commit message's claim that the sync
"preserves any partial progress (copy) made on the TFO packet". Should
this shift the existing value instead, i.e. msk->copied_seq += iasn,
with map_seq/end_seq shifted by the same amount?

[ ... ]

> @@ -426,9 +424,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;
>  	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>  

[Severity: Critical]
Is it safe to assume the skb bytes before tp->copied_seq belong to the
DSS mapping that is active now?

With D = mptcp_subflow_get_mapped_dsn(subflow), mptcp_init_skb() now
claims the whole skb carries DSN [D - offset, D - offset + skb->len),
prefix included. That only holds if the prefix was covered by the same
mapping. mptcp_subflow_discard_data() can leave tp->copied_seq in the
middle of an skb, and get_mapping_status() accepts an skb that is only
partially covered by the current mapping, with the next mapping
starting inside the same skb. Once the old mapping's tail has been
discarded as duplicate data, the prefix holds bytes from a different
mapping. The skb is still placed as if they were DSN [D - offset, D).
Before this patch map_seq was D, and the CB offset kept the prefix out
of the msk sequence space entirely.

__mptcp_move_skb() then classifies the skb on the shifted map_seq. If
the new mapping is out of order but D - offset <= msk->ack_seq < D, the
skb takes the partial packet path and ack_seq jumps over the missing
[ack_seq, D) range. __mptcp_recvmsg_mskq() then hands the stale prefix
bytes to user space in place of that range. If D - offset is still
after ack_seq, the skb is queued out of order covering
[D - offset, D). mptcp_data_queue_ofo() then drops the genuine
segments for that range as DUPDATA, so the stale bytes are delivered
later instead.

Dropping the MPTCP_SKB_CB(from)->offset test in __mptcp_try_coalesce()
widens this. skb_try_coalesce() appends from starting at byte 0, so any
to skb ending at D - offset now absorbs the stale prefix as DSN
[D - offset, D). That applies to the receive queue, the out-of-order
queue and the backlog list. The old check was what kept an skb with
unconsumed leading bytes from being merged.

The peer controls both the DSS mappings and the segmentation, so this
looks remotely triggerable, and it corrupts the byte stream delivered
to the application. Should mptcp_init_skb() trim the prefix off the skb
so map_seq can stay at the mapped DSN? Or is there another way to keep
the prefix out of the msk sequence space?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-10-30 20:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-29  6:47 [PATCH mptcp-next v16 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-10-29  6:47 ` [PATCH mptcp-next v16 1/4] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
2026-10-29  6:47 ` [PATCH mptcp-next v16 2/4] mptcp: remove CB offset field Geliang Tang
2026-10-29  7:10   ` sashiko-bot
2026-10-29  6:47 ` [PATCH mptcp-next v16 3/4] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
2026-10-29  7:19   ` sashiko-bot
2026-10-29  6:47 ` [PATCH mptcp-next v16 4/4] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
2026-10-29  8:42 ` [PATCH mptcp-next v16 0/4] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
  -- strict thread matches above, loose matches on Subject: below --
2026-10-30 20:42 [PATCH mptcp-next v16 2/4] mptcp: remove CB offset field netdev-bot+sashiko

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