* [PATCH mptcp-next v20 1/6] mptcp: drop the mptcp_ooo_try_coalesce() helper
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 8:57 ` [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field Geliang Tang
` (5 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 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] 18+ messages in thread* [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-11-03 8:57 ` [PATCH mptcp-next v20 1/6] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 9:24 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
2026-11-03 8:57 ` [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen Geliang Tang
` (4 subsequent siblings)
6 siblings, 2 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 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..f3b07b303f55 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:
@@ -2375,6 +2397,8 @@ static bool mptcp_move_skbs(struct sock *sk)
u32 moved = 0;
mptcp_data_lock(sk);
+ if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
+ __mptcp_sync_rcv_sequence(sk);
while (mptcp_can_spool_backlog(sk, &skbs)) {
mptcp_data_unlock(sk);
enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
@@ -3680,6 +3704,8 @@ static int mptcp_disconnect(struct sock *sk, int flags)
msk->bytes_retrans = 0;
msk->rcvspace_init = 0;
msk->fastclosing = 0;
+ msk->tfo_iasn = 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] 18+ messages in thread* Re: [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field
2026-11-03 8:57 ` [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field Geliang Tang
@ 2026-11-03 9:24 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-11-03 9:24 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is deferred
--
commit 198606f4de002c65c8846681be2fa1a33a1379e5
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: drop the cant_coalesce CB field
This commit removes the cant_coalesce field from struct mptcp_skb_cb and
synchronizes the passive Fast Open skb sequence numbers with the peer's
initial ACK sequence number once the 3rd ACK arrives. When the socket is
owned by user space at 3rd ACK arrival, synchronization is deferred via
the MPTCP_SYNC_SEQ flag in cb_flags.
> 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 [1], when the 3rd ACK arrives on a passive Fast
Open socket while user space holds the socket lock, subflow_set_remote_key()
immediately updates msk->ack_seq to subflow->iasn while deferring
__mptcp_sync_rcv_sequence() via MPTCP_SYNC_SEQ.
Until MPTCP_SYNC_SEQ is processed, the TFO skb in sk->sk_receive_queue still
carries its dummy sequence mapping (MPTCP_SKB_CB(skb)->map_seq = -skb->len
plus any consumed offset). If mptcp_inq_hint() is called under lock_sock(sk)
during that window, it subtracts the dummy map_seq from the newly updated
msk->ack_seq:
net/mptcp/protocol.c:mptcp_inq_hint() {
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;
}
}
This can happen before release_sock(sk) runs mptcp_release_cb() in three
paths:
1. mptcp_recvmsg() -> __mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust()
calls mptcp_inq_hint(sk) before mptcp_move_skbs(sk) is invoked, which can
underflow copied -= mptcp_inq_hint(sk).
2. mptcp_recvmsg() skips mptcp_move_skbs(sk) when msk->backlog_list is empty
(for example when the 3rd ACK carries no payload) and then calls
mptcp_inq_hint(sk) for TCP_CM_INQ:
net/mptcp/protocol.c:mptcp_recvmsg() {
if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
continue;
...
if (cmsg_flags & MPTCP_CMSG_INQ) {
unsigned int inq = mptcp_inq_hint(sk);
put_cmsg(msg, SOL_TCP, TCP_CM_INQ, sizeof(inq), &inq);
}
}
3. In mptcp_ioctl() for SIOCINQ, if the 3rd ACK arrives in BH after
mptcp_move_skbs(sk) drops mptcp_data_lock(sk) and before
mptcp_inq_hint(sk) reads msk->ack_seq:
net/mptcp/protocol.c:mptcp_ioctl() {
lock_sock(sk);
if (mptcp_move_skbs(sk))
mptcp_cleanup_rbuf(msk, 0);
*karg = mptcp_inq_hint(sk);
release_sock(sk);
}
Could this cause mptcp_inq_hint() to return INT_MAX (or a bogus value derived
from subflow->iasn) to user space for TCP_CM_INQ and SIOCINQ, and corrupt the
receive buffer space calculation in mptcp_rcv_space_adjust()?
[1] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field
2026-11-03 8:57 ` [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field Geliang Tang
2026-11-03 9:24 ` sashiko-bot
@ 2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-05 0:41 UTC (permalink / raw)
To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is
deferred
- [Low] The commit message says MPTCP_SYNC_SEQ is consumed "using
test_and_clear_bit() since __mptcp_sync_rcv_sequence() can also be…
--
mptcp: drop the cant_coalesce CB field
This commit removes the cant_coalesce field from the MPTCP skb control
block. The passive fastopen skb's dummy mapping is now synced from the
IASN stored in msk->tfo_iasn before any other skb lands in the msk. When
the msk is owned by user space at 3rd ack time, the sync is deferred via
the new MPTCP_SYNC_SEQ cb_flags bit.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..f3b07b303f555 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2375,6 +2397,8 @@ static bool mptcp_move_skbs(struct sock *sk)
> u32 moved = 0;
>
> mptcp_data_lock(sk);
> + if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> + __mptcp_sync_rcv_sequence(sk);
> while (mptcp_can_spool_backlog(sk, &skbs)) {
> mptcp_data_unlock(sk);
> enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
[Severity: Medium]
On a passive TFO socket, what happens if the 3rd ack arrives while
user space holds the msk socket lock? It looks like msk->ack_seq and
the queued TFO skb are then out of sync until release_sock().
subflow_set_remote_key() moves msk->ack_seq to subflow->iasn right
away. The TFO skb queued by mptcp_fastopen_subflow_synack_set_params()
keeps its dummy mapping (map_seq = -skb->len, end_seq = 0) until
MPTCP_SYNC_SEQ is consumed by mptcp_move_skbs() or mptcp_release_cb().
In that window mptcp_inq_hint() still peeks that skb and computes:
u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
That evaluates to subflow->iasn + skb->len. It is almost always above
INT_MAX, so user space gets INT_MAX as the inq value. When the IASN
happens to be small, it gets a bogus value derived from it instead.
All of the following run under lock_sock(sk) and can reach
mptcp_inq_hint() while the bit is still pending:
- mptcp_recvmsg() -> __mptcp_recvmsg_mskq() ->
mptcp_rcv_space_adjust(), before mptcp_move_skbs() has run. If I
read it correctly, the copied -= mptcp_inq_hint(sk) there would
underflow as well.
- the TCP_CM_INQ cmsg in mptcp_recvmsg(). When the 3rd ack carries
no data, msk->backlog_list is empty and mptcp_move_skbs() is
skipped, so the flag is only consumed later by mptcp_release_cb().
- mptcp_ioctl(SIOCINQ). The 3rd ack can land in BH after
mptcp_move_skbs() drops the data lock and before mptcp_inq_hint()
reads msk->ack_seq.
A partial read or MSG_PEEK of the TFO data that races with the peer's
3rd ack should be enough to trigger this. The point of this patch is
to keep the TFO skb mapping consistent with msk->ack_seq, but the
deferred path still leaves a window where it is not.
Since these callers already own the socket, could they consume
MPTCP_SYNC_SEQ under mptcp_data_lock() before peeking at
sk->sk_receive_queue? Or is there another way to make the msk->ack_seq
update and the TFO skb resync visible together to the lock owner?
[Severity: Low]
This isn't a bug, but the commit message doesn't seem to match the code
here. It says:
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.
Both mptcp_move_skbs() and mptcp_release_cb() use the non-atomic
__test_and_clear_bit(). The only producer, subflow_set_remote_key() in
net/mptcp/subflow.c, uses the atomic variant:
else
set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
The other cb_flags producers use __set_bit(). Examples are
mptcp_propagate_state() setting MPTCP_SYNC_STATE and __mptcp_data_acked()
setting MPTCP_CLEAN_UNA.
The non-atomic consumers look correct, because every access to
MPTCP_SYNC_SEQ appears to be serialized by the msk spinlock
(sk_lock.slock):
- check_fully_established() wraps __mptcp_subflow_fully_established()
in mptcp_data_lock()/mptcp_data_unlock() on the 3rd ack path
- mptcp_propagate_state() holds mptcp_data_lock()
- mptcp_sk_clone_init() runs under bh_lock_sock(nsk)
- mptcp_move_skbs() holds mptcp_data_lock()
- mptcp_release_cb() runs with sk_lock.slock held
The "without the socket lock" wording seems to refer only to the owner
lock. The spinlock is what actually protects cb_flags.
Could the commit message say that the msk data lock is what protects
MPTCP_SYNC_SEQ? Could subflow_set_remote_key() also use __set_bit(), like
the other cb_flags producers?
The same mismatch is still there at the end of the series. After
"mptcp: remove CB offset field", both consumers still use
__test_and_clear_bit() and subflow_set_remote_key() still uses set_bit().
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793695518.git.tanggeliang%40kylinos.cn
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-11-03 8:57 ` [PATCH mptcp-next v20 1/6] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
2026-11-03 8:57 ` [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 9:38 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
2026-11-03 8:57 ` [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field Geliang Tang
` (3 subsequent siblings)
6 siblings, 2 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
In mptcp_fastopen_subflow_synack_set_params(), the TFO skb's map_seq is
set to -skb->len to represent data before the IASN.
Since skb->len is u32, the negation -skb->len is computed in u32
arithmetic, producing a large positive value (e.g., -1420 becomes
4294965876). When assigned to map_seq (u64), this value is zero-extended
rather than sign-extended.
This causes incorrect behavior in sequence number comparisons throughout
the code. Functions like after64() and before64() may misorder TFO skbs,
and the out-of-order queue handling may misclassify TFO data.
Fix by casting to u64 before negation: -(u64)skb->len. This ensures
proper 64-bit two's complement representation and correct semantics for
all sequence number operations.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
net/mptcp/fastopen.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index e18b31028e7e..79765cf55286 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -45,8 +45,10 @@ 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;
--
2.55.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen
2026-11-03 8:57 ` [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen Geliang Tang
@ 2026-11-03 9:38 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-11-03 9:38 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: inaccurate commit message on before64()/after64() and OoO queue
--
commit c0adf4769c9ae204c304e167fe42012e5a15d3c6
Author: Geliang Tang <geliang@kernel.org>
mptcp: fix TFO map_seq sign extension in fastopen
This commit casts skb->len to u64 before negating it when initializing the
TFO skb's dummy map_seq in mptcp_fastopen_subflow_synack_set_params(), so
that it is properly sign-extended in 64-bit arithmetic.
> mptcp: fix TFO map_seq sign extension in fastopen
>
> In mptcp_fastopen_subflow_synack_set_params(), the TFO skb's map_seq is
> set to -skb->len to represent data before the IASN.
>
> Since skb->len is u32, the negation -skb->len is computed in u32
> arithmetic, producing a large positive value (e.g., -1420 becomes
> 4294965876). When assigned to map_seq (u64), this value is zero-extended
> rather than sign-extended.
>
> This causes incorrect behavior in sequence number comparisons throughout
> the code. Functions like after64() and before64() may misorder TFO skbs,
> and the out-of-order queue handling may misclassify TFO data.
>
> Fix by casting to u64 before negation: -(u64)skb->len. This ensures
> proper 64-bit two's complement representation and correct semantics for
> all sequence number operations.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
[Severity: Medium]
Is the commit message description of the impact on after64(), before64(), and
the out-of-order queue accurate?
In mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c, the
TFO skb is queued directly to the tail of the in-order receive queue:
MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
MPTCP_SKB_CB(skb)->end_seq = 0;
MPTCP_SKB_CB(skb)->offset = 0;
...
__skb_queue_tail(&sk->sk_receive_queue, skb);
Because it goes directly onto sk->sk_receive_queue, it never passes through
__mptcp_move_skb(), mptcp_data_queue_ofo(), or msk->out_of_order_queue, and
its dummy map_seq is not compared with before64() or after64(). When
subsequent skbs arrive in __mptcp_move_skb(), __mptcp_sync_rcv_sequence()
updates the TFO skb's dummy map_seq and end_seq using msk->tfo_iasn before
any comparison or coalescing takes place, and __mptcp_try_coalesce() checks
MPTCP_SKB_CB(to)->end_seq rather than map_seq.
Instead, the missing sign extension affects mptcp_inq_hint() in
net/mptcp/protocol.c before the 3rd ACK arrives (while msk->ack_seq is 0):
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;
}
Without the cast to u64, 0ULL - MPTCP_SKB_CB(skb)->map_seq underflows and
clamps to INT_MAX for SIOCINQ, TCP_CM_INQ, and mptcp_rcv_space_adjust(). It
is also needed by the subsequent commit 698eb6e05455 ("mptcp: remove CB
offset field"), which initializes msk->copied_seq from
MPTCP_SKB_CB(skb)->map_seq and relies on 64-bit two's complement wrapping
when adding msk->tfo_iasn.
Could the commit message be updated to describe the actual callers affected
by the unextended map_seq value?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=3
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen
2026-11-03 8:57 ` [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen Geliang Tang
2026-11-03 9:38 ` sashiko-bot
@ 2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-05 0:41 UTC (permalink / raw)
To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit message describes the wrong impact.
- [Medium] If user space partly reads the TFO data before the MPC 3rd
ACK, the read progress stored in map_seq is lost when the sequence is…
- [Low] The patch is described as a fix for a user-visible bug but has no
Fixes: tag.
--
mptcp: fix TFO map_seq sign extension in fastopen
This patch changes how mptcp_fastopen_subflow_synack_set_params() sets the
dummy map_seq of the TFO skb. It now uses -(u64)skb->len instead of
-skb->len, so the value is a proper 64-bit negative offset from an assumed
IASN of 0.
> mptcp: fix TFO map_seq sign extension in fastopen
>
> [ ... ]
>
> This causes incorrect behavior in sequence number comparisons throughout
> the code. Functions like after64() and before64() may misorder TFO skbs,
> and the out-of-order queue handling may misclassify TFO data.
[Severity: Medium]
Is this description of the impact accurate?
mptcp_fastopen_subflow_synack_set_params() puts the TFO skb straight onto
msk->sk_receive_queue. It never goes through __mptcp_move_skb() or
mptcp_data_queue_ofo(), so it is never in msk->out_of_order_queue.
The after64()/before64() users in protocol.c are mptcp_prune_ofo_queue(),
mptcp_data_queue_ofo(), __mptcp_move_skb() and __mptcp_ofo_queue(). Each
of them compares an incoming or out-of-order skb against msk->ack_seq or
against another out-of-order skb. None of them reads the TFO skb's
map_seq.
The only place another skb meets the TFO skb is __mptcp_try_coalesce().
That function compares from->map_seq with to->end_seq, not map_seq.
Also, __mptcp_move_skb() runs the resync first:
if (unlikely(msk->rcvd_dummy_seq))
__mptcp_sync_rcv_sequence(sk);
and the resync rewrites map_seq in u64 arithmetic before any comparison
happens.
The code that does read the dummy map_seq is mptcp_inq_hint():
u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
if (hint_val >= INT_MAX)
return INT_MAX;
On a passive TFO msk, msk->ack_seq is still 0 until the MPC 3rd ACK
arrives. With the old truncated value, hint_val was 0 - (2^32 - len).
SIOCINQ/FIONREAD through mptcp_ioctl(), the TCP_CM_INQ cmsg in
mptcp_recvmsg() and mptcp_rcv_space_adjust() therefore all saw INT_MAX.
With this patch they see skb->len.
Could the commit message describe this user-visible effect instead of the
ordering and out-of-order queue effects?
There is a related problem later in the series. "mptcp: sync mptcp skb cb
layout with tcp one" makes the CB map_seq a u32, but mptcp_inq_hint()
still computes the u64 ack_seq minus the u32 map_seq. The TFO skb then
gets map_seq = 2^32 - len again, and the INT_MAX result before the 3rd
ACK comes back.
Is the benefit of this patch lost by the end of the series?
> Fix by casting to u64 before negation: -(u64)skb->len. This ensures
> proper 64-bit two's complement representation and correct semantics for
> all sequence number operations.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
[Severity: Low]
Should this have a Fixes: tag? The truncating assignment
MPTCP_SKB_CB(skb)->map_seq = -skb->len;
was added by commit 36b122baf6a8 ("mptcp: add
subflow_v(4,6)_send_synack()").
In that design, __mptcp_fastopen_gen_msk_ackseq() added ack_seq to the
truncated value, which still left map_seq 2^32 too large. After commit
f03afb3aeb9d removed that helper, the dummy value was never corrected.
The new comment ("before receiving the remote key, IASN is assumed being
0") also relies on the resync added by the earlier "mptcp: drop the
cant_coalesce CB field" patch. Without that patch, this one-line change
only fixes the window before the 3rd ACK, because map_seq is never
rebased afterwards.
Could the commit message name this dependency, or could the change be
folded into the patch that introduces the resync?
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index e18b31028e7e3..79765cf55286b 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -45,8 +45,10 @@ 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;
[Severity: Medium]
What happens if user space reads only part of the TFO data before the 3rd
ACK arrives?
A short recv() in __mptcp_recvmsg_mskq() (and likewise in
__mptcp_read_sock()) advances both fields:
MPTCP_SKB_CB(skb)->offset += count;
MPTCP_SKB_CB(skb)->map_seq += count;
so map_seq always points at the first unread byte. mptcp_inq_hint()
relies on that.
When the 3rd ACK arrives, subflow_set_remote_key() calls
__mptcp_sync_rcv_sequence(), either directly or deferred through
MPTCP_SYNC_SEQ. That function does:
MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
skb->len has not changed, because consumption is tracked in offset. The
+count progress in map_seq is therefore lost.
After that, mptcp_inq_hint() returns ack_seq - map_seq = len, although
only len - offset bytes remain. That overstated value goes to
SIOCINQ/FIONREAD, the TCP_CM_INQ cmsg and mptcp_rcv_space_adjust() until
the TFO skb is fully drained.
This patch makes the value before the sync a correct relative offset.
Would a plain rebase in __mptcp_sync_rcv_sequence() (map_seq += tfo_iasn,
end_seq += tfo_iasn), or tfo_iasn - len + offset, keep the read progress?
By the end of the series, "mptcp: remove CB offset field" moves the
partial-read progress into msk->copied_seq. However, mptcp_inq_hint()
still computes ack_seq - map_seq and never reads copied_seq, so the
overstatement seems to remain.
After "mptcp: sync mptcp skb cb layout with tcp one", the value also
becomes INT_MAX whenever ack_seq >= 2^32.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793695518.git.tanggeliang%40kylinos.cn
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (2 preceding siblings ...)
2026-11-03 8:57 ` [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 10:21 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
2026-11-03 8:57 ` [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
` (2 subsequent siblings)
6 siblings, 2 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 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.
In mptcp_init_skb(), map_seq is computed as mptcp_subflow_get_mapped_dsn()
minus offset. This is correct even when offset > 0 and the mapping is
out-of-order, because mptcp_subflow_get_mapped_dsn() returns the DSN for
the byte at tp->copied_seq (position 'offset' in the skb), and the
consumed prefix bytes are part of the same logical DSN range.
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 | 2 +-
net/mptcp/protocol.c | 93 +++++++++++++++++++++++---------------------
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 15 +++++--
4 files changed, 62 insertions(+), 50 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 79765cf55286..78481c100e2a 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -50,7 +50,6 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
*/
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);
@@ -58,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 f3b07b303f55..3bd7d38ce78c 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;
@@ -424,10 +422,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
+ *
+ * This is correct even when offset > 0 and mapping is out-of-order,
+ * because consumed prefix bytes are part of the same logical DSN range.
*/
- 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 +452,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 +495,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 +852,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 +915,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 +2201,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 +2237,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 {
@@ -2397,9 +2380,15 @@ static bool mptcp_move_skbs(struct sock *sk)
u32 moved = 0;
mptcp_data_lock(sk);
- if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
- __mptcp_sync_rcv_sequence(sk);
- while (mptcp_can_spool_backlog(sk, &skbs)) {
+ while (1) {
+ if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
+ msk->copied_seq += msk->tfo_iasn;
+ __mptcp_sync_rcv_sequence(sk);
+ }
+
+ if (!mptcp_can_spool_backlog(sk, &skbs))
+ break;
+
mptcp_data_unlock(sk);
enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);
@@ -2441,6 +2430,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;
@@ -2459,6 +2449,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;
@@ -2468,7 +2463,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)
@@ -2478,8 +2473,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()
@@ -2522,6 +2520,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);
@@ -3698,6 +3700,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 +3911,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 +4635,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 +4677,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] 18+ messages in thread* Re: [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field
2026-11-03 8:57 ` [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field Geliang Tang
@ 2026-11-03 10:21 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-11-03 10:21 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: stale map_seq subtraction in mptcp_inq_hint() after removing CB offset
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero subflow offset
--
commit 698eb6e054556fc8ba45c463c10e26a894f29975
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: remove CB offset field
This commit removes the offset field from struct mptcp_skb_cb and introduces
copied_seq in struct mptcp_sock to track consumed bytes across receive skbs.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f3b07b303f555..3bd7d38ce78c4 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;
> @@ -424,10 +422,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
> + *
> + * This is correct even when offset > 0 and mapping is out-of-order,
> + * because consumed prefix bytes are part of the same logical DSN range.
> */
> - 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;
[Severity: High]
As reported by Sashiko on v19 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() corrupt the receive
stream when a subflow skb has a non-zero offset at the start of a new DSS
mapping?
In __mptcp_move_skbs_from_subflow(), the head skb on ssk->sk_receive_queue can
have offset = tp->copied_seq - TCP_SKB_CB(skb)->seq > 0 while starting a new
DSS mapping M2 at tp->copied_seq.
For example, in subflow_check_data_avail() -> get_mapping_status(), a single
TCP skb can carry the tail of an earlier mapping M1 in skb->data[0..offset-1]
and a new mapping M2 in its SKB_EXT_MPTCP extension starting at offset:
net/mptcp/subflow.c:get_mapping_status() {
...
if (skb_is_fully_mapped(ssk, skb)) {
MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSNOMATCH);
return MAPPING_INVALID;
}
/* will validate the next map after consuming the current one */
goto validate_csum;
...
}
When the remaining bytes of M1 have already been acknowledged at the MPTCP
level, mptcp_subflow_discard_data() advances tp->copied_seq by offset without
eating the skb and resets subflow->map_valid = 0. On the next loop iteration
in subflow_check_data_avail(), get_mapping_status() installs M2 starting at
tp->copied_seq (offset bytes into the skb). The same situation also occurs
when tcp_data_queue() or tcp_ofo_queue() queues a partially overlapping TCP
segment whose first offset bytes overlap the end of M1 and whose remaining
bytes start M2 at tp->copied_seq.
Because MPTCP stripes data across multiple subflows, M1 and M2 are contiguous
only in subflow sequence space, not in data sequence space when intervening
DSNs [mapped_dsn - offset, mapped_dsn) were sent on another subflow. Setting
MPTCP_SKB_CB(skb)->map_seq = mapped_dsn - offset assigns the DSN range
[mapped_dsn - offset, mapped_dsn) to the stale M1 prefix bytes in
skb->data[0..offset-1]:
1. 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. It treats the skb as
in-sequence (or partial in-sequence), advances msk->ack_seq across the
unreceived gap [msk->ack_seq, mapped_dsn), delivers the stale M1 prefix
bytes from skb->data[0..offset-1] to user space via __mptcp_recvmsg_mskq()
and mptcp_recv_skb(), and drops the genuine data arriving on the other
subflow as MPTCP_MIB_DUPDATA.
2. Because __mptcp_try_coalesce() no longer checks MPTCP_SKB_CB(from)->offset,
whenever mapped_dsn - offset equals MPTCP_SKB_CB(to)->end_seq in
__mptcp_add_backlog(), __mptcp_move_skb(), mptcp_data_queue_ofo(), or
__mptcp_ofo_queue(), skb_try_coalesce() appends all from->len bytes
starting at byte 0 (including the stale M1 prefix in from->data[0..offset-1])
onto to.
3. When mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
skb into msk->out_of_order_queue under the falsified start DSN
mapped_dsn - offset, where it can coalesce with or replace legitimate
out-of-order segments covering [mapped_dsn - offset, mapped_dsn) and be
dequeued prematurely by __mptcp_ofo_queue().
[ ... ]
> @@ -2252,14 +2237,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 {
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8a3ba490b5772..71c3b148cf04a 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);
> + }
> }
[Severity: High]
As reported by Sashiko on v15 [2], does mptcp_inq_hint() still compute the
unread byte count using MPTCP_SKB_CB(skb)->map_seq instead of msk->copied_seq?
The commit message notes that this change fixes and simplifies
mptcp_inq_hint() via the new ack_seq - copied_seq hint, but mptcp_inq_hint()
in net/mptcp/protocol.c still reads:
net/mptcp/protocol.c:mptcp_inq_hint() {
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;
if (hint_val >= INT_MAX)
return INT_MAX;
return (unsigned int)hint_val;
}
...
}
Because __mptcp_recvmsg_mskq(), __mptcp_read_sock(), __mptcp_move_skb(), and
__mptcp_ofo_queue() no longer advance MPTCP_SKB_CB(skb)->map_seq as bytes are
consumed or overlapped, and mptcp_init_skb() subtracts offset from map_seq,
MPTCP_SKB_CB(skb)->map_seq stays at the start of the head skb while
msk->copied_seq advances. Whenever the head skb in sk->sk_receive_queue is
partially read (count < data_len) or has a non-zero offset, mptcp_inq_hint()
over-counts unread bytes by msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq for
SIOCINQ in mptcp_ioctl(), TCP_CM_INQ in mptcp_recvmsg(), and receive buffer
auto-tuning in mptcp_rcv_space_adjust().
In addition, on a passive TFO socket when the peer's 3rd ACK arrives while
sock_owned_by_user(sk) is true, subflow_set_remote_key() updates msk->ack_seq
to subflow->iasn immediately while deferring msk->copied_seq += msk->tfo_iasn
and __mptcp_sync_rcv_sequence(sk) via MPTCP_SYNC_SEQ. If mptcp_inq_hint() runs
before MPTCP_SYNC_SEQ is processed (for example in __mptcp_recvmsg_mskq() ->
mptcp_rcv_space_adjust(), at out_err in mptcp_recvmsg() when msk->backlog_list
is empty, in __mptcp_read_sock() -> mptcp_rcv_space_adjust(), or in
mptcp_ioctl() if the 3rd ACK arrives after mptcp_move_skbs()), it subtracts
the pre-IASN sequence from the post-IASN msk->ack_seq and returns INT_MAX.
Furthermore, a later patch in this series (commit 0495d49fc957, "mptcp: sync
mptcp skb cb layout with tcp one") narrows MPTCP_SKB_CB(skb)->map_seq from u64
to u32, which will cause READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq
to return INT_MAX whenever the upper 32 bits of msk->ack_seq are non-zero.
Should mptcp_inq_hint() use msk->copied_seq (while accounting for any pending
MPTCP_SYNC_SEQ adjustment) instead of MPTCP_SKB_CB(skb)->map_seq?
[1] https://lore.kernel.org/r/1dac839fffd26114e35382b256fdee9923c0949b.1793361629.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=4
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field
2026-11-03 8:57 ` [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field Geliang Tang
2026-11-03 10:21 ` sashiko-bot
@ 2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-05 0:41 UTC (permalink / raw)
To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 2 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] mptcp_inq_hint() (net/mptcp/protocol.c:2406-2425) was not
updated and still returns `READ_ONCE(msk->ack_seq) -…
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero subflow
offset
- [Medium] The commit message credits this patch with fixing the TFO
"SIOCINQ and TCP_CM_INQ returned INT_MAX before the MPC ACK" behaviour…
- [Medium] subflow_check_data_avail() (net/mptcp/subflow.c:1505) changes
the fallback dummy mapping anchor from `map_subflow_seq =…
- [Low] The new fallback else-branch in mptcp_propagate_state()
(net/mptcp/subflow.c:562-569) does `WRITE_ONCE(msk->ack_seq, 0)` and a…
--
mptcp: remove CB offset field
This patch removes the offset field from struct mptcp_skb_cb. Read
progress is now tracked in a new msk->copied_seq field, as TCP does, and
readers compute the in-skb offset as copied_seq - map_seq. The patch also
shifts copied_seq on IASN resync and sets the fallback sequence space to 0
in mptcp_propagate_state().
> 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.
[Severity: Medium]
Is this description accurate for this commit?
The -(u64)skb->len assignment in
mptcp_fastopen_subflow_synack_set_params() is only unchanged context in
the fastopen.c hunk of this diff. It appears to come from the parent
commit "mptcp: fix TFO map_seq sign extension in fastopen".
This patch doesn't touch mptcp_inq_hint() either, so there is no
ack_seq - copied_seq hint:
u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
With the sign fix already applied, the hint before the MPC ACK is
ack_seq (0) - (-(u64)len) = len. Backporting only this commit for the
INT_MAX problem would not fix it.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f3b07b303f555..3bd7d38ce78c4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2252,14 +2237,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;
> - }
[Severity: High]
Without the map_seq += count adjustment, does mptcp_inq_hint() still
return the right value after a partial read? It wasn't updated and still
computes:
net/mptcp/protocol.c:mptcp_inq_hint() {
...
skb = skb_peek(&sk->sk_receive_queue);
if (skb) {
u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
...
}
That formula only worked because every partial consumer moved the head
skb's map_seq up to the first unread byte. This patch removes all of
those adjustments:
- here in __mptcp_recvmsg_mskq()
- in __mptcp_read_sock()
- in the partial packet path of __mptcp_move_skb()
- in the uncoalesced path of __mptcp_ofo_queue()
map_seq now always refers to byte 0 of the skb.
For example, say the head skb covers DSN [X, X+1000) and ack_seq is
X+1000. After recvmsg() reads 100 bytes, copied_seq is X+100, but the
hint still returns 1000 instead of 900.
A peer can also inflate the hint by sending data whose DSN overlaps data
already received, because __mptcp_move_skb() now queues skbs with
map_seq < ack_seq.
The inflated value reaches user space through ioctl(SIOCINQ) in
mptcp_ioctl() and through the TCP_CM_INQ cmsg in mptcp_recvmsg(). It also
feeds "copied -= mptcp_inq_hint(sk)" in mptcp_rcv_space_adjust(), which
skews rcvbuf autotuning.
The selftest tools/testing/selftests/net/mptcp/mptcp_inq.c checks this
case in process_one_client() after a 1 byte read:
assert((size_t)tcp_inq == (expect_len - 1));
Would this assertion, and the later tcp_inq <= expect_len - tot check,
now fail?
The commit message says the patch simplifies mptcp_inq_hint() and adds
an ack_seq - copied_seq hint, but that change isn't anywhere in the
series. Later in the series, "mptcp: sync mptcp skb cb layout with tcp
one" makes map_seq a u32. From then on the u64 subtraction returns
INT_MAX whenever ack_seq >= 2^32. That is almost always the case, since
the IASN is derived from a hash.
Should mptcp_inq_hint() use READ_ONCE(msk->ack_seq) - msk->copied_seq
instead?
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8a3ba490b5772..71c3b148cf04a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -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;
> }
[Severity: Low]
The commit message says:
The writes are safe without sock_owned_by_user() deferral because
mptcp_propagate_state() already holds mptcp_data_lock().
Does the data lock actually protect copied_seq? The same message says
copied_seq is "always accessed under the msk socket lock".
The socket owner reads and updates it without the spinlock:
- mptcp_recvmsg() seeds peek_seq from it
- __mptcp_recvmsg_mskq() does *seq += count
- __mptcp_read_sock() does msk->copied_seq += count
This branch is reached from subflow_finish_connect() in softirq context
with only the data lock held. Unlike the MPTCP_SYNC_STATE handling just
below it, there is no sock_owned_by_user() check.
On the only caller path today, ack_seq and copied_seq are both already 0.
They are zeroed at allocation and reset in mptcp_disconnect(), so these
stores look redundant rather than harmful.
Are these writes needed at all? If they are, should they use the same
deferral as the other owner-protected state?
[ ... ]
> @@ -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;
[Severity: Medium]
The commit message doesn't mention this change, and there's no Fixes:
tag. Should it be described, or split into its own patch?
Commit 2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation")
anchored subflow->map_seq at the DSN of skb->seq
(iasn + skb->seq - ssn_offset - 1). It left map_subflow_seq anchored at
copied_seq. As a result, in fallback mode
mptcp_subflow_get_mapped_dsn() returned DSN(skb->seq) where
DSN(copied_seq) was expected.
TCP can queue a partially overlapping segment with seq < rcv_nxt. The
head subflow skb can then start before copied_seq, giving offset > 0 in
__mptcp_move_skbs_from_subflow().
Before this patch, mptcp_init_skb() then gave the data at offset a DSN
that was offset bytes too low. __mptcp_move_skb() would then skip another
offset bytes, or drop the skb as old data. That leaves a hole in the msk
stream: later fallback data waits in the OFO queue and the connection
stalls.
The new mapped_dsn - offset formula in mptcp_init_skb() only works
because of this anchor change. So this hunk is both a prerequisite for
the patch and a fix that stable kernels carrying 2834f8edd74d would want.
Also, the new fallback branch in mptcp_propagate_state() above still sets
map_subflow_seq with the old copied_seq based formula. That only works
because subflow_check_data_avail() overwrites it before use.
[ ... ]
> @@ -424,10 +422,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
> + *
> + * This is correct even when offset > 0 and mapping is out-of-order,
> + * because consumed prefix bytes are part of the same logical DSN range.
> */
> - 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);
[Severity: High]
Does the claim in the new comment hold when the head skb straddles a
DSS mapping boundary at tp->copied_seq?
mptcp_init_skb() now gives the already consumed prefix
skb->data[0 .. offset - 1] the DSN range
[mapped_dsn - offset, mapped_dsn), and end_seq covers the whole
skb->len. That is only right when the mapping installed at
tp->copied_seq also covers the start of the skb, as in fallback.
With several subflows, consecutive mappings on one subflow are
contiguous in subflow sequence space but not in DSN space. The DSNs
in between may have been sent on another subflow.
There seem to be at least two ways to get offset > 0 with a new
mapping M2 starting at tp->copied_seq:
- get_mapping_status() keeps the MPTCP extension on an skb that is
not fully covered by the current mapping ("will validate the next
map after consuming the current one"). Suppose the tail of the old
mapping M1 inside that skb was already acked at msk level.
subflow_check_data_avail() then calls mptcp_subflow_discard_data().
That advances tp->copied_seq by offset without eating the skb and
clears map_valid. The next loop iteration installs M2 at
tp->copied_seq, offset bytes into the skb.
- After M1 was fully consumed, tcp_data_queue() can queue a
partially overlapping segment with seq < rcv_nxt. Its prefix
overlaps the end of M1 and its remainder carries M2.
In both cases the prefix belongs to M1, not to
[mapped_dsn - offset, mapped_dsn).
Now suppose M2 is out of order at msk level: mapped_dsn > ack_seq
because [ack_seq, mapped_dsn) is still in flight on another subflow,
and mapped_dsn - offset <= ack_seq.
__mptcp_move_skb() then takes the in-sequence or partial packet path
and advances msk->ack_seq across the hole. __mptcp_recvmsg_mskq() and
mptcp_recv_skb() hand the stale M1 bytes to user space as if they
were those DSNs. When the real data for that range arrives on the
other subflow, it is dropped as MPTCP_MIB_DUPDATA. A peer can trigger
this silent stream corruption.
Before this patch the prefix never got a DSN. map_seq was mapped_dsn,
end_seq was map_seq + copy_len, and the in-skb offset was kept in the
cb.
Removing the offset check in __mptcp_try_coalesce() makes this worse.
When mapped_dsn - offset equals to->end_seq, skb_try_coalesce()
appends the whole from skb, prefix included. That can happen in
__mptcp_add_backlog(), __mptcp_move_skb(), mptcp_data_queue_ofo() and
__mptcp_ofo_queue().
In mptcp_data_queue_ofo() the falsified start DSN can also replace,
or merge with, legitimate out of order segments covering
[mapped_dsn - offset, mapped_dsn).
Should mptcp_init_skb() use mapped_dsn - offset only when the current
mapping covers TCP_SKB_CB(skb)->seq (fallback, or map_subflow_seq at
or before the skb start)? Otherwise it could trim the prefix before
the skb reaches the msk, for example with pskb_pull(). Or is there
something that rules out a mapping boundary inside the head skb that
I am missing?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793695518.git.tanggeliang%40kylinos.cn
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (3 preceding siblings ...)
2026-11-03 8:57 ` [PATCH mptcp-next v20 4/6] mptcp: remove CB offset field Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 10:43 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
2026-11-03 8:57 ` [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
2026-11-03 10:22 ` [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
6 siblings, 2 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 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, 68 insertions(+), 39 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 3bd7d38ce78c..3e10b06b7772 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);
@@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
*
* This is correct even when offset > 0 and mapping is out-of-order,
* because consumed prefix bytes are part of the same logical DSN range.
+ *
+ * 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);
@@ -451,13 +464,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;
@@ -469,7 +483,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)) {
@@ -487,14 +501,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
return true;
- } else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
+ } else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
mptcp_data_queue_ofo(msk, skb);
return false;
}
/* Partial packet */
- if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
- copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
+ if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
+ copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
goto insert;
}
@@ -889,39 +903,37 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
{
struct sock *sk = (struct sock *)msk;
struct sk_buff *skb, *tail;
+ u32 seq_delta, ack_seq;
bool moved = false;
struct rb_node *p;
- u64 end_seq;
p = rb_first(&msk->out_of_order_queue);
pr_debug("msk=%p empty=%d\n", msk, RB_EMPTY_ROOT(&msk->out_of_order_queue));
while (p) {
+ ack_seq = msk->ack_seq;
skb = rb_to_skb(p);
- if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq))
+ if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
break;
p = rb_next(p);
rb_erase(&skb->rbnode, &msk->out_of_order_queue);
- if (unlikely(!after64(MPTCP_SKB_CB(skb)->end_seq,
- msk->ack_seq))) {
+ if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
mptcp_drop(sk, skb);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
continue;
}
- end_seq = MPTCP_SKB_CB(skb)->end_seq;
+ seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;
tail = skb_peek_tail(&sk->sk_receive_queue);
if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
- 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;
@@ -2210,7 +2222,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
int copied = 0;
skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
- u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
+ u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;
u32 data_len = skb->len - offset;
u32 count;
int err;
@@ -4635,7 +4647,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;
@@ -4885,11 +4897,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] 18+ messages in thread* Re: [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one
2026-11-03 8:57 ` [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
@ 2026-11-03 10:43 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-11-03 10:43 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 in mptcp_init_skb() corrupts out-of-order data
- [High] mptcp: 64-bit ack_seq minus 32-bit map_seq overflow in mptcp_inq_hint()
--
commit 0495d49fc9571bbbbcc61fc3640dabeb8f98b52b
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: sync mptcp skb cb layout with tcp one
This commit aligns struct mptcp_skb_cb with struct tcp_skb_cb by narrowing
map_seq and end_seq to 32-bit sequence numbers and adding a 64-bit
map_seq64 field for receive window admission and coalescing checks.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3bd7d38ce78c4..3e10b06b7772d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
> *
> * This is correct even when offset > 0 and mapping is out-of-order,
> * because consumed prefix bytes are part of the same logical DSN range.
> + *
> + * 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 v19 [1], when offset > 0 in mptcp_init_skb() and
the active DSS mapping is out of order at the MPTCP level
(mptcp_subflow_get_mapped_dsn(subflow) > msk->ack_seq), can subtracting
offset from map_seq64 and map_seq without stripping the [0, offset) prefix
from skb corrupt the MPTCP receive stream?
On a subflow where data is multiplexed across multiple subflows,
consecutive DSS mappings do not have contiguous DSNs. If the leading
[0, offset) bytes of the head skb on ssk->sk_receive_queue belonged to a
previous DSS mapping (for example after mptcp_subflow_discard_data() in
subflow_check_data_avail() discards an already-received mapping and
advances to the next mapping in the same skb) or an overlapping TCP
retransmission, those prefix bytes are not part of the active mapping
starting at tp->copied_seq and are not covered by its DSS checksum in
validate_data_csum().
Setting map_seq64 = mapped_dsn - offset and end_seq = map_seq + skb->len
assigns the fabricated DSN range [mapped_dsn - offset, mapped_dsn) to the
unpulled [0, offset) prefix bytes:
- If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
treats the skb as covering msk->ack_seq, queues or coalesces it into
sk->sk_receive_queue, and advances msk->ack_seq across the unreceived
gap. __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 stale [0, offset) subflow bytes to user space.
- If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
skb into msk->out_of_order_queue starting at mapped_dsn - offset, where
__mptcp_try_coalesce() can coalesce the stale prefix into a preceding OFO
skb, real segments arriving for [mapped_dsn - offset, mapped_dsn) are
dropped as duplicate data, and __mptcp_ofo_queue() dequeues the skb once
msk->ack_seq reaches mapped_dsn - offset.
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7878e6ac6c0b6..fa54f70ab501d 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;
> };
[Severity: High]
Now that MPTCP_SKB_CB(skb)->map_seq is a u32, does mptcp_inq_hint()
overflow when subtracting it from the 64-bit msk->ack_seq?
In net/mptcp/protocol.c:mptcp_inq_hint():
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;
}
Because READ_ONCE(msk->ack_seq) is a u64 and MPTCP_SKB_CB(skb)->map_seq is
zero-extended from u32 to u64, the upper 32 bits of msk->ack_seq remain in
hint_val (or the subtraction underflows across a 32-bit wrap), causing
hint_val >= INT_MAX to evaluate to true whenever sk->sk_receive_queue is
non-empty and msk->ack_seq has non-zero upper 32 bits.
In addition to returning INT_MAX for SIOCINQ in mptcp_ioctl() and
TCP_CM_INQ in mptcp_recvmsg(), mptcp_rcv_space_adjust() does:
copied = msk->rcvq_space.copied;
copied -= mptcp_inq_hint(sk);
if (copied <= msk->rcvq_space.space)
goto new_measure;
When mptcp_inq_hint() returns INT_MAX, copied becomes negative and jumps to
new_measure, preventing mptcp_rcvbuf_grow() from auto-tuning the MPTCP and
subflow receive buffers. Should mptcp_inq_hint() use 32-bit sequence
arithmetic (or subtract msk->copied_seq from msk->ack_seq) instead?
[1] https://lore.kernel.org/r/1c928472a89719130041c92cadd7e0fb6dbf552a.1793361629.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=5
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one
2026-11-03 8:57 ` [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
2026-11-03 10:43 ` sashiko-bot
@ 2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-05 0:41 UTC (permalink / raw)
To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 1 · High: 1 · Medium: 0 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Critical] mptcp: subtracting offset in mptcp_init_skb() corrupts out-
of-order data
- [High] mptcp_inq_hint() in net/mptcp/protocol.c was not converted to
the new CB layout.
- [Low] The commit message is inaccurate and leaves out motivation and
some behaviour changes.
--
mptcp: sync mptcp skb cb layout with tcp one
This patch narrows the MPTCP skb control block sequence numbers to 32 bits
and keeps the full 64-bit DSN in a new map_seq64 field. map_seq, end_seq and
flags are placed so they line up with the matching tcp_skb_cb fields. The
receive path sequence arithmetic is switched to the 32-bit before()/after()
helpers, and a build time layout check is added in mptcp_proto_init().
> 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.
[ ... ]
> Add build time check to ensure the latter invariant.
[Severity: Low]
Is the layout description accurate for has_rxtstamp?
In the new struct mptcp_skb_cb, has_rxtstamp is a full u8 at offset 14. That
overlaps tcp_skb_cb's sacked. In tcp_skb_cb, has_rxtstamp is a bitfield in
the byte at offset 16, and here that byte is the low byte of map_seq64.
The build time check added in mptcp_proto_init() only compares field offsets
and sizes:
CHK_CB_FIELD(map_seq, seq);
CHK_CB_FIELD(end_seq, end_seq);
CHK_CB_FIELD(flags, tcp_flags);
It checks nothing about sequence numbers staying within 2^31 of ack_seq or
copied_seq, which "the latter invariant" seems to refer to. Could the
wording be adjusted?
Nothing in this patch or the rest of the series seems to use the shared
layout. flags is only ever set to 0. The final patch in the series ("mptcp:
defer read_sock cleanup to mptcp_worker") adds no TCP_SKB_CB() user for
msk-level skbs. Could the commit message say what the layout change is for?
The patch also changes some behaviour that the message does not mention.
mptcp_data_queue_ofo() now uses max(rcv_wnd_sent, ack_seq) as the admission
bound and adds an after64(map_seq64, max_seq) check.
__mptcp_try_coalesce() gains a map_seq64 contiguity check. Should these be
described as well?
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7878e6ac6c0b6..fa54f70ab501d 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;
> };
[Severity: High]
Now that map_seq is a u32, was mptcp_inq_hint() in net/mptcp/protocol.c
missed in the conversion? It still does:
net/mptcp/protocol.c:mptcp_inq_hint() {
...
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;
}
...
}
The u32 map_seq is zero-extended, so hint_val keeps the upper 32 bits of the
64-bit msk->ack_seq. subflow_set_remote_key() initializes msk->ack_seq from
the IASN:
mptcp_crypto_key_sha(subflow->remote_key, NULL, &subflow->iasn);
subflow->iasn++;
...
WRITE_ONCE(msk->ack_seq, subflow->iasn);
The IASN is derived from the peer's random key, so its upper 32 bits are
non-zero on nearly every connection. When ack_seq is below 2^32 and the low
32 bits have wrapped, the u64 subtraction underflows instead.
In both cases, does mptcp_inq_hint() return INT_MAX whenever the receive
queue is not empty?
That value reaches userspace through SIOCINQ in mptcp_ioctl():
*karg = mptcp_inq_hint(sk);
It also reaches userspace through the TCP_CM_INQ cmsg in mptcp_recvmsg().
mptcp_rcv_space_adjust() is affected too:
copied = msk->rcvq_space.copied;
copied -= mptcp_inq_hint(sk);
if (copied <= msk->rcvq_space.space)
goto new_measure;
Subtracting INT_MAX makes the int copied negative. Would msk-level rcvbuf
autotuning, and the subflow tcp_rcvbuf_grow() calls after it, then be
skipped whenever data is still queued at measurement time?
The other map_seq readers were converted. __mptcp_recvmsg_mskq(),
mptcp_recv_skb() and __mptcp_ofo_queue() use a (u32) cast, and
__mptcp_move_skb() switched to map_seq64. This one is still unchanged at the
end of the series, including after "mptcp: defer read_sock cleanup to
mptcp_worker".
Would something like this work, possibly with a clamp, or should it use
map_seq64 instead?
u32 hint_val = (u32)READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
[ ... ]
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
> *
> * This is correct even when offset > 0 and mapping is out-of-order,
> * because consumed prefix bytes are part of the same logical DSN range.
> + *
> + * 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);
[Severity: Critical]
The offset subtraction is not new in this patch. This hunk rewrites it,
though, and the new 32-bit end_seq is derived from it, so could the
assumption in the comment above be double checked?
mptcp_init_skb() gives the first offset bytes of the skb the DSN range
[mapped_dsn - offset, mapped_dsn). end_seq is computed from the full
skb->len, and those bytes are never pulled. That is only correct if the
bytes before tp->copied_seq in this skb belong to the same DSS mapping
as the bytes after it.
Is that guaranteed? In subflow_check_data_avail(),
mptcp_subflow_discard_data() can drop the tail of a previous mapping M1
and clear map_valid. The loop then goes back to get_mapping_status()
and can activate the next mapping M2 on the remaining bytes of the same
skb. A partially overlapping segment queued by tcp_data_queue(), with
seq < copied_seq < end_seq, looks like it can lead to the same state.
When data is striped over several subflows, consecutive mappings on one
subflow are generally not DSN-contiguous. The prefix bytes would then
get a DSN range they never had. validate_mapping() and
validate_data_csum() only cover data from tp->copied_seq onwards, so
the prefix is not protected by the M2 DSS checksum either.
If M2 is out of order at the msk level, there seem to be two outcomes.
When mapped_dsn - offset is not after msk->ack_seq, __mptcp_move_skb()
treats the skb as in sequence or partial and moves msk->ack_seq across
a gap that was never received. __mptcp_recvmsg_mskq() or
mptcp_recv_skb() then copies the stale prefix bytes to userspace for
that range.
Otherwise mptcp_data_queue_ofo() puts the skb in the OoO queue starting
at mapped_dsn - offset. __mptcp_try_coalesce() can append it to a
preceding skb that ends there. The real data for that range, arriving
on another subflow, is then dropped as MPTCP_MIB_DUPDATA.
__mptcp_ofo_queue() releases the skb once ack_seq reaches
mapped_dsn - offset.
Wouldn't either case silently corrupt the byte stream the application
sees, in a way a remote peer could steer?
Should the prefix be pulled, or the offset tracked separately, so that
map_seq64 always starts at mapped_dsn? Or is there a guarantee I am
missing that the head skb can never span two mappings at this point?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793695518.git.tanggeliang%40kylinos.cn
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (4 preceding siblings ...)
2026-11-03 8:57 ` [PATCH mptcp-next v20 5/6] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
@ 2026-11-03 8:57 ` Geliang Tang
2026-11-03 11:08 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
2026-11-03 10:22 ` [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
6 siblings, 2 replies; 18+ messages in thread
From: Geliang Tang @ 2026-11-03 8:57 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang, Paolo Abeni
From: Geliang Tang <tanggeliang@kylinos.cn>
When MPTCP carries TLS, __mptcp_read_sock() may be 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.
Conditionally defer cleanup to mptcp_worker(): when the caller doesn't
own the socket lock (BH context), accumulate consumed bytes in
msk->read_copied and signal the worker through MPTCP_WORK_READ_COMPLETE.
When the caller owns the lock (process context), perform inline cleanup
to avoid delaying window updates and autotuning.
Replace msk_owned_by_me() assertion with DEBUG_NET_WARN_ON_ONCE using
lockdep_sock_is_held() to verify proper locking for both process and
BH contexts.
Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()) 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 | 41 +++++++++++++++++++++++++++++++++--------
net/mptcp/protocol.h | 2 ++
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3e10b06b7772..25f0d10373d2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3156,6 +3156,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;
@@ -3231,6 +3246,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);
@@ -3274,6 +3292,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);
@@ -3721,6 +3740,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
msk->fastclosing = 0;
msk->tfo_iasn = 0;
msk->rcvd_dummy_seq = false;
+ msk->read_copied = 0;
mptcp_init_rtt_est(msk);
/* for fallback's sake */
@@ -4643,9 +4663,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) {
@@ -4660,6 +4677,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)
@@ -4669,7 +4687,7 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
int copied = 0;
u32 offset;
- msk_owned_by_me(msk);
+ DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk));
if (sk->sk_state == TCP_LISTEN)
return -ENOTCONN;
@@ -4701,11 +4719,18 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
if (noack)
goto out;
- mptcp_rcv_space_adjust(msk, copied);
-
if (copied > 0) {
- mptcp_recv_skb(sk, &offset);
- mptcp_cleanup_rbuf(msk, copied);
+ if (sock_owned_by_user(sk)) {
+ /* Process context: inline cleanup */
+ mptcp_rcv_space_adjust(msk, copied);
+ mptcp_recv_skb(sk, &offset);
+ mptcp_cleanup_rbuf(msk, copied);
+ } else {
+ /* BH context: defer to worker */
+ 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] 18+ messages in thread* Re: [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker
2026-11-03 8:57 ` [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
@ 2026-11-03 11:08 ` sashiko-bot
2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-11-03 11:08 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mptcp: broken mptcp_inq_hint() 32-bit map_seq subtraction and MSG_PEEK sync
- [High] mptcp: data corruption in mptcp_init_skb() when subtracting non-zero offset
- [Medium] mptcp: link failure in __mptcp_read_sock() when CONFIG_LOCKDEP=n
- [Low] mptcp: redundant mptcp_recv_skb() call retained in __mptcp_read_sock()
--
commit db7d508291f7183730bd0d3c5939151c2b4d01c3
Author: Geliang Tang <geliang@kernel.org>
mptcp: defer read_sock cleanup to mptcp_worker
This commit defers mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() from
__mptcp_read_sock() to mptcp_worker() via MPTCP_WORK_READ_COMPLETE when called
in BH context, while keeping cleanup inline when the user owns the socket
lock. It also updates the socket lock check in __mptcp_read_sock() and removes
the backlog flush from mptcp_recv_skb().
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3e10b06b7772d..25f0d10373d22 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3156,6 +3156,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);
[Severity: High]
As reported by Sashiko on v19 [1], does mptcp_rcv_space_adjust() ->
mptcp_inq_hint() break receive buffer autotuning here (as well as SIOCINQ and
TCP_CM_INQ) due to subtracting the 32-bit MPTCP_SKB_CB(skb)->map_seq from the
64-bit msk->ack_seq?
In commit 698eb6e05455 ("mptcp: remove CB offset field"),
MPTCP_SKB_CB(skb)->map_seq stopped being advanced on partial reads, which
instead advance msk->copied_seq, and commit 0495d49fc957 ("mptcp: sync mptcp
skb cb layout with tcp one") then narrowed MPTCP_SKB_CB(skb)->map_seq from u64
to u32:
net/mptcp/protocol.h:
struct mptcp_skb_cb {
u32 map_seq;
u32 end_seq;
u32 unused;
u16 flags;
u8 has_rxtstamp;
u64 map_seq64;
};
However, mptcp_inq_hint() still subtracts MPTCP_SKB_CB(skb)->map_seq from
msk->ack_seq:
net/mptcp/protocol.c:mptcp_inq_hint() {
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;
}
}
Because MPTCP_SKB_CB(skb)->map_seq is zero-extended from u32 to u64 while
msk->ack_seq is a 64-bit sequence number whose upper 32 bits are non-zero on
nearly all MPTCP connections (or 0 while a passive TFO packet has map_seq =
(u32)-skb->len), hint_val retains non-zero upper 32 bits and returns INT_MAX
whenever any skb is present in sk->sk_receive_queue. Even when the upper 32
bits of msk->ack_seq happen to be zero, MPTCP_SKB_CB(skb)->map_seq does not
account for bytes already consumed via msk->copied_seq.
In mptcp_rcv_space_adjust(), subtracting INT_MAX causes copied to become
negative and skip mptcp_rcvbuf_grow():
net/mptcp/protocol.c:mptcp_rcv_space_adjust() {
copied = msk->rcvq_space.copied;
copied -= mptcp_inq_hint(sk);
if (copied <= msk->rcvq_space.space)
goto new_measure;
}
Additionally, in mptcp_recvmsg(), peek_seq is only updated when
mptcp_move_skbs(sk) returns true:
net/mptcp/protocol.c:mptcp_recvmsg() {
if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk)) {
if (flags & MSG_PEEK)
peek_seq = msk->copied_seq + copied;
continue;
}
}
If the third ACK arrives with an out-of-order or duplicate segment while
lock_sock(sk) is held during MSG_PEEK, mptcp_move_skbs(sk) processes
MPTCP_SYNC_SEQ (adding msk->tfo_iasn to msk->copied_seq and updating the TFO
skb map_seq) but returns false, leaving peek_seq unsynchronized in the
pre-IASN sequence space. Should mptcp_inq_hint() subtract msk->copied_seq from
READ_ONCE(msk->ack_seq), and should mptcp_recvmsg() also resynchronize
peek_seq when MPTCP_SYNC_SEQ runs?
[ ... ]
> @@ -4643,9 +4663,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;
[Severity: High]
As reported by Sashiko on v19 [2], can subtracting a non-zero subflow offset
in mptcp_init_skb() corrupt data delivery when an out-of-order subflow skb has
a discarded prefix?
In subflow_check_data_avail(), when a TCP skb on ssk->sk_receive_queue
contains the tail of an already-acknowledged DSS mapping in bytes 0..offset
and the start of a new DSS mapping at offset (which get_mapping_status()
allows when !skb_is_fully_mapped(ssk, skb)), mptcp_subflow_discard_data()
advances tp->copied_seq by offset, clears subflow->map_valid, and loops to
activate the new DSS mapping on the same skb:
net/mptcp/subflow.c:subflow_check_data_avail() {
if (unlikely(before64(ack_seq, old_ack))) {
mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq);
continue;
}
}
When __mptcp_move_skbs_from_subflow() then initializes the skb via
mptcp_init_skb(), it subtracts offset from mptcp_subflow_get_mapped_dsn():
net/mptcp/protocol.c:mptcp_init_skb() {
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;
}
Because MPTCP stripes mappings across subflows, the new mapping starting at
offset in skb can have an out-of-order DSN (mapped_dsn > msk->ack_seq) that is
not contiguous with the discarded bytes in 0..offset. Subtracting offset
assigns the unreceived DSN range [mapped_dsn - offset, mapped_dsn) to the
stale discarded prefix in 0..offset:
net/mptcp/protocol.c:__mptcp_move_skb() {
if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
/* in sequence */
insert:
...
} 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)->map_seq64 + skb->len, msk->ack_seq)) {
copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
goto insert;
}
}
If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb() sees
map_seq64 <= msk->ack_seq, queues the out-of-order skb onto
sk->sk_receive_queue, and advances msk->ack_seq across the missing DSN gap,
causing mptcp_recv_skb() and __mptcp_recvmsg_mskq() to deliver the discarded
prefix bytes from 0..offset to user space. If mapped_dsn - offset >
msk->ack_seq, mptcp_data_queue_ofo() queues the skb into
msk->out_of_order_queue as covering [mapped_dsn - offset, mapped_dsn +
skb->len - offset), dropping legitimate segments for [mapped_dsn - offset,
mapped_dsn) as duplicates and delivering the stale prefix once msk->ack_seq
reaches mapped_dsn - offset.
> @@ -4660,6 +4677,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)
> @@ -4669,7 +4687,7 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> int copied = 0;
> u32 offset;
>
> - msk_owned_by_me(msk);
> + DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk));
[Severity: Medium]
Does replacing msk_owned_by_me(msk) with
DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk)) cause a link failure when
CONFIG_DEBUG_NET=y and CONFIG_LOCKDEP=n?
In include/net/sock.h, lockdep_sock_is_held(sk) calls lockdep_is_held() on
both sk->sk_lock and sk->sk_lock.slock:
include/net/sock.h:lockdep_sock_is_held() {
return lockdep_is_held(&sk->sk_lock) ||
lockdep_is_held(&sk->sk_lock.slock);
}
When CONFIG_LOCKDEP=n, include/linux/lockdep.h only declares lockdep_is_held()
as a dummy extern prototype with no definition, relying on dead-code
elimination. Because CONFIG_DEBUG_NET does not depend on CONFIG_LOCKDEP,
enabling CONFIG_DEBUG_NET=y with CONFIG_LOCKDEP=n expands
DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk)) to WARN_ON_ONCE() and leaves
an undefined reference to lockdep_is_held at link time.
Note that msk_owned_by_me(msk) calls sock_owned_by_me(sk):
include/net/sock.h:sock_owned_by_me() {
#ifdef CONFIG_LOCKDEP
WARN_ON_ONCE(!lockdep_sock_is_held(sk) && debug_locks);
#endif
}
Since sock_owned_by_me(sk) already checks lockdep_sock_is_held(sk) for both
process and BH contexts while guarding on CONFIG_LOCKDEP and debug_locks, can
msk_owned_by_me(msk) be kept here as-is?
>
> if (sk->sk_state == TCP_LISTEN)
> return -ENOTCONN;
> @@ -4701,11 +4719,18 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> if (noack)
> goto out;
>
> - mptcp_rcv_space_adjust(msk, copied);
> -
> if (copied > 0) {
> - mptcp_recv_skb(sk, &offset);
> - mptcp_cleanup_rbuf(msk, copied);
> + if (sock_owned_by_user(sk)) {
> + /* Process context: inline cleanup */
> + mptcp_rcv_space_adjust(msk, copied);
> + mptcp_recv_skb(sk, &offset);
[Severity: Low]
The commit message states:
Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()) and drop the redundant mptcp_recv_skb() call after
the main loop.
Should this mptcp_recv_skb(sk, &offset) call in the sock_owned_by_user(sk)
branch be dropped to match the commit message, since the main loop already
calls mptcp_eat_recv_skb() on fully consumed skbs and mptcp_recv_skb() no
longer flushes msk->backlog_list?
[1] https://lore.kernel.org/r/90a300d74f44443759ebe14be70c07385267db5b.1793361629.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/90a300d74f44443759ebe14be70c07385267db5b.1793361629.git.tanggeliang@kylinos.cn
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=6
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker
2026-11-03 8:57 ` [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
2026-11-03 11:08 ` sashiko-bot
@ 2026-11-05 0:41 ` netdev-bot+sashiko
1 sibling, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-05 0:41 UTC (permalink / raw)
To: geliang; +Cc: mptcp, tanggeliang, pabeni, kuba
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 2
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: broken mptcp_inq_hint() 32-bit map_seq subtraction and
MSG_PEEK sync
- [High] mptcp: data corruption in mptcp_init_skb() when subtracting non-
zero offset
- [Medium] The commit message and the new code describe a BH call path
into __mptcp_read_sock() that does not exist in this tree.
- [Low] The commit message says the patch will 'drop the redundant
mptcp_recv_skb() call after the main loop', but the call is still…
- [Low] The commit message gives the wrong reason for replacing
msk_owned_by_me(msk) with…
--
mptcp: defer read_sock cleanup to mptcp_worker
This patch changes __mptcp_read_sock() so that, when the caller does not
own the socket lock, mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() are
deferred to mptcp_worker(). The commit message describes that case as the
BH context reached when MPTCP carries TLS. The patch also replaces the
msk_owned_by_me() assertion with a lockdep_sock_is_held() based
DEBUG_NET_WARN_ON_ONCE(), and removes the backlog flush from
mptcp_recv_skb().
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3e10b06b7772d..25f0d10373d22 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -4660,6 +4677,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)
> @@ -4669,7 +4687,7 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> int copied = 0;
> u32 offset;
>
> - msk_owned_by_me(msk);
> + DEBUG_NET_WARN_ON_ONCE(!lockdep_sock_is_held(sk));
[Severity: Low]
Is the reason given in the commit message for this change accurate?
Replace msk_owned_by_me() assertion with DEBUG_NET_WARN_ON_ONCE using
lockdep_sock_is_held() to verify proper locking for both process and
BH contexts.
msk_owned_by_me() wraps sock_owned_by_me(). Under CONFIG_LOCKDEP that
function is:
WARN_ON_ONCE(!lockdep_sock_is_held(sk) && debug_locks);
lockdep_sock_is_held() returns true when either sk->sk_lock or
sk->sk_lock.slock is held. A BH caller holding bh_lock_sock() therefore
already passed the old check.
The new code also calls sock_owned_by_user(), which calls
sock_owned_by_me() itself. The same check still runs whenever
copied > 0.
The only real change is that the check at the top of the function now
also depends on CONFIG_DEBUG_NET. That reduces debug coverage slightly,
and the commit message doesn't mention it.
Could the commit message be corrected, or the original assertion kept?
>
> if (sk->sk_state == TCP_LISTEN)
> return -ENOTCONN;
> @@ -4701,11 +4719,18 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> if (noack)
> goto out;
>
> - mptcp_rcv_space_adjust(msk, copied);
> -
> if (copied > 0) {
> - mptcp_recv_skb(sk, &offset);
> - mptcp_cleanup_rbuf(msk, copied);
> + if (sock_owned_by_user(sk)) {
> + /* Process context: inline cleanup */
> + mptcp_rcv_space_adjust(msk, copied);
> + mptcp_recv_skb(sk, &offset);
[Severity: Low]
The commit message says:
... and drop the redundant mptcp_recv_skb() call after the main loop.
This call is still present in the sock_owned_by_user() branch, which is
the path that actually runs. Only the new deferral branch leaves it out.
Was this call meant to be removed as well?
Now that the backlog flush is gone from mptcp_recv_skb(), this call does
almost nothing. The loop above already eats every fully consumed skb. It
exits with either an empty queue or a partially consumed skb at the head.
> + mptcp_cleanup_rbuf(msk, copied);
> + } else {
> + /* BH context: defer to worker */
> + msk->read_copied += copied;
> + set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
> + mptcp_schedule_work(sk);
> + }
[Severity: Medium]
Can this else branch be reached in the current tree?
The commit message says:
When MPTCP carries TLS, __mptcp_read_sock() may be reached from BH
context (via tls_strp_read_copyin() -> sk->sk_read_sock()), ...
However, tls_strp_read_copyin() in net/tls/tls_strp.c calls
tcp_read_sock() directly:
tcp_read_sock(strp->sk, &desc, tls_strp_copyin);
There also doesn't seem to be an sk_read_sock hook anywhere under
include/ or net/.
In addition, mptcp_setsockopt_sol_tcp() in net/mptcp/sockopt.c rejects
TCP_ULP:
case TCP_ULP:
return -EOPNOTSUPP;
So kTLS can't be attached to an MPTCP socket at this point.
Two callers can actually reach mptcp_read_sock(): mptcp_splice_read(),
and svc_tcp_read_msg() in net/sunrpc/svcsock.c. Both hold lock_sock():
lock_sock(sock->sk);
len = sock->ops->read_sock(sock->sk, &desc, svc_tcp_recv_actor);
The other generic ->read_sock() user that runs in BH is strparser, via
strp_data_ready(). Its KCM user only attaches IPPROTO_TCP sockets, as
checked in kcm_attach():
if ((csk->sk_family != AF_INET && csk->sk_family != AF_INET6) ||
csk->sk_protocol != IPPROTO_TCP) {
If that is right, sock_owned_by_user() is always true here. This branch,
MPTCP_WORK_READ_COMPLETE and mptcp_read_complete() would then be dead
code. The "not safe as both expect process context" problem described in
the commit message also can't be triggered today.
Is this groundwork for a TLS-over-MPTCP series that hasn't been merged
yet? If so, could the commit message say that and name the series, or
could the patch be carried as part of that series?
Nothing in the tree currently exercises the locking assumptions this
branch depends on (slock held, owned == 0).
> }
> out:
> return copied;
[ ... ]
> @@ -3156,6 +3156,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;
[Severity: High]
This isn't introduced by this patch, but both the inline branch and
the new mptcp_read_complete() go through mptcp_rcv_space_adjust(),
which does:
copied = msk->rcvq_space.copied;
copied -= mptcp_inq_hint(sk);
if (copied <= msk->rcvq_space.space)
goto new_measure;
mptcp_inq_hint() still computes:
u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
MPTCP_SKB_CB(skb)->map_seq is now a u32. It is zero-extended and
subtracted from the 64-bit msk->ack_seq. msk->ack_seq is derived from
the IASN, so its upper 32 bits are non-zero on almost every
connection. Whenever anything is left in sk->sk_receive_queue,
hint_val ends up >= INT_MAX and mptcp_inq_hint() returns INT_MAX.
Even with a zero upper half the result is wrong. map_seq is no longer
advanced on partial reads, and consumption is tracked in
msk->copied_seq instead. The hint therefore also counts bytes already
read from the head skb. The commit that removed the CB offset field
said mptcp_inq_hint() would compute ack_seq - copied_seq, but that
change doesn't seem to be in the tree.
As a result, copied goes negative in mptcp_rcv_space_adjust() and
mptcp_rcvbuf_grow() is skipped. Receive buffer autotuning silently
stops whenever a read leaves a partially consumed skb behind. SIOCINQ
and TCP_CM_INQ report the same bogus value to userspace.
There is a related MSG_PEEK problem in mptcp_recvmsg(). peek_seq is
only resynced when mptcp_move_skbs() returns true. On a passive TFO
connection, mptcp_move_skbs() can process MPTCP_SYNC_SEQ, which adds
msk->tfo_iasn to msk->copied_seq, and still return false if no new
data was enqueued. peek_seq is then left in the dummy sequence space
used before the IASN was known.
Should mptcp_inq_hint() be computing msk->ack_seq - msk->copied_seq
here? This probably wants its own patch with a Fixes: tag rather than
being folded into this series. Is someone already looking at it?
[ ... ]
> @@ -4643,9 +4663,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) {
[Severity: High]
This one is pre-existing rather than something this patch adds.
mptcp_recv_skb() relies on MPTCP_SKB_CB(skb)->map_seq to find the
read offset in the head skb. That value can be wrong for an skb whose
prefix was discarded on the subflow.
In subflow_check_data_avail(), a subflow skb can hold the tail of an
already acked DSS mapping followed by the start of a new mapping. In
that case mptcp_subflow_discard_data() advances tp->copied_seq past
the old bytes, clears subflow->map_valid, and leaves the skb at the
head of ssk->sk_receive_queue. The next iteration then installs the
new mapping starting at that offset inside the same skb.
__mptcp_move_skbs_from_subflow() then calls
mptcp_init_skb(ssk, skb, offset), which does:
MPTCP_SKB_CB(skb)->map_seq64 =
mptcp_subflow_get_mapped_dsn(subflow) - offset;
That assigns the DSN range [mapped_dsn - offset, mapped_dsn) to the
discarded prefix bytes. This is only right if the old and new mappings
are contiguous in DSN space. When data is striped across several
subflows, the new mapping can be out of order, so contiguity is not
guaranteed.
__mptcp_move_skb() then classifies the skb using the shifted
map_seq64:
- If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, the skb is
treated as in-sequence or partial. It is queued on
sk->sk_receive_queue, and msk->ack_seq jumps over the hole
[msk->ack_seq, mapped_dsn). The reader then copies the stale
prefix bytes to userspace instead of data that was never
received.
- If mapped_dsn - offset > msk->ack_seq, the skb goes into
out_of_order_queue covering the wrong range. Valid segments for
[mapped_dsn - offset, mapped_dsn) are dropped as duplicates, and
the stale prefix is delivered later.
Either way the application gets the wrong bytes. The segment layout
that triggers this is under the peer's control.
Should the discarded prefix be trimmed off the skb (or the skb split)
before it is moved to the msk, instead of shifting map_seq64 back by
offset? This looks like it needs its own fix with a Fixes: tag.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793695518.git.tanggeliang%40kylinos.cn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage
2026-11-03 8:57 [PATCH mptcp-next v20 0/6] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (5 preceding siblings ...)
2026-11-03 8:57 ` [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
@ 2026-11-03 10:22 ` MPTCP CI
6 siblings, 0 replies; 18+ messages in thread
From: MPTCP CI @ 2026-11-03 10:22 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): Script error! ❓
- KVM Validation: normal (only selftest_mptcp_join): Script error! ❓
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Script error! ❓
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Unstable: 0 failed test(s): ⚠️
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/41609865581
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8b0ebaec7c2e
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1200005
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] 18+ messages in thread