* [PATCH mptcp-next v19 1/5] mptcp: drop the mptcp_ooo_try_coalesce() helper
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 ` Geliang Tang
2026-10-30 12:06 ` [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field Geliang Tang
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Geliang Tang @ 2026-10-30 12:06 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field
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 ` 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
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2026-10-30 12:06 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni
From: Paolo Abeni <pabeni@redhat.com>
Such field is used to ensure in-sequence processing in case of fastopen.
Instead let's perform synchronization of the fastopen skb sequence
when the IASN becomes available with the 3rd ack.
When the cant_coalesce field has been introduced, commit f03afb3aeb9d
("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()") noted that updating the
already queued skb for passive fastopen socket at 3rd ack time would be
difficult and race prone. The main point is that such update don't need
to be synchronously performed at 3rd ack time, but is sufficient to
perform it before the next segment is introduced into the msk.
To such extent, add __mptcp_sync_rcv_sequence() to update the TFO skb
map_seq and end_seq using the IASN stored in msk->tfo_iasn, and call it
from __mptcp_move_skb() before any new skb lands into the msk.
When the 3rd ack arrives while the msk is owned by user space, defer the
sync via MPTCP_SYNC_SEQ in cb_flags. subflow_set_remote_key() stores
tfo_iasn and either calls __mptcp_sync_rcv_sequence() directly (when the
socket is not locked) or sets MPTCP_SYNC_SEQ for deferred processing.
mptcp_move_skbs() and mptcp_release_cb() check MPTCP_SYNC_SEQ at each
iteration before spooling the backlog, using test_and_clear_bit() since
__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.
---
net/mptcp/fastopen.c | 6 ++++--
net/mptcp/protocol.c | 33 +++++++++++++++++++++++++++++++--
net/mptcp/protocol.h | 5 ++++-
net/mptcp/subflow.c | 8 ++++++++
4 files changed, 47 insertions(+), 5 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..e855aaa59da2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
int limit = READ_ONCE(sk->sk_rcvbuf);
if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
- unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
MPTCP_SKB_CB(from)->offset ||
((to->len + from->len) > (limit >> 3)) ||
!skb_try_coalesce(to, from, fragstolen, delta))
@@ -430,7 +429,6 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
MPTCP_SKB_CB(skb)->offset = offset;
MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
- MPTCP_SKB_CB(skb)->cant_coalesce = 0;
__skb_unlink(skb, &ssk->sk_receive_queue);
@@ -438,6 +436,24 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
skb_dst_drop(skb);
}
+void __mptcp_sync_rcv_sequence(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+ struct sk_buff *skb;
+
+ if (likely(!msk->rcvd_dummy_seq))
+ return;
+
+ /* User space can have already received the TFO skb. */
+ msk->rcvd_dummy_seq = false;
+ skb = skb_peek_tail(&sk->sk_receive_queue);
+ if (!skb)
+ return;
+
+ MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
+ MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
+}
+
static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
{
u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
@@ -446,6 +462,12 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
mptcp_borrow_fwdmem(sk, skb);
+ /* Be sure to sync the eventual fastopen dummy mapping before any other
+ * skb lands into the msk.
+ */
+ if (unlikely(msk->rcvd_dummy_seq))
+ __mptcp_sync_rcv_sequence(sk);
+
if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
/* in sequence */
insert:
@@ -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);
+
mptcp_data_lock(sk);
while (mptcp_can_spool_backlog(sk, &skbs)) {
mptcp_data_unlock(sk);
@@ -3680,6 +3705,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
msk->bytes_retrans = 0;
msk->rcvspace_init = 0;
msk->fastclosing = 0;
+ msk->rcvd_dummy_seq = false;
mptcp_init_rtt_est(msk);
/* for fallback's sake */
@@ -3882,6 +3908,9 @@ static void mptcp_release_cb(struct sock *sk)
struct list_head join_list, skbs;
bool spool_bl;
+ if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+ __mptcp_sync_rcv_sequence(sk);
+
spool_bl = mptcp_can_spool_backlog(sk, &skbs);
if (!flags && !spool_bl)
break;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..b4e49d719200 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -126,13 +126,13 @@
#define MPTCP_FLUSH_JOIN_LIST 5
#define MPTCP_SYNC_STATE 6
#define MPTCP_SYNC_SNDBUF 7
+#define MPTCP_SYNC_SEQ 8
struct mptcp_skb_cb {
u64 map_seq;
u64 end_seq;
u32 offset;
u8 has_rxtstamp;
- u8 cant_coalesce;
};
#define MPTCP_SKB_CB(__skb) ((struct mptcp_skb_cb *)&((__skb)->cb[0]))
@@ -325,6 +325,7 @@ struct mptcp_sock {
u32 last_ack_recv;
unsigned long timer_ival;
u32 token;
+ u64 tfo_iasn;
unsigned long flags;
unsigned long cb_flags;
bool recovery; /* closing subflow write queue reinjected */
@@ -336,6 +337,7 @@ struct mptcp_sock {
bool use_64bit_ack; /* Set when we received a 64-bit DSN */
bool csum_enabled;
bool allow_infinite_fallback;
+ bool rcvd_dummy_seq;
u8 pending_state; /* A subflow asked to set this sk_state,
* protected by the msk data lock
*/
@@ -1193,6 +1195,7 @@ void mptcp_event_pm_listener(const struct sock *ssk,
enum mptcp_event_type event);
bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
+void __mptcp_sync_rcv_sequence(struct sock *sk);
void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
struct request_sock *req);
int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..8a3ba490b577 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -515,6 +515,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
{
+ struct sock *sk = (struct sock *)msk;
+
/* active MPC subflow will reach here multiple times:
* at subflow_finish_connect() time and at 4th ack time
*/
@@ -533,6 +535,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
WRITE_ONCE(msk->ack_seq, subflow->iasn);
WRITE_ONCE(msk->can_ack, true);
atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
+ msk->tfo_iasn = subflow->iasn;
+
+ if (!sock_owned_by_user(sk))
+ __mptcp_sync_rcv_sequence(sk);
+ else
+ set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
}
static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field
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
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 12:29 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mptcp: unlocked MPTCP_SYNC_SEQ bitop on cb_flags in mptcp_move_skbs()
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is deferred
- [Medium] mptcp: unreset tfo_iasn in mptcp_disconnect() corrupts passive TFO fallback
--
commit 6271a3eb665a4e03a0631758a5637752d3b353c9
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: drop the cant_coalesce CB field
This commit removes the cant_coalesce skb control buffer field and instead
synchronizes the passive Fast Open skb sequence numbers with msk->tfo_iasn
via __mptcp_sync_rcv_sequence() when the 3rd ACK arrives or before the next
segment moves into the msk receive queue.
> 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;
> +}
[ ... ]
> @@ -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);
> +
> mptcp_data_lock(sk);
> while (mptcp_can_spool_backlog(sk, &skbs)) {
> mptcp_data_unlock(sk);
[Severity: Medium]
As reported by Sashiko on v15 [1], can calling test_and_clear_bit() on
msk->cb_flags and __mptcp_sync_rcv_sequence() here in mptcp_move_skbs()
without holding mptcp_data_lock(sk) race with concurrent non-atomic updates
to msk->cb_flags in BH or timer contexts?
While user space holds lock_sock(sk) and runs mptcp_move_skbs() (from
mptcp_recvmsg(), mptcp_ioctl(), or mptcp_recv_skb()), the 3rd ACK can arrive
in softirq on another CPU:
check_fully_established()
mptcp_propagate_state()
mptcp_data_lock(sk);
subflow_set_remote_key()
msk->tfo_iasn = subflow->iasn;
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
__set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);
mptcp_data_unlock(sk);
Other BH and timer paths (__mptcp_data_acked(), __mptcp_check_push(),
mptcp_retransmit_timer(), mptcp_finish_join(), and subflow_error_report())
also modify msk->cb_flags with non-atomic __set_bit() under
mptcp_data_lock(sk). If a BH handler loads msk->cb_flags while
MPTCP_SYNC_SEQ is set and mptcp_move_skbs() executes unlocked
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) before the BH CPU stores
the modified word back, the store can resurrect MPTCP_SYNC_SEQ. That causes
MPTCP_SYNC_SEQ to be processed a second time in mptcp_release_cb(), which in
the subsequent patch ("mptcp: remove CB offset field") adds msk->tfo_iasn to
msk->copied_seq twice.
In addition, because set_bit() is a relaxed atomic operation without release
semantics and mptcp_move_skbs() does not hold mptcp_data_lock(sk) here,
could __mptcp_sync_rcv_sequence() observe a stale msk->tfo_iasn on weakly
ordered architectures?
Finally, the commit message states that mptcp_move_skbs() checks
MPTCP_SYNC_SEQ at each iteration before spooling the backlog, but the check
is placed before mptcp_data_lock(sk) outside the while
(mptcp_can_spool_backlog(sk, &skbs)) loop. Should the MPTCP_SYNC_SEQ check
be moved inside the loop under mptcp_data_lock(sk) (using
__test_and_clear_bit() across all msk->cb_flags accessors)?
> @@ -3680,6 +3705,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
> msk->bytes_retrans = 0;
> msk->rcvspace_init = 0;
> msk->fastclosing = 0;
> + msk->rcvd_dummy_seq = false;
> mptcp_init_rtt_est(msk);
>
> /* for fallback's sake */
[Severity: Medium]
Does mptcp_disconnect() also need to reset msk->tfo_iasn = 0 (or initialize
it in __mptcp_init_sock())?
If a client MPTCP socket connects and receives an MP_CAPABLE SYN-ACK,
subflow_set_remote_key() sets msk->tfo_iasn to subflow->iasn. If the socket
then disconnects via mptcp_disconnect() and is reused as a listener via
mptcp_listen(), child sockets created by mptcp_sk_clone_init() inherit the
non-zero msk->tfo_iasn via sk_clone_lock().
If a passive TFO connection on that listener receives a SYN with MP_CAPABLE
and data (setting msk->rcvd_dummy_seq = true in
mptcp_fastopen_subflow_synack_set_params()) and then falls back to TCP on
the 3rd ACK in check_fully_established() without calling
subflow_set_remote_key(), the next incoming segment in __mptcp_move_skb()
calls __mptcp_sync_rcv_sequence(sk). Because msk->tfo_iasn still holds the
previous connection's IASN instead of 0, __mptcp_sync_rcv_sequence() will
overwrite the queued TFO skb's map_seq and end_seq with the stale
msk->tfo_iasn, breaking skb coalescing and mptcp_inq_hint() (and causing
__mptcp_recvmsg_mskq() and mptcp_recv_skb() to skip the TFO payload after
the subsequent commit removing the CB offset field).
[ ... ]
> 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
> @@ -515,6 +515,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
> struct mptcp_subflow_context *subflow,
> const struct mptcp_options_received *mp_opt)
> {
> + struct sock *sk = (struct sock *)msk;
> +
> /* active MPC subflow will reach here multiple times:
> * at subflow_finish_connect() time and at 4th ack time
> */
> @@ -533,6 +535,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
> WRITE_ONCE(msk->ack_seq, subflow->iasn);
> WRITE_ONCE(msk->can_ack, true);
> atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
> + msk->tfo_iasn = subflow->iasn;
> +
> + if (!sock_owned_by_user(sk))
> + __mptcp_sync_rcv_sequence(sk);
> + else
> + set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
> }
[Severity: Medium]
As reported by Sashiko on v15 [2], when the 3rd ACK arrives in BH context
while user space holds lock_sock(sk), subflow_set_remote_key() updates
msk->ack_seq to subflow->iasn immediately while deferring
__mptcp_sync_rcv_sequence() via MPTCP_SYNC_SEQ. Can this cause
mptcp_inq_hint() to compute a bogus value before MPTCP_SYNC_SEQ is
processed?
In net/mptcp/protocol.c, mptcp_inq_hint() computes:
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;
}
While MPTCP_SYNC_SEQ is still pending, MPTCP_SKB_CB(skb)->map_seq on the
queued TFO skb is still the dummy value (-skb->len), whereas msk->ack_seq
has already been updated to subflow->iasn (and in the later commit
"mptcp: remove CB offset field", msk->copied_seq remains -(u64)skb->len
until MPTCP_SYNC_SEQ is processed).
This state is reachable under lock_sock(sk) in three places before
release_sock(sk) runs mptcp_release_cb():
1. mptcp_recvmsg() -> __mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust()
calls mptcp_inq_hint(sk) before mptcp_move_skbs(sk), underflowing
copied -= mptcp_inq_hint(sk).
2. In mptcp_recvmsg(), mptcp_move_skbs(sk) is only called when
!list_empty(&msk->backlog_list); if the 3rd ACK carries no data payload,
backlog_list is empty and the TCP_CM_INQ cmsg path calls
mptcp_inq_hint(sk) with MPTCP_SYNC_SEQ still pending, reporting INT_MAX
(or a corrupted value based on subflow->iasn) to user space.
3. In mptcp_ioctl(SIOCINQ), if the 3rd ACK arrives in BH after
mptcp_move_skbs(sk) checks MPTCP_SYNC_SEQ and before mptcp_inq_hint(sk)
reads msk->ack_seq, SIOCINQ likewise returns a bogus value.
[1] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793361629.git.tanggeliang@kylinos.cn?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH mptcp-next v19 3/5] mptcp: remove CB offset field
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:06 ` 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
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2026-10-30 12:06 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, 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 mptcp_move_skbs()
and the msk release CB, when the socket is owned by the user-space at
remote key reception time. Such synchronization preserves any partial
progress (copy) made on the TFO packet.
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().
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 | 8 ++--
net/mptcp/protocol.c | 101 ++++++++++++++++++++-----------------------
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 15 +++++--
4 files changed, 64 insertions(+), 62 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index e18b31028e7e..78481c100e2a 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,7 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
msk = mptcp_sk(sk);
msk->rcvd_dummy_seq = true;
+ msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
mptcp_borrow_fwdmem(sk, skb);
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e855aaa59da2..3f967c4dc825 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);
@@ -451,7 +449,7 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
return;
MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
- MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
+ MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
}
static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
@@ -494,10 +492,6 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
/* Partial packet */
if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
- MPTCP_SKB_CB(skb)->offset += msk->ack_seq -
- MPTCP_SKB_CB(skb)->map_seq;
- MPTCP_SKB_CB(skb)->map_seq += msk->ack_seq -
- MPTCP_SKB_CB(skb)->map_seq;
goto insert;
}
@@ -855,7 +849,7 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
if (offset < skb->len) {
size_t len = skb->len - offset;
- mptcp_init_skb(ssk, skb, offset, len);
+ mptcp_init_skb(ssk, skb, offset);
if (own_msk) {
mptcp_subflow_lend_fwdmem(subflow, skb);
@@ -918,12 +912,9 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
- /* skip overlapping data, if any */
pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
delta);
- MPTCP_SKB_CB(skb)->offset += delta;
- MPTCP_SKB_CB(skb)->map_seq += delta;
__skb_queue_tail(&sk->sk_receive_queue, skb);
}
msk->bytes_received += end_seq - msk->ack_seq;
@@ -2207,33 +2198,24 @@ static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
}
static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
- size_t len, int flags, int copied_total,
+ size_t len, int flags, u64 *seq,
struct scm_timestamping_internal *tss,
int *cmsg_flags, struct sk_buff **last)
{
struct mptcp_sock *msk = mptcp_sk(sk);
struct sk_buff *skb, *tmp;
- int total_data_len = 0;
int copied = 0;
skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
- u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
+ u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
u32 data_len = skb->len - offset;
u32 count;
int err;
- if (flags & MSG_PEEK) {
- /* skip already peeked skbs */
- if (total_data_len + data_len <= copied_total) {
- total_data_len += data_len;
- *last = skb;
- continue;
- }
-
- /* skip the already peeked data in the current skb */
- delta = copied_total - total_data_len;
- offset += delta;
- data_len -= delta;
+ /* Skip the already peeked data. */
+ if (offset >= skb->len) {
+ *last = skb;
+ continue;
}
count = min_t(size_t, len - copied, data_len);
@@ -2252,14 +2234,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 {
@@ -2396,8 +2376,10 @@ 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))
+ if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+ msk->copied_seq += msk->tfo_iasn;
__mptcp_sync_rcv_sequence(sk);
+ }
mptcp_data_lock(sk);
while (mptcp_can_spool_backlog(sk, &skbs)) {
@@ -2418,22 +2400,17 @@ static bool mptcp_move_skbs(struct sock *sk)
static unsigned int mptcp_inq_hint(const struct sock *sk)
{
const struct mptcp_sock *msk = mptcp_sk(sk);
- const struct sk_buff *skb;
-
- 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;
- if (hint_val >= INT_MAX)
- return INT_MAX;
+ hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
+ if (hint_val >= INT_MAX)
+ return INT_MAX;
- return (unsigned int)hint_val;
- }
-
- if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
+ if (!hint_val &&
+ (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
return 1;
- return 0;
+ return (unsigned int)hint_val;
}
static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
@@ -2442,6 +2419,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;
@@ -2460,6 +2438,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;
@@ -2469,7 +2452,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)
@@ -2479,8 +2462,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()
@@ -2523,6 +2509,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);
@@ -3699,6 +3689,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
mptcp_pm_data_reset(msk);
mptcp_ca_reset(sk);
msk->bytes_consumed = 0;
+ msk->copied_seq = 0;
msk->bytes_acked = 0;
msk->bytes_received = 0;
msk->bytes_sent = 0;
@@ -3908,8 +3899,10 @@ static void mptcp_release_cb(struct sock *sk)
struct list_head join_list, skbs;
bool spool_bl;
- if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+ if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+ msk->copied_seq += msk->tfo_iasn;
__mptcp_sync_rcv_sequence(sk);
+ }
spool_bl = mptcp_can_spool_backlog(sk, &skbs);
if (!flags && !spool_bl)
@@ -4630,7 +4623,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;
@@ -4672,11 +4665,9 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
copied += count;
msk->bytes_consumed += count;
- if (count < data_len) {
- MPTCP_SKB_CB(skb)->offset += count;
- MPTCP_SKB_CB(skb)->map_seq += count;
+ msk->copied_seq += count;
+ if (count < data_len)
break;
- }
mptcp_eat_recv_skb(sk, skb);
if (!desc->count)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index b4e49d719200..7878e6ac6c0b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -131,7 +131,6 @@
struct mptcp_skb_cb {
u64 map_seq;
u64 end_seq;
- u32 offset;
u8 has_rxtstamp;
};
@@ -311,6 +310,7 @@ struct mptcp_sock {
u64 rcv_data_fin_seq;
u64 bytes_retrans;
u64 bytes_consumed;
+ u64 copied_seq;
int snd_burst;
int old_wspace;
u64 recovery_snd_nxt; /* in recovery mode accept up to this seq;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8a3ba490b577..71c3b148cf04 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -537,10 +537,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
msk->tfo_iasn = subflow->iasn;
- if (!sock_owned_by_user(sk))
+ if (!sock_owned_by_user(sk)) {
+ msk->copied_seq += subflow->iasn;
__mptcp_sync_rcv_sequence(sk);
- else
+ } else {
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
+ }
}
static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
@@ -557,6 +559,13 @@ 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) */
+ 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)) {
@@ -1493,7 +1502,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
skb = skb_peek(&ssk->sk_receive_queue);
subflow->map_valid = 1;
subflow->map_data_len = skb->len;
- subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
+ subflow->map_subflow_seq = TCP_SKB_CB(skb)->seq - subflow->ssn_offset;
subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
subflow->iasn +
TCP_SKB_CB(skb)->seq -
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH mptcp-next v19 3/5] mptcp: remove CB offset field
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
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 12:52 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero subflow offset
- [High] mptcp: unsynchronized copied_seq when MPTCP_SYNC_SEQ is deferred
- [Medium] mptcp: unlocked test_and_clear_bit() on cb_flags in mptcp_move_skbs()
- [Medium] mptcp: missing reset of tfo_iasn in mptcp_disconnect() and __mptcp_init_sock()
--
commit 373a78c8c2985557d0ce492ed496a374c5080029
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: remove CB offset field
This commit replaces the per-skb MPTCP_SKB_CB(skb)->offset field with an
msk-level copied_seq counter to track bytes consumed from the receive queue.
It also updates fastopen sequence synchronization, receive queue coalescing,
and mptcp_inq_hint() to compute offsets and hints from copied_seq.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e855aaa59da29..3f967c4dc825f 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;
[Severity: High]
As reported by Sashiko on v18 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() while removing the
MPTCP_SKB_CB(from)->offset check in __mptcp_try_coalesce() corrupt the
MPTCP receive stream when offset > 0?
In __mptcp_move_skbs_from_subflow(), the skb at the head of
ssk->sk_receive_queue can have a non-zero offset (tp->copied_seq -
TCP_SKB_CB(skb)->seq > 0) while starting a new DSS mapping M2 at
tp->copied_seq with DSN mapped_dsn = mptcp_subflow_get_mapped_dsn(subflow).
For example, in subflow_check_data_avail() -> get_mapping_status(), a single
TCP skb can carry the tail of a previous DSS mapping M1 in bytes
0 .. offset - 1 and a new DSS mapping M2 in its SKB_EXT_MPTCP extension
starting at byte offset. When M1's remaining bytes were already received on
another subflow (before64(ack_seq, old_ack)), get_mapping_status() sees
!skb_is_fully_mapped(ssk, skb) and keeps SKB_EXT_MPTCP on the skb,
mptcp_subflow_discard_data() advances tp->copied_seq by offset without
eating the skb and resets subflow->map_valid = 0, and the next iteration of
subflow_check_data_avail() installs M2 starting at tp->copied_seq. A
partially overlapping TCP segment queued by tcp_data_queue() or
tcp_ofo_queue() at a mapping boundary can also produce offset > 0.
Because MPTCP stripes data across multiple subflows, the discarded prefix
bytes in skb->data[0 .. offset) do not belong to DSN range
[mapped_dsn - offset, mapped_dsn). Setting map_seq = mapped_dsn - offset and
end_seq = map_seq + skb->len appears to cause three issues:
1. In __mptcp_add_backlog(), __mptcp_move_skb(), mptcp_data_queue_ofo(), and
__mptcp_ofo_queue(), whenever mapped_dsn - offset equals to->end_seq,
__mptcp_try_coalesce() calls skb_try_coalesce(to, from, ...), which appends
all from->len bytes starting from byte 0 (including the discarded prefix
from->data[0 .. offset)) onto to.
2. When M2 is out of order at the MPTCP level (mapped_dsn > msk->ack_seq)
because [msk->ack_seq, mapped_dsn) is in flight on another subflow, and
mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb() sees
MPTCP_SKB_CB(skb)->map_seq <= msk->ack_seq and treats the out-of-order skb
as in-sequence or a partial packet starting at msk->ack_seq. It advances
msk->ack_seq and msk->bytes_received across the unreceived gap
[msk->ack_seq, mapped_dsn), queues or coalesces the skb into
sk->sk_receive_queue, delivers the stale bytes from skb->data[0 .. offset)
to user space in __mptcp_recvmsg_mskq() and mptcp_recv_skb(), and causes the
genuine data for [msk->ack_seq, mapped_dsn) arriving on the other subflow to
be dropped as MPTCP_MIB_DUPDATA.
3. When mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts
the skb into msk->out_of_order_queue at mapped_dsn - offset, where it can
coalesce with or replace legitimate out-of-order segments covering
[mapped_dsn - offset, mapped_dsn) and be dequeued early by
__mptcp_ofo_queue() once msk->ack_seq reaches mapped_dsn - offset.
[ ... ]
> @@ -2396,8 +2376,10 @@ 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))
> + if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> + msk->copied_seq += msk->tfo_iasn;
> __mptcp_sync_rcv_sequence(sk);
> + }
>
> mptcp_data_lock(sk);
> while (mptcp_can_spool_backlog(sk, &skbs)) {
[Severity: Medium]
As reported by Sashiko on v15 [2], is it safe to call
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) and
__mptcp_sync_rcv_sequence(sk) in mptcp_move_skbs() before acquiring
mptcp_data_lock(sk)?
All other flags in msk->cb_flags share the same unsigned long word and are
updated when sock_owned_by_user(sk) is true under mptcp_data_lock(sk) using
non-atomic __set_bit() or |=, such as in mptcp_propagate_state() right after
subflow_set_remote_key():
mptcp_propagate_state()
subflow_set_remote_key()
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
...
__set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);
as well as in __mptcp_data_acked(), __mptcp_check_push(),
mptcp_retransmit_timer(), mptcp_subflow_process_delegated(),
mptcp_finish_join(), subflow_error_report(), and
mptcp_check_and_set_pending().
If a BH or timer handler on another CPU holds mptcp_data_lock(sk) and
performs a non-atomic read-modify-write on msk->cb_flags while
MPTCP_SYNC_SEQ is set, and mptcp_move_skbs() clears MPTCP_SYNC_SEQ without
holding mptcp_data_lock(sk) between that load and store, can the store
restore MPTCP_SYNC_SEQ to 1 and cause mptcp_release_cb() or a later
mptcp_move_skbs() call to add msk->tfo_iasn to msk->copied_seq a second
time?
> @@ -2418,22 +2400,17 @@ static bool mptcp_move_skbs(struct sock *sk)
> static unsigned int mptcp_inq_hint(const struct sock *sk)
> {
> const struct mptcp_sock *msk = mptcp_sk(sk);
> - const struct sk_buff *skb;
> -
> - 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;
>
> - if (hint_val >= INT_MAX)
> - return INT_MAX;
> + hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> + if (hint_val >= INT_MAX)
> + return INT_MAX;
>
> - return (unsigned int)hint_val;
> - }
> -
> - if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> + if (!hint_val &&
> + (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> return 1;
>
> - return 0;
> + return (unsigned int)hint_val;
> }
[Severity: High]
As reported by Sashiko on v15 [2], can deferring the msk->copied_seq update
via MPTCP_SYNC_SEQ in subflow_set_remote_key() leave msk->copied_seq out of
sync with sk->sk_receive_queue and msk->ack_seq?
When subflow_set_remote_key() runs in BH context while sock_owned_by_user(sk)
is true, it immediately updates WRITE_ONCE(msk->ack_seq, subflow->iasn) and
msk->tfo_iasn = subflow->iasn under mptcp_data_lock(sk), while deferring
msk->copied_seq += subflow->iasn and __mptcp_sync_rcv_sequence(sk) via
MPTCP_SYNC_SEQ. This appears to leave two windows where sequence numbers are
mismatched:
1. In mptcp_move_skbs(), test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)
is only checked once before acquiring mptcp_data_lock(sk) and outside the
while (mptcp_can_spool_backlog(sk, &skbs)) loop (which drops
mptcp_data_lock(sk) on each iteration). If the 3rd ACK carrying MP_CAPABLE
and data arrives in BH after that initial check, subflow_set_remote_key()
sets msk->tfo_iasn and MPTCP_SYNC_SEQ, and mptcp_data_ready() queues the new
skb onto msk->backlog_list.
mptcp_move_skbs() then splices msk->backlog_list and calls
__mptcp_move_skbs() -> __mptcp_move_skb(), which sees msk->rcvd_dummy_seq
and calls __mptcp_sync_rcv_sequence(sk) directly:
__mptcp_move_skb()
if (unlikely(msk->rcvd_dummy_seq))
__mptcp_sync_rcv_sequence(sk);
This clears msk->rcvd_dummy_seq = false and rebases the TFO skb in
sk->sk_receive_queue to map_seq = msk->tfo_iasn - skb->len, without adding
msk->tfo_iasn to msk->copied_seq. When mptcp_move_skbs() returns to
mptcp_recv_skb() or mptcp_recvmsg(), sk->sk_receive_queue is in the
post-IASN sequence space while msk->copied_seq (and peek_seq) is still in
the pre-IASN sequence space. In mptcp_recv_skb(),
offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq underflows by
msk->tfo_iasn, causing mptcp_eat_recv_skb() to drop unread skbs (or return a
corrupted offset if (u32)(-msk->tfo_iasn) < skb->len), and
__mptcp_recvmsg_mskq() skips unread skbs as already-peeked data.
2. While MPTCP_SYNC_SEQ is still pending, msk->ack_seq holds subflow->iasn
while msk->copied_seq is still pre-IASN. Calling mptcp_inq_hint(sk) before
MPTCP_SYNC_SEQ is processed (such as in __mptcp_recvmsg_mskq() ->
mptcp_rcv_space_adjust(), in mptcp_recvmsg() for TCP_CM_INQ when
msk->backlog_list is empty after a pure 3rd ACK without payload, in
__mptcp_read_sock() -> mptcp_rcv_space_adjust(), or in mptcp_ioctl(SIOCINQ)
if the remote key arrives after mptcp_move_skbs()) subtracts the pre-IASN
msk->copied_seq from the post-IASN msk->ack_seq, returning INT_MAX (or an
IASN-inflated value) and underflowing copied -= mptcp_inq_hint(sk) in
mptcp_rcv_space_adjust().
[ ... ]
> @@ -3699,6 +3689,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
> mptcp_pm_data_reset(msk);
> mptcp_ca_reset(sk);
> msk->bytes_consumed = 0;
> + msk->copied_seq = 0;
> msk->bytes_acked = 0;
> msk->bytes_received = 0;
> msk->bytes_sent = 0;
[Severity: Medium]
Should mptcp_disconnect() (and __mptcp_init_sock()) also reset
msk->tfo_iasn = 0?
If an MPTCP socket connects and receives a remote key,
subflow_set_remote_key() sets msk->tfo_iasn = subflow->iasn. If the socket
is later disconnected via mptcp_disconnect() and reused as a listener, child
sockets cloned in mptcp_sk_clone_init() via sk_clone_lock() will inherit the
stale non-zero msk->tfo_iasn.
If a passive TFO connection on that listener queues a SYN payload in
mptcp_fastopen_subflow_synack_set_params() (setting map_seq and
msk->copied_seq to -(u64)skb->len and msk->rcvd_dummy_seq = true) and then
falls back to TCP on the 3rd ACK without receiving MP_CAPABLE,
subflow_set_remote_key() is never called on the child. When the next
fallback data segment arrives in __mptcp_move_skb(),
__mptcp_sync_rcv_sequence() will rebase the TFO skb's map_seq to
msk->tfo_iasn - skb->len using the stale msk->tfo_iasn while
msk->copied_seq remains -(u64)skb->len.
[1] https://lore.kernel.org/r/3cc892c20ed48f8f31f40e7a9ff7363d4b7b0806.1793349369.git.geliang@kernel.org
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793361629.git.tanggeliang@kylinos.cn?part=3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH mptcp-next v19 4/5] mptcp: sync mptcp skb cb layout with tcp one
2026-10-30 12:06 [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (2 preceding siblings ...)
2026-10-30 12:06 ` [PATCH mptcp-next v19 3/5] mptcp: remove CB offset field Geliang Tang
@ 2026-10-30 12:06 ` 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 14:06 ` [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
5 siblings, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2026-10-30 12:06 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, 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 | 94 +++++++++++++++++++++++++++-----------------
net/mptcp/protocol.h | 7 +++-
3 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 78481c100e2a..bd52e9528b29 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -48,8 +48,10 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
/* The TFO segment data sits before the IASN; before receiving
* the remote key, IASN is assumed being 0.
*/
- MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
+ MPTCP_SKB_CB(skb)->map_seq64 = -(u64)skb->len;
+ MPTCP_SKB_CB(skb)->map_seq = MPTCP_SKB_CB(skb)->map_seq64;
MPTCP_SKB_CB(skb)->end_seq = 0;
+ MPTCP_SKB_CB(skb)->flags = 0;
MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
mptcp_data_lock(sk);
@@ -57,7 +59,7 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
msk = mptcp_sk(sk);
msk->rcvd_dummy_seq = true;
- msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
+ msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq64;
mptcp_borrow_fwdmem(sk, skb);
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3f967c4dc825..3ff07b9de08c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,11 +168,13 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
int limit = READ_ONCE(sk->sk_rcvbuf);
if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
+ MPTCP_SKB_CB(from)->map_seq64 != MPTCP_SKB_CB(to)->map_seq64 +
+ to->len ||
((to->len + from->len) > (limit >> 3)) ||
!skb_try_coalesce(to, from, fragstolen, delta))
return false;
- pr_debug("colesced seq %llx into %llx new len %d new end seq %llx\n",
+ pr_debug("colesced seq %x into %x new len %d new end seq %x\n",
MPTCP_SKB_CB(from)->map_seq, MPTCP_SKB_CB(to)->map_seq,
to->len, MPTCP_SKB_CB(from)->end_seq);
MPTCP_SKB_CB(to)->end_seq = MPTCP_SKB_CB(from)->end_seq;
@@ -254,8 +256,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
struct sk_buff *skb = rb_to_skb(node);
/* Stop pruning if the incoming skb would land in OoO tail. */
- if (after64(MPTCP_SKB_CB(in_skb)->map_seq,
- MPTCP_SKB_CB(skb)->map_seq))
+ if (after(MPTCP_SKB_CB(in_skb)->map_seq,
+ MPTCP_SKB_CB(skb)->map_seq))
break;
pruned = true;
@@ -301,16 +303,23 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
{
struct sock *sk = (struct sock *)msk;
struct rb_node **p, *parent;
- u64 seq, end_seq, max_seq;
+ u64 end_seq, max_seq;
struct sk_buff *skb1;
+ u32 seq;
seq = MPTCP_SKB_CB(skb)->map_seq;
- end_seq = MPTCP_SKB_CB(skb)->end_seq;
+ end_seq = MPTCP_SKB_CB(skb)->map_seq64 + skb->len;
max_seq = atomic64_read(&msk->rcv_wnd_sent);
+ if (after64(READ_ONCE(msk->ack_seq), max_seq))
+ max_seq = READ_ONCE(msk->ack_seq);
- pr_debug("msk=%p seq=%llx limit=%llx empty=%d\n", msk, seq, max_seq,
+ pr_debug("msk=%p seq=%x limit=%llx empty=%d\n", msk, seq, max_seq,
RB_EMPTY_ROOT(&msk->out_of_order_queue));
- if (after64(end_seq, max_seq)) {
+ /* Use the full sequence space to perform the admission checks, to
+ * protect vs possible wrap-arounds.
+ */
+ if (after64(MPTCP_SKB_CB(skb)->map_seq64, max_seq) ||
+ after64(end_seq, max_seq)) {
/* out of window */
mptcp_drop(sk, skb);
pr_debug("oow by %lld, rcv_wnd_sent %llu\n",
@@ -345,7 +354,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
}
/* Can avoid an rbtree lookup if we are adding skb after ooo_last_skb */
- if (!before64(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
+ if (!before(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
parent = &msk->ooo_last_skb->rbnode;
p = &parent->rb_right;
@@ -357,18 +366,18 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
while (*p) {
parent = *p;
skb1 = rb_to_skb(parent);
- if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+ if (before(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
p = &parent->rb_left;
continue;
}
- if (before64(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
- if (!after64(end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+ if (before(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+ if (!after((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
/* All the bits are present. Drop. */
mptcp_drop(sk, skb);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
return;
}
- if (after64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+ if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
/* partial overlap:
* | skb |
* | skb1 |
@@ -399,7 +408,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
merge_right:
/* Remove other segments covered by skb. */
while ((skb1 = skb_rb_next(skb)) != NULL) {
- if (before64(end_seq, MPTCP_SKB_CB(skb1)->end_seq))
+ if (before((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq))
break;
rb_erase(&skb1->rbnode, &msk->out_of_order_queue);
mptcp_drop(sk, skb1);
@@ -421,11 +430,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);
@@ -448,13 +459,14 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
if (!skb)
return;
- MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
+ MPTCP_SKB_CB(skb)->map_seq64 = msk->tfo_iasn - skb->len;
+ MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
}
static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
{
- u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
+ u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
struct mptcp_sock *msk = mptcp_sk(sk);
struct sk_buff *tail;
@@ -466,7 +478,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
if (unlikely(msk->rcvd_dummy_seq))
__mptcp_sync_rcv_sequence(sk);
- if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
+ if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
/* in sequence */
insert:
if (!mptcp_try_rmem_schedule(sk, skb)) {
@@ -484,14 +496,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;
}
@@ -886,39 +898,37 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
{
struct sock *sk = (struct sock *)msk;
struct sk_buff *skb, *tail;
+ u32 seq_delta, ack_seq;
bool moved = false;
struct rb_node *p;
- u64 end_seq;
p = rb_first(&msk->out_of_order_queue);
pr_debug("msk=%p empty=%d\n", msk, RB_EMPTY_ROOT(&msk->out_of_order_queue));
while (p) {
+ ack_seq = msk->ack_seq;
skb = rb_to_skb(p);
- if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq))
+ if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
break;
p = rb_next(p);
rb_erase(&skb->rbnode, &msk->out_of_order_queue);
- if (unlikely(!after64(MPTCP_SKB_CB(skb)->end_seq,
- msk->ack_seq))) {
+ if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
mptcp_drop(sk, skb);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
continue;
}
- end_seq = MPTCP_SKB_CB(skb)->end_seq;
+ seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;
tail = skb_peek_tail(&sk->sk_receive_queue);
if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
- int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
-
- pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
- MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
- delta);
+ pr_debug("uncoalesced seq=%x ack seq=%x delta=%d\n",
+ MPTCP_SKB_CB(skb)->map_seq, ack_seq,
+ ack_seq - MPTCP_SKB_CB(skb)->map_seq);
__skb_queue_tail(&sk->sk_receive_queue, skb);
}
- msk->bytes_received += end_seq - msk->ack_seq;
- WRITE_ONCE(msk->ack_seq, end_seq);
+ msk->bytes_received += seq_delta;
+ WRITE_ONCE(msk->ack_seq, msk->ack_seq + seq_delta);
moved = true;
}
return moved;
@@ -2207,7 +2217,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;
@@ -4623,7 +4633,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;
@@ -4873,11 +4883,23 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
return work_done;
}
+#define CHK_CB_FIELD(mptcp_field, tcp_field) \
+ ({ \
+ BUILD_BUG_ON(offsetof(struct mptcp_skb_cb, mptcp_field) != \
+ offsetof(struct tcp_skb_cb, tcp_field)); \
+ BUILD_BUG_ON(offsetofend(struct mptcp_skb_cb, mptcp_field) != \
+ offsetofend(struct tcp_skb_cb, tcp_field)); \
+ })
+
void __init mptcp_proto_init(void)
{
struct mptcp_delegated_action *delegated;
int cpu;
+ CHK_CB_FIELD(map_seq, seq);
+ CHK_CB_FIELD(end_seq, end_seq);
+ CHK_CB_FIELD(flags, tcp_flags);
+
mptcp_prot.h.hashinfo = tcp_prot.h.hashinfo;
if (percpu_counter_init(&mptcp_sockets_allocated, 0, GFP_KERNEL))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7878e6ac6c0b..fa54f70ab501 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -129,9 +129,12 @@
#define MPTCP_SYNC_SEQ 8
struct mptcp_skb_cb {
- u64 map_seq;
- u64 end_seq;
+ u32 map_seq;
+ u32 end_seq;
+ u32 unused;
+ u16 flags;
u8 has_rxtstamp;
+ u64 map_seq64;
};
#define MPTCP_SKB_CB(__skb) ((struct mptcp_skb_cb *)&((__skb)->cb[0]))
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH mptcp-next v19 4/5] mptcp: sync mptcp skb cb layout with tcp one
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
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 12:39 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: subtracting offset from map_seq64 in mptcp_init_skb() corrupts OFO data
- [High] mptcp: TFO sequence desync in mptcp_move_skbs() and mptcp_inq_hint()
--
commit 078393e5a4729e83b409065c8fc8170a9248094b
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: sync mptcp skb cb layout with tcp one
This commit updates struct mptcp_skb_cb to store 32-bit map_seq and end_seq
fields aligned with struct tcp_skb_cb alongside a 64-bit map_seq64 field, and
converts receive and out-of-order queue operations to use 32-bit sequence
arithmetic after window validation.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3f967c4dc825f..3ff07b9de08c0 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -421,11 +430,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;
[Severity: Critical]
As reported by Sashiko on v18 [1], can subtracting offset from map_seq64 and
map_seq in mptcp_init_skb() corrupt out-of-order data when offset is non-zero?
In __mptcp_move_skbs_from_subflow(), offset = tp->copied_seq -
TCP_SKB_CB(skb)->seq is non-zero when the leading [0, offset) bytes of the
subflow skb were already consumed or discarded (for instance, when an skb
straddles the end of a discarded DSS mapping in subflow_check_data_avail() and
the start of the next mapping, or when TCP queues a partially overlapping
segment). In subflow_check_data_avail(), validate_mapping() and
validate_data_csum() only validate the active mapping starting at
tp->copied_seq, which corresponds to mapped_dsn =
mptcp_subflow_get_mapped_dsn(subflow).
Subtracting offset from map_seq64 without pulling the leading offset bytes
assumes msk->ack_seq and msk->copied_seq have already reached mapped_dsn,
which only holds for in-order data. When the active mapping is out of order
(mapped_dsn > msk->ack_seq) and offset > 0:
1. If mapped_dsn - offset <= msk->ack_seq, __mptcp_move_skb() sees map_seq64
<= msk->ack_seq and treats the skb as in-sequence or partial data, queueing
or coalescing it into sk->sk_receive_queue and advancing msk->ack_seq
across the unreceived gap [msk->ack_seq, mapped_dsn). Both
__mptcp_recvmsg_mskq() and mptcp_recv_skb() then compute a read offset of
(u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq < offset and copy the
unmapped leading subflow bytes to user space.
2. If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
skb into out_of_order_queue covering [mapped_dsn - offset, mapped_dsn +
skb->len - offset). __mptcp_try_coalesce() can then coalesce those leading
[0, offset) prefix bytes into a preceding OFO skb ending at mapped_dsn -
offset, legitimate segments arriving for [mapped_dsn - offset, mapped_dsn)
are dropped as MPTCP_MIB_DUPDATA, and __mptcp_ofo_queue() dequeues the skb
as soon as msk->ack_seq reaches mapped_dsn - offset.
>
> __skb_unlink(skb, &ssk->sk_receive_queue);
> @@ -448,13 +459,14 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
> if (!skb)
> return;
>
> - MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
> + MPTCP_SKB_CB(skb)->map_seq64 = msk->tfo_iasn - skb->len;
> + MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
> MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
> }
>
> static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
> {
> - u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
> + u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
> struct mptcp_sock *msk = mptcp_sk(sk);
> struct sk_buff *tail;
>
> @@ -466,7 +478,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
> if (unlikely(msk->rcvd_dummy_seq))
> __mptcp_sync_rcv_sequence(sk);
[Severity: High]
As reported by Sashiko on v18 [2], can __mptcp_move_skb() desynchronize the
TFO skb's map_seq from msk->copied_seq when subflow_set_remote_key() runs
while the msk socket is owned by user space?
During passive TFO, mptcp_fastopen_subflow_synack_set_params() places the TFO
skb on sk->sk_receive_queue with map_seq64 = -(u64)skb->len, sets
msk->rcvd_dummy_seq = true, and initializes msk->copied_seq = -(u64)skb->len.
If the third ACK carrying MP_CAPABLE arrives while sock_owned_by_user(sk) is
true, subflow_set_remote_key() under mptcp_data_lock(sk) immediately updates
msk->ack_seq and msk->tfo_iasn to subflow->iasn, and defers the
msk->copied_seq update by 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);
msk->tfo_iasn = subflow->iasn;
if (!sock_owned_by_user(sk)) {
msk->copied_seq += subflow->iasn;
__mptcp_sync_rcv_sequence(sk);
} else {
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
}
}
In mptcp_move_skbs(), MPTCP_SYNC_SEQ is only checked once before acquiring
mptcp_data_lock(sk) and before the mptcp_can_spool_backlog() loop:
net/mptcp/protocol.c:mptcp_move_skbs() {
if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
msk->copied_seq += msk->tfo_iasn;
__mptcp_sync_rcv_sequence(sk);
}
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);
}
...
}
If subflow_set_remote_key() and an incoming data skb run after that initial
test_and_clear_bit() (or while mptcp_data_lock(sk) is dropped inside the
loop), __mptcp_move_skbs() -> __mptcp_move_skb() sees msk->rcvd_dummy_seq and
calls __mptcp_sync_rcv_sequence(sk). That updates the TFO skb's map_seq to
(u32)(msk->tfo_iasn - skb->len) and clears msk->rcvd_dummy_seq while
msk->copied_seq is still -(u64)skb->len and MPTCP_SYNC_SEQ remains pending.
When mptcp_move_skbs() returns to mptcp_recv_skb() or mptcp_recvmsg() ->
__mptcp_recvmsg_mskq(), the offset calculation:
offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
evaluates to (u32)(-msk->tfo_iasn), which underflows to >= skb->len and causes
mptcp_recv_skb() to drop the unread skb via mptcp_eat_recv_skb() and
__mptcp_recvmsg_mskq() to skip it.
In addition, while MPTCP_SYNC_SEQ is pending under lock_sock(sk) before
release_sock(sk) runs mptcp_release_cb(), mptcp_inq_hint() computes
READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq) using the IASN-shifted
msk->ack_seq and the unshifted msk->copied_seq, returning INT_MAX.
[1] https://lore.kernel.org/r/ad478dff911f705f9d49c66a1a0ef1794b59c989.1793349369.git.geliang@kernel.org
[2] https://lore.kernel.org/r/ad478dff911f705f9d49c66a1a0ef1794b59c989.1793349369.git.geliang@kernel.org
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793361629.git.tanggeliang@kylinos.cn?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH mptcp-next v19 5/5] mptcp: defer read_sock cleanup to mptcp_worker
2026-10-30 12:06 [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (3 preceding siblings ...)
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:06 ` 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
5 siblings, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2026-10-30 12:06 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang, Paolo Abeni
From: Geliang Tang <tanggeliang@kylinos.cn>
When MPTCP carries TLS, __mptcp_read_sock() is reached from BH context
(via tls_strp_read_copyin() -> sk->sk_read_sock()), where calling
mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() is not safe as both
expect process context.
Move both calls to mptcp_worker(): __mptcp_read_sock() accumulates the
consumed bytes 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 rcvbuf autotuning and RBU update in process
context.
Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()), drop the msk_owned_by_me() assertion (BH caller
does not own the msk lock), and drop the redundant mptcp_recv_skb()
call after the main loop.
Initialize msk->read_copied in __mptcp_init_sock() and reset it in
mptcp_disconnect().
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
net/mptcp/protocol.c | 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 3ff07b9de08c..13078f2f3c49 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3143,6 +3143,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;
@@ -3218,6 +3233,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);
@@ -3261,6 +3279,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);
@@ -3707,6 +3726,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
msk->rcvspace_init = 0;
msk->fastclosing = 0;
msk->rcvd_dummy_seq = false;
+ msk->read_copied = 0;
mptcp_init_rtt_est(msk);
/* for fallback's sake */
@@ -4629,9 +4649,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) {
@@ -4646,6 +4663,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)
@@ -4655,8 +4673,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) {
@@ -4687,11 +4703,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 fa54f70ab501..3580cf66f303 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -117,6 +117,7 @@
#define MPTCP_FALLBACK_DONE 2
#define MPTCP_WORK_CLOSE_SUBFLOW 3
#define MPTCP_RTX_ENABLED 4
+#define MPTCP_WORK_READ_COMPLETE 5
/* MPTCP socket release cb flags */
#define MPTCP_PUSH_PENDING 1
@@ -326,6 +327,7 @@ struct mptcp_sock {
u32 last_data_sent;
u32 last_data_recv;
u32 last_ack_recv;
+ int read_copied;
unsigned long timer_ival;
u32 token;
u64 tfo_iasn;
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH mptcp-next v19 5/5] mptcp: defer read_sock cleanup to mptcp_worker
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
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 12:56 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: unsynchronized copied_seq when MPTCP_SYNC_SEQ is deferred
- [High] mptcp: wrong map_seq64 in mptcp_init_skb() on non-zero skb offset
--
commit 1033c608c403db5f17f6be77476b0bb66333000f
Author: Geliang Tang <geliang@kernel.org>
mptcp: defer read_sock cleanup to mptcp_worker
This series reworks MPTCP receive sequence tracking to use msk->copied_seq and
32-bit SKB control block sequence numbers aligned with TCP, and defers
__mptcp_read_sock() receive buffer autotuning and cleanup to mptcp_worker().
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..13078f2f3c498 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -424,21 +423,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;
> + 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 +445,40 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> skb_dst_drop(skb);
> }
[ ... ]
> 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) {
> + /* 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_seq64 == msk->ack_seq) {
> /* in sequence */
> insert:
> if (!mptcp_try_rmem_schedule(sk, skb)) {
> @@ -472,18 +496,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;
> - 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;
> + 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;
> }
[Severity: High]
As reported by Sashiko on v18 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() corrupt the receive
stream when an out-of-order subflow skb has a non-zero offset?
In __mptcp_move_skbs_from_subflow(), offset = seq - TCP_SKB_CB(skb)->seq is
non-zero when the leading offset bytes of skb on ssk->sk_receive_queue were
already consumed or discarded (for instance, when skb spans the end of a
duplicate DSS mapping discarded by mptcp_subflow_discard_data() in
subflow_check_data_avail() and the start of a new DSS mapping at
tp->copied_seq).
Setting MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
offset assigns DSNs [mapped_dsn - offset, mapped_dsn) to those already
discarded leading bytes 0..offset of skb.
When the new DSS mapping starting at offset is out-of-order at the MPTCP level
(mapped_dsn > msk->ack_seq because the intervening DSN range
[msk->ack_seq, mapped_dsn) is in flight on another subflow):
1. If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
sees map_seq64 <= msk->ack_seq and map_seq64 + skb->len > msk->ack_seq. It
treats the out-of-order skb as in-sequence or a partial packet, queues it
onto sk->sk_receive_queue, and advances msk->ack_seq across the missing DSN
gap [msk->ack_seq, mapped_dsn), causing __mptcp_recvmsg_mskq() or
mptcp_recv_skb() to copy the stale leading bytes in 0..offset to user space.
2. If mapped_dsn - offset > msk->ack_seq, __mptcp_move_skb() queues skb into
msk->out_of_order_queue with map_seq = mapped_dsn - offset, so
mptcp_data_queue_ofo() and __mptcp_ofo_queue() treat bytes 0..offset as valid
data for [mapped_dsn - offset, mapped_dsn) and drop the real segments arriving
on other subflows as duplicate data.
[ ... ]
> @@ -2383,6 +2386,11 @@ 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)) {
> + msk->copied_seq += msk->tfo_iasn;
> + __mptcp_sync_rcv_sequence(sk);
> + }
> +
> mptcp_data_lock(sk);
> while (mptcp_can_spool_backlog(sk, &skbs)) {
> mptcp_data_unlock(sk);
> @@ -2402,22 +2410,17 @@ static bool mptcp_move_skbs(struct sock *sk)
> static unsigned int mptcp_inq_hint(const struct sock *sk)
> {
> const struct mptcp_sock *msk = mptcp_sk(sk);
> - const struct sk_buff *skb;
> + u64 hint_val;
>
> - skb = skb_peek(&sk->sk_receive_queue);
> - if (skb) {
> - u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> + hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> + if (hint_val >= INT_MAX)
> + return INT_MAX;
>
> - if (hint_val >= INT_MAX)
> - return INT_MAX;
> -
> - return (unsigned int)hint_val;
> - }
> -
> - if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> + if (!hint_val &&
> + (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> return 1;
>
> - return 0;
> + return (unsigned int)hint_val;
> }
>
> static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
[ ... ]
> @@ -2463,8 +2472,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;
> + }
[Severity: High]
As reported by Sashiko on v18 [2], can deferring the msk->copied_seq update
via MPTCP_SYNC_SEQ leave msk->copied_seq out of sync with msk->ack_seq and
sk->sk_receive_queue while the socket lock is held?
In subflow_set_remote_key() in net/mptcp/subflow.c, when the remote key and
IASN arrive in BH context while sock_owned_by_user(sk) is true, msk->ack_seq
is updated immediately to subflow->iasn while updating msk->copied_seq is
deferred:
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)) {
msk->copied_seq += subflow->iasn;
__mptcp_sync_rcv_sequence(sk);
} else {
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
}
This appears to leave several paths with an unsynchronized msk->copied_seq:
1. In mptcp_move_skbs(), test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) is
checked only once before mptcp_data_lock(sk) rather than inside the
while (mptcp_can_spool_backlog(sk, &skbs)) loop. If the 3rd ACK and payload
arrive in BH after that initial check or while mptcp_data_lock(sk) is dropped
around __mptcp_move_skbs(), the next iteration of the loop spools the new skb
via __mptcp_move_skb(), which calls __mptcp_sync_rcv_sequence(sk) (updating
the TFO skb in sk->sk_receive_queue to the IASN sequence space and clearing
msk->rcvd_dummy_seq) and queues the new IASN-based skb while msk->copied_seq
remains unsynchronized for the next iteration of mptcp_recvmsg().
2. While MPTCP_SYNC_SEQ is pending, mptcp_inq_hint() subtracts the
pre-IASN msk->copied_seq from the IASN-based msk->ack_seq and returns INT_MAX.
In mptcp_recvmsg(), if the 3rd ACK or SYN-ACK arrives without payload while
lock_sock(sk) is held, msk->backlog_list is empty so mptcp_move_skbs() is not
called; both mptcp_rcv_space_adjust() and the TCP_CM_INQ cmsg calculation run
before release_sock(sk) and observe INT_MAX from mptcp_inq_hint(sk). Similarly,
in mptcp_ioctl() for SIOCINQ, if the remote key arrives in BH after
mptcp_move_skbs() checks MPTCP_SYNC_SEQ and before mptcp_inq_hint(sk), SIOCINQ
returns INT_MAX.
3. In mptcp_recvmsg(), when flags & MSG_PEEK is set and mptcp_move_skbs(sk)
consumes MPTCP_SYNC_SEQ (updating msk->copied_seq and the TFO skb map_seq) but
returns false because the backlog skb was out-of-order or dropped, peek_seq is
only updated inside the if (mptcp_move_skbs(sk)) branch and remains stale if
the loop continues without sleeping.
[1] https://lore.kernel.org/r/b6e4ed70642e2f86c041381cea810db82116b836.1793349369.git.geliang@kernel.org
[2] https://lore.kernel.org/r/b6e4ed70642e2f86c041381cea810db82116b836.1793349369.git.geliang@kernel.org
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793361629.git.tanggeliang@kylinos.cn?part=5
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage
2026-10-30 12:06 [PATCH mptcp-next v19 0/5] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (4 preceding siblings ...)
2026-10-30 12:06 ` [PATCH mptcp-next v19 5/5] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
@ 2026-10-30 14:06 ` MPTCP CI
5 siblings, 0 replies; 11+ messages in thread
From: MPTCP CI @ 2026-10-30 14:06 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_simult_flows ⚠️
- 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): Unstable: 3 failed test(s): bpftest_test_progs-cpuv4_mptcp bpftest_test_progs-no_alu32_mptcp bpftest_test_progs_mptcp ⚠️
- KVM Validation: btf-debug (only bpftest_all): Unstable: 3 failed test(s): bpftest_test_progs-cpuv4_mptcp bpftest_test_progs-no_alu32_mptcp bpftest_test_progs_mptcp ⚠️
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/41054949264
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b0f44e221b7b
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1197128
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)
^ permalink raw reply [flat|nested] 11+ messages in thread