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

From: Geliang Tang <tanggeliang@kylinos.cn>

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

	struct tls_skb_cb {
	    u32 seq;
	};

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

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

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

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

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

Geliang Tang (1):
  mptcp: defer read_sock cleanup to mptcp_worker

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

 net/mptcp/fastopen.c |  16 ++-
 net/mptcp/protocol.c | 268 ++++++++++++++++++++++++++-----------------
 net/mptcp/protocol.h |  16 ++-
 net/mptcp/subflow.c  |  19 ++-
 4 files changed, 201 insertions(+), 118 deletions(-)

-- 
2.55.0


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field
@ 2026-11-01  2:02 netdev-bot+sashiko
  0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-11-01  2:02 UTC (permalink / raw)
  To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba

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

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

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

- [High] __mptcp_move_skb() (net/mptcp/protocol.c:468-469) calls
  __mptcp_sync_rcv_sequence() whenever msk->rcvd_dummy_seq is true.
- [Medium] mptcp_move_skbs() (net/mptcp/protocol.c:2399-2400) does
  `test_and_clear_bit(MPTCP_SYNC_SEQ)` and then reads msk->tfo_iasn in…
- [Medium] The new lockless atomic `test_and_clear_bit(MPTCP_SYNC_SEQ,
  &msk->cb_flags)` in mptcp_move_skbs() (net/mptcp/protocol.c:2399)…
- [Low] Commit "mptcp: drop the cant_coalesce CB field" has no Signed-
  off-by tag, and no tags of any kind.
- [Low] In this commit, __mptcp_sync_rcv_sequence()
  (net/mptcp/protocol.c:439-455) rebases the passive-TFO skb with…
- [Low] The commit message does not match the code.

--

mptcp: drop the cant_coalesce CB field

This patch removes the cant_coalesce field from the MPTCP skb control
block. The dummy mapping of the passive fastopen skb is instead rebased
to the IASN, saved in msk->tfo_iasn, before any other skb lands in the
msk. When the msk is owned by user space, the update is deferred via
MPTCP_SYNC_SEQ.

> mptcp_move_skbs() and mptcp_release_cb() check MPTCP_SYNC_SEQ at each
> iteration before spooling the backlog, using test_and_clear_bit() since
> __mptcp_sync_rcv_sequence() can also be called without the socket lock
> held. A rcvd_dummy_seq flag tracks whether the TFO skb still carries the
> dummy mapping and needs synchronization.

[Severity: Low]
Is the "at each iteration" wording accurate for mptcp_move_skbs()? It
tests the bit only once, before the spool loop:

net/mptcp/protocol.c:mptcp_move_skbs() {
    ...
	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
		__mptcp_sync_rcv_sequence(sk);

	mptcp_data_lock(sk);
	while (mptcp_can_spool_backlog(sk, &skbs)) {
    ...
}

Only mptcp_release_cb() checks it inside its for (;;) loop.

The message also leaves out a change that user space can see. Since commit
f03afb3aeb9d, after the 3rd ack a passive TFO msk had ack_seq = IASN
while the TFO skb kept map_seq = -skb->len (2^32 - len). As a result,
mptcp_inq_hint() returned INT_MAX through SIOCINQ and TCP_CM_INQ, and
rcvspace autotuning was skewed. With the rebase done here, the hint
becomes len.

Could the commit message describe this behaviour change?

[Severity: Low]
The commit message ends here with no Signed-off-by tag, and no other tag
of any kind. The other patches in the series, such as "mptcp: drop the
mptcp_ooo_try_coalesce() helper" and "mptcp: remove CB offset field",
carry Signed-off-by lines. Was it dropped by accident?

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

[ ... ]

> @@ -438,6 +436,24 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }
>  
> +void __mptcp_sync_rcv_sequence(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sk_buff *skb;
> +
> +	if (likely(!msk->rcvd_dummy_seq))
> +		return;
> +
> +	/* User space can have already received the TFO skb. */
> +	msk->rcvd_dummy_seq = false;
> +	skb = skb_peek_tail(&sk->sk_receive_queue);
> +	if (!skb)
> +		return;
> +
> +	MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
> +	MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;

[Severity: Low]
Does this rebase lose the partial read progress on the TFO skb? Partial
reads in __mptcp_recvmsg_mskq() and __mptcp_read_sock() advance offset
and map_seq together, so map_seq points at the first unread byte:

net/mptcp/protocol.c:__mptcp_recvmsg_mskq() {
    ...
			if (count < data_len) {
				MPTCP_SKB_CB(skb)->offset += count;
				MPTCP_SKB_CB(skb)->map_seq += count;
				break;
    ...
}

Suppose user space accept()s the passive TFO child and reads k bytes
before the 3rd ack arrives. The sync here then leaves map_seq k bytes too
low, while the CB offset stays at k. mptcp_inq_hint() over-reports unread
data by k:

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

That value feeds SIOCINQ, TCP_CM_INQ and mptcp_rcv_space_adjust(). Data
delivery is still correct, because recvmsg uses the CB offset.

The later patch "mptcp: remove CB offset field" in this series appears to
fix this. It drops the CB offset, adds tfo_iasn to copied_seq at sync
time, and computes the hint as ack_seq - copied_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 +462,12 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> +	/* Be sure to sync the eventual fastopen dummy mapping before any other
> +	 * skb lands into the msk.
> +	 */
> +	if (unlikely(msk->rcvd_dummy_seq))
> +		__mptcp_sync_rcv_sequence(sk);

[Severity: High]
Can this sync run with a tfo_iasn value that was never set for this
connection?

Take a passive TFO connection whose 3rd ack has no MPC option.
check_fully_established() falls back via mptcp_try_fallback(), and
subflow_set_remote_key() is never called. tfo_iasn then keeps whatever
value sk_clone_lock() copied from the listener msk, because neither
__mptcp_init_sock() nor mptcp_sk_clone_init() initializes it.

mptcp_disconnect() resets the flag but leaves tfo_iasn alone:

	msk->rcvd_dummy_seq = false;

Consider a socket that does an active MPTCP connect(), which sets
tfo_iasn in subflow_set_remote_key(). It then calls connect(AF_UNSPEC),
then listen() with TCP_FASTOPEN. Its passive children inherit a stale
non-zero tfo_iasn (X):

mptcp_fastopen_subflow_synack_set_params()
    msk->rcvd_dummy_seq = true;

3rd ack without MPC:
check_fully_established()
    mptcp_try_fallback()        /* tfo_iasn left as X */

first fallback data skb:
__mptcp_move_skb()
    __mptcp_sync_rcv_sequence()
        map_seq = X - len, end_seq = X

Fallback data is mapped from 0 (subflow->iasn and msk->ack_seq are both
0), so the TFO skb no longer lines up with the rest of the stream. In
this commit, mptcp_inq_hint() then returns INT_MAX for SIOCINQ,
TCP_CM_INQ and autotuning.

After "mptcp: remove CB offset field" later in the series, the effect is
worse. Fastopen sets copied_seq = -len, and the passive fallback path
never adjusts it. __mptcp_recvmsg_mskq() then computes:

	u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;

This gives offset >= skb->len for the TFO skb and for every following
fallback skb. recvmsg skips each of them as already peeked, and
mptcp_recv_skb() on the read_sock path throws them away.

Would the application on such a connection ever see the received data?
Should tfo_iasn also be reset in mptcp_disconnect()?

> +
>  	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
>  		/* in sequence */
>  insert:

[ ... ]

> @@ -2374,6 +2396,9 @@ static bool mptcp_move_skbs(struct sock *sk)
>  	bool enqueued = false;
>  	u32 moved = 0;
>  
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +		__mptcp_sync_rcv_sequence(sk);

[Severity: Medium]
Can this lockless test_and_clear_bit() race with the non-atomic cb_flags
updates that run under sk_lock.slock while the msk is owned? For example:

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

MPTCP_PUSH_PENDING, MPTCP_RETRANSMIT, MPTCP_SYNC_SNDBUF,
MPTCP_FLUSH_JOIN_LIST, MPTCP_SYNC_STATE and MPTCP_ERROR_REPORT are
updated the same way.

A softirq __set_bit() on another CPU can load cb_flags before this atomic
clear and store it back after. MPTCP_SYNC_SEQ is then set again, and the
next mptcp_release_cb() sees it a second time. KCSAN would also report
the mix of plain and atomic accesses.

The commit message explains the atomic op with "since
__mptcp_sync_rcv_sequence() can also be called without the socket lock
held". A single atomic access does not protect a word whose other
writers are non-atomic, though.

In this commit a resurrected bit is harmless, because
__mptcp_sync_rcv_sequence() returns early once rcvd_dummy_seq is false.
After "mptcp: remove CB offset field", however, both mptcp_move_skbs()
and mptcp_release_cb() run this unconditionally whenever the bit is set:

	msk->copied_seq += msk->tfo_iasn;

A resurrected bit would add the IASN twice and stall delivery in
__mptcp_recvmsg_mskq().

Could this check move under mptcp_data_lock() using
__test_and_clear_bit(), with the writer using __set_bit(), as the other
cb_flags users do?

> +
>  	mptcp_data_lock(sk);
>  	while (mptcp_can_spool_backlog(sk, &skbs)) {
>  		mptcp_data_unlock(sk);

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..8a3ba490b5772 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

[ ... ]

> @@ -533,6 +535,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
> +	msk->tfo_iasn = subflow->iasn;
> +
> +	if (!sock_owned_by_user(sk))
> +		__mptcp_sync_rcv_sequence(sk);
> +	else
> +		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);

[Severity: Medium]
What orders the tfo_iasn store before the set_bit()? Per
Documentation/atomic_bitops.txt, set_bit() gives no ordering. The reader
in mptcp_move_skbs() tests the bit and reads tfo_iasn before taking
mptcp_data_lock(), so the spin_unlock on this side does not help:

CPU0 (owner)                         CPU1 (softirq)
ioctl(SIOCINQ)
  lock_sock()
  mptcp_move_skbs()
                                     check_fully_established()
                                       mptcp_data_lock()
                                       subflow_set_remote_key()
                                         msk->tfo_iasn = subflow->iasn;
                                         set_bit(MPTCP_SYNC_SEQ, ...);
    test_and_clear_bit(MPTCP_SYNC_SEQ)
    __mptcp_sync_rcv_sequence()
      reads old msk->tfo_iasn

On weakly ordered CPUs such as arm64 or powerpc, the owner can see the
bit set and still read the old tfo_iasn. On 32-bit, the u64 read can also
tear. __mptcp_sync_rcv_sequence() clears rcvd_dummy_seq unconditionally,
so the stale mapping is never corrected.

In this commit, that leaves the TFO skb with a dummy-like mapping and a
wrong inq hint. After "mptcp: remove CB offset field", mptcp_move_skbs()
also does copied_seq += tfo_iasn from the same read. A stale value there
throws copied_seq out of sync, and __mptcp_recvmsg_mskq() then skips
every later skb.

>  }

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-11-01  2:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-30 12:06 [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-10-30 12:06 ` [PATCH mptcp-next v19 1/5] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
2026-10-30 12:06 ` [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field Geliang Tang
2026-10-30 12:29   ` sashiko-bot
2026-10-30 12:06 ` [PATCH mptcp-next v19 3/5] mptcp: remove CB offset field Geliang Tang
2026-10-30 12:52   ` sashiko-bot
2026-10-30 12:06 ` [PATCH mptcp-next v19 4/5] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
2026-10-30 12:39   ` sashiko-bot
2026-10-30 12:06 ` [PATCH mptcp-next v19 5/5] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
2026-10-30 12:56   ` sashiko-bot
2026-10-30 14:06 ` [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
  -- strict thread matches above, loose matches on Subject: below --
2026-11-01  2:02 [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB 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