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 3/4] mptcp: sync mptcp skb cb layout with tcp one
@ 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 7 potential
issue(s) to consider.

Critical: 2 · High: 1 · 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).

- [Critical] mptcp: race and copied_seq reset in subflow_set_remote_key()
  on TFO
- [Critical] mptcp: mptcp_init_skb() subtracts offset from map_seq64
  corrupting data
- [High] mptcp: __mptcp_recvmsg_mskq() offset underflow on MSG_PEEK and
  skb leak
- [Medium] mptcp_inq_hint() (net/mptcp/protocol.c:2395-2410) over-reports
  how many bytes are queued.
- [Medium] mptcp_data_queue_ofo() can admit an skb whose 64-bit DSN is
  about 2^63 away from msk->ack_seq.
- [Medium] mptcp: mptcp_inq_hint() uses map_seq64 instead of
  msk->copied_seq
- [Low] Two statements in the commit message do not match the code.

--

mptcp: sync mptcp skb cb layout with tcp one

This commit stores the MPTCP-level sequence numbers in the skb cb as 32
bits, so that map_seq, end_seq and flags sit at the same offsets as the
matching tcp_skb_cb fields. A separate 64-bit map_seq64 is kept for checks
that need the full sequence space. The receive and OFO queue comparisons
move to the 32-bit before()/after() helpers, and build time checks are
added for the cb layout.

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

[Severity: Low]
Does "the latter invariant" refer to the 32-bit distance guarantee in the
previous paragraph? The only build time checks added are the CHK_CB_FIELD()
offsetof()/offsetofend() comparisons in mptcp_proto_init(). Those check the
cb layout, not any sequence distance.

Is it also accurate that skbs in the receive queue have been validated
against the receive window? In __mptcp_move_skb(), both the in-sequence
branch and the partial packet branch reach insert: without looking at
msk->rcv_wnd_sent:

    if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
        /* in sequence */
insert:
    ...
    /* Partial packet */
    if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
        ...
        goto insert;
    }

Only mptcp_data_queue_ofo() reads rcv_wnd_sent. For skbs in the receive
queue, the bound seems to come from them starting at or before ack_seq,
plus the skb length and rmem limits, not from a window check.

As noted below for mptcp_data_queue_ofo(), the window check also does not
bound the distance when rcv_wnd_sent lags ack_seq.

Could the commit message give the actual reason the truncation is safe?

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

[ ... ]

> @@ -302,16 +304,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);
>  

[ ... ]

> -	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)) {

[Severity: Medium]
Do these checks bound the distance from msk->ack_seq that the later 32-bit
comparisons depend on? They only compare against the upper edge,
rcv_wnd_sent, using modular 64-bit arithmetic.

rcv_wnd_sent can lag ack_seq:

  - subflow_set_remote_key() sets both to iasn.
  - __mptcp_move_skb() advances ack_seq for in-sequence data without
    checking rcv_wnd_sent.
  - rcv_wnd_sent only moves in mptcp_set_rwin() when a DSS ack is written.

Let A = ack_seq, W = rcv_wnd_sent and L = A - W > 0. Suppose the peer sends
a dsn64 mapping with data_seq = A + 2^63 - L/2 and len > L/2:

  subflow_check_data_avail()
    before64(ack_seq, old_ack) is false, mapping accepted
  __mptcp_move_skb()
    after64(map_seq64, ack_seq) is true -> mptcp_data_queue_ofo()
  mptcp_data_queue_ofo()
    map_seq64 - W = 2^63 + L/2, which is negative as s64, so after64()
    is false
    end_seq - W is also negative as s64, so after64() is false
    the skb is admitted to the OFO queue

The 32-bit values stored are map_seq = (u32)A - L/2 and
end_seq = (u32)A + len - L/2. Then in __mptcp_ofo_queue():

		if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
			break;
		...
		if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
		...
		seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;

the skb appears to overlap ack_seq. It is queued to sk_receive_queue and
ack_seq advances by len - L/2. Readers then deliver this data at stream
position A, even though its DSN is about 2^63 away.

Before this patch, after64(map_seq, ack_seq) in __mptcp_ofo_queue() would
have stopped at this skb, so it was never delivered.

Would an explicit full-width distance check from ack_seq help here? For
example, rejecting skbs where map_seq64 - ack_seq exceeds the window or
INT_MAX, before relying on the truncated values.

[ ... ]

> @@ -2392,7 +2399,8 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
>  
>  	skb = skb_peek(&sk->sk_receive_queue);
>  	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +		u64 hint_val = READ_ONCE(msk->ack_seq) -
> +			       MPTCP_SKB_CB(skb)->map_seq64;

[Severity: Medium]
The head skb's map_seq64 is only written when the skb is queued, in
mptcp_init_skb(), mptcp_fastopen_subflow_synack_set_params() and
subflow_set_remote_key(). Nothing moves it forward as data is read or
skipped. So ack_seq - map_seq64 also counts bytes that userspace has
already consumed.

Besides the partial read case in __mptcp_recvmsg_mskq() and
__mptcp_read_sock(), the head skb can start before copied_seq without
any read at all:

  - __mptcp_move_skb() queues a partially overlapping skb through the
    partial packet branch with map_seq64 < ack_seq;
  - __mptcp_ofo_queue() does the same for an OFO skb whose map_seq is
    before ack_seq;
  - mptcp_init_skb() sets map_seq64 to the mapped DSN minus offset
    when the subflow skb has a non-zero offset.

In each case the skipped prefix is reported as unread through SIOCINQ
in mptcp_ioctl() and through the TCP_CM_INQ cmsg in mptcp_recvmsg().
Later in-order data can be coalesced into the head skb by
mptcp_try_coalesce(), up to sk_rcvbuf >> 3. That keeps the stale
map_seq64 at the head for longer, and the error can grow with each
partial read.

In mptcp_rcv_space_adjust(), copied -= mptcp_inq_hint(sk) subtracts
the inflated value. copied can then drop to or below rcvq_space.space,
or even go negative, and mptcp_rcvbuf_grow() is skipped while the
application is draining data.

Since copied_seq now tracks the first unread byte, would
READ_ONCE(msk->ack_seq) - msk->copied_seq be the right value here?

[Severity: Medium]
Can this over-report the amount of queued data after a partial read?

The previous patch in the series ("mptcp: remove CB offset field") removed
the MPTCP_SKB_CB(skb)->map_seq updates from __mptcp_recvmsg_mskq(),
__mptcp_read_sock(), __mptcp_move_skb() and __mptcp_ofo_queue(). Read
progress now lives in msk->copied_seq.

That commit message mentions "the new ack_seq - copied_seq hint", but the
hint here still subtracts the start sequence of the head skb.

After a partial read in __mptcp_recvmsg_mskq():

			msk->copied_seq += count;
			if (count < data_len)
				break;

the head skb stays in sk_receive_queue with its original map_seq64. For
example, with a single 1000-byte skb, after reading 100 bytes SIOCINQ and
TCP_CM_INQ report 1000 instead of 900.

If the head skb partially overlapped earlier data (queued with
map_seq64 < ack_seq), the overlapping bytes are counted as well.

mptcp_rcv_space_adjust() also does:

	copied -= mptcp_inq_hint(sk);

so rcvbuf autotuning is skewed too.

Should this be READ_ONCE(msk->ack_seq) - msk->copied_seq instead? The same
expression is still there at the end of the series.

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -542,6 +542,7 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  		skb = skb_peek(&sk->sk_receive_queue);
>  		if (skb) {
>  			MPTCP_SKB_CB(skb)->map_seq = subflow->iasn - skb->len;
> +			MPTCP_SKB_CB(skb)->map_seq64 = subflow->iasn - skb->len;
>  			MPTCP_SKB_CB(skb)->end_seq = subflow->iasn;
>  			msk->copied_seq -= skb->len;
>  		}

[Severity: Critical]
With read progress now tracked only in msk->copied_seq, can this
rewrite discard data the application has already read?

On a passive MPTFO connection,
mptcp_fastopen_subflow_synack_set_params() queues the SYN data skb on
the msk receive queue with map_seq64 = -(u64)skb->len. It sets
copied_seq to the same value. The child can be accepted and read
before the third ACK carrying the peer key arrives.

Suppose the application first reads count < skb->len bytes.
__mptcp_recvmsg_mskq() moves copied_seq to -(u64)skb->len + count and
leaves the skb at the head of sk_receive_queue. When the key arrives,
subflow_set_remote_key() sets copied_seq to iasn and then subtracts
skb->len, so copied_seq equals the new map_seq64 again. The next read
returns those count bytes a second time, and bytes_consumed runs ahead
of bytes_received.

IASN is assumed to be 0 until the key is known. Would shifting the
existing copied_seq by iasn, instead of resetting it, preserve the
partial read?

There is also a locking question. check_fully_established() reaches
this function through __mptcp_subflow_fully_established() in softirq.
It holds only mptcp_data_lock() and does not check
sock_owned_by_user() on the msk. __mptcp_recvmsg_mskq() and
__mptcp_read_sock() walk sk_receive_queue under lock_sock() only.
mptcp_eat_recv_skb() unlinks and frees the head skb without the data
lock.

This allows two races:

  - If the reader eats the TFO skb right after the skb_peek() here,
    the map_seq, map_seq64 and end_seq writes land in a freed skb.
  - If the reader runs between msk->copied_seq += count and
    mptcp_eat_recv_skb() on a fully read skb, copied_seq is left
    skb->len behind ack_seq once the skb is unlinked. Every later skb
    then computes offset >= skb->len in __mptcp_recvmsg_mskq() and is
    skipped.

The skb_peek() and cb rewrite predate this patch, but the reliance on
copied_seq makes the race more damaging. When the msk is owned by the
user, should this adjustment be deferred, for example through a
cb_flags bit handled in mptcp_release_cb()?

[ ... ]

> @@ -422,11 +429,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;
>  	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>  

[Severity: Critical]
Does subtracting offset here assign DSNs to bytes that the current DSS
mapping does not cover?

offset is tp->copied_seq - TCP_SKB_CB(skb)->seq on the subflow. The
mapping returned by mptcp_subflow_get_mapped_dsn() starts at
tp->copied_seq, so it only covers skb bytes [offset, skb->len). The
leading offset bytes belong to an earlier mapping.

For example, subflow_check_data_avail() can call
mptcp_subflow_discard_data() for an old mapping that ends in the
middle of the skb. get_mapping_status() then installs the next mapping
on the rest of the same skb. A partially overlapping TCP segment left
on the subflow queue has the same effect.

With map_seq64 = mapped_dsn - offset and end_seq = map_seq + skb->len,
the skb claims DSN [mapped_dsn - offset, mapped_dsn) for that prefix.
When mapped_dsn is at or before ack_seq, the prefix overlaps data that
was already received and is skipped.

The mapping can also be out of order at the msk level, because the
intervening DSNs travel on another subflow. In that case
__mptcp_move_skb() hands the skb to mptcp_data_queue_ofo() with the
shifted range, and then:

  - __mptcp_ofo_queue() moves it to sk_receive_queue once ack_seq
    reaches mapped_dsn - offset, and advances ack_seq over the prefix.
    The stale subflow bytes are delivered to userspace as stream data.
  - The real data for [mapped_dsn - offset, mapped_dsn) from the other
    subflow can be dropped as MPTCP_MIB_DUPDATA. This happens either in
    __mptcp_move_skb() or in the overlap handling in
    mptcp_data_queue_ofo().

The DSS checksum in validate_data_csum() also starts at offset, so the
prefix is never checked either.

The subtraction came with the removal of the cb offset field earlier in
the series. Here it is carried into map_seq64, which all the admission
checks now rely on. Could the prefix be trimmed from the skb before it
is moved to the msk? Or should map_seq64 stay at mapped_dsn, with the
skip recorded some other way?

[ ... ]

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -128,10 +128,13 @@
>  #define MPTCP_SYNC_SNDBUF	7
>  
>  struct mptcp_skb_cb {
> -	u64 map_seq;
> -	u64 end_seq;
> +	u32 map_seq;
> +	u32 end_seq;
> +	u32 unused;
> +	u16 flags;
>  	u8  has_rxtstamp;
>  	u8  cant_coalesce;
> +	u64 map_seq64;
>  };
>  
>  #define MPTCP_SKB_CB(__skb)	((struct mptcp_skb_cb *)&((__skb)->cb[0]))

[Severity: High]
Not part of this hunk, but __mptcp_recvmsg_mskq() now computes the read
position against this 32-bit field:

    offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;

and skips the skb when offset >= skb->len. Does that still work for
MSG_PEEK?

With MSG_PEEK, copied_seq is not advanced while walking the queue. For
the second skb in sk_receive_queue, map_seq is after copied_seq, so the
u32 subtraction wraps to a value close to 2^32. The offset >= skb->len
check then fires. That skb and every following one are skipped before
any of the MSG_PEEK handling below is reached.

A peek can therefore never return more than the remainder of the head
skb. With MSG_PEEK | MSG_WAITALL, or with SO_RCVLOWAT larger than that
remainder, the caller can sleep in sk_wait_data() even though enough
data is queued.

Would it fix this to track a local u32 sequence instead? It would start
at msk->copied_seq, advance by the bytes each walked skb contributes,
and offset would be computed against it.

In the !MSG_PEEK case, an skb lying entirely before copied_seq is also
skipped with continue. It is left on sk_receive_queue, still charged to
sk_rmem_alloc. mptcp_recv_skb() calls mptcp_eat_recv_skb() in the same
situation. Should __mptcp_recvmsg_mskq() eat such skbs too? Otherwise
they stay at the head of the queue, and mptcp_inq_hint() keeps using
their map_seq64.

-- 
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 3/4] mptcp: sync mptcp skb cb layout with tcp one 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