Re: [PATCH mptcp-next v5 06/12] mptcp: remove CB offset field

[gang.yan@linux.dev]

May 11, 2026 at 5:03 AM, "Paolo Abeni" <pabeni@redhat.com mailto:pabeni@redhat.com?to=%22Paolo%20Abeni%22%20%3Cpabeni%40redhat.com%3E > wrote:


> 
> 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 the msk release
> CB, when the socket is owned by the user-space at remote key reception
> time. Such synchronization preserves any partial progress (copy) made on
> the TFO packet.
> 
> Note that the explicit synchronization in __mptcp_move_skb() is needed to
> ensure that the TFO skb in the receive queue got its map_seq synched
> before the next skb lands into the receive queue when spooling the backlog
> at mptcp_release_cb() time, as the release CB synchronization will happen
> later.
> 
> Prior to this patch, the TFO skb dummy mapping was always ignored, now it
> affects the `copied_seq` initial update: be sure to extends the sign
> correctly of such mapping initialization time.
> 
> Overall this simplify a bit the __mptcp_recvmsg_mskq(), mptcp_inq_hint()
> and the __mptcp_move_skb() code and will also make possible the next
> patch.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v4 -> v5:
>  - fix transient build issue, restoring a __mptcp_move_skb() chunk that
>  leaked in the next patch.
> 
> v3 -> v4:
>  - fix peek seq race
> 
> v2 -> v3:
>  - do not use msk->first in release_cb to deal with MPTCP_SYNC_SEQ:
>  subflow->iasn access is (data) racy and msk->first can be null, instead
>  recompute iasn from msk bytes_received and TFO skb len
>  - when updating copied_seq after remote key reception, add iasn to it
>  instead of overwriting, to avoid deleting any partial progress.
> 
> v1 -> v2:
>  - deal correctly with peek, as usally "inspired" from the correspondent
>  tcp code
>  - update mptcp_inq_hint(), too
> 
> Notes:
> - this explicitly relays on "mptcp: do not drop partial packets" to
> avoid dropping partially consumed packets
> - sashiko may confuse the 'offset' in mptcp_init_skb for an MPTCP-level
>  one, but it refers to the TCP sequence space. Conclusion out of the
>  that assumptions are wrong.
> - the data race in mptcp_inq_hint() is real, but pre-existing and can
>  impact only sockopt() output - the other call-sites are race free, as
>  ack_seq updates are serialized by the RX path.
>  Fixing the race for good without sashiko tripping on other similar
>  minor races would require another largish series. Postponed.
> - sashiko may see a race with `copied_seq` in mptcp_recv_skb(), that is
>  not real: subflow_set_remote_key()/__mptcp_sync_rcv_sequence() has seen
>  the msk owned; if mptcp_data_ready() has seen again the msk owend, the
>  only skb in the receive queue can be the (unsynched) TFO one, with dummy
>  sequence. If mptcp_data_ready() observed msk not owned and queued more
>  skbs, the release_cb() has run and synched `copied_seq` and TFO skb
>  map_seq.
> ---
>  net/mptcp/fastopen.c | 15 +++--
>  net/mptcp/protocol.c | 129 ++++++++++++++++++-------------------------
>  net/mptcp/protocol.h | 8 ++-
>  net/mptcp/subflow.c | 7 ++-
>  4 files changed, 77 insertions(+), 82 deletions(-)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index c7d5bee8088e..03e605b050f8 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;
> @@ -43,20 +44,24 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
>  subflow->ssn_offset += skb->len;
>  has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
>  
> - /* Only the sequence delta is relevant */
> - MPTCP_SKB_CB(skb)->map_seq = -skb->len;
> + /* The TFO segment data sits before the IASN; before receiving
> + * the remote key, IASN is assumed being 0.
> + */
> + MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
>  MPTCP_SKB_CB(skb)->end_seq = 0;
> - MPTCP_SKB_CB(skb)->offset = 0;
>  MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
>  
>  mptcp_data_lock(sk);
>  DEBUG_NET_WARN_ON_ONCE(sock_owned_by_user_nocheck(sk));
>  
> - mptcp_sk(sk)->rcvd_dummy_seq = true;
> + msk = mptcp_sk(sk);
> + msk->rcvd_dummy_seq = true;
> + msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
> + msk->tfo_skb_len = skb->len;
>  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 6909586a3090..47df6d4a26a7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -28,7 +28,7 @@
>  #include "protocol.h"
>  #include "mib.h"
>  
> -static unsigned int mptcp_inq_hint(const struct sock *sk);
> +static unsigned int mptcp_inq_hint(struct sock *sk);
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/mptcp.h>
> @@ -160,7 +160,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;
> @@ -342,8 +341,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;
> @@ -352,9 +350,9 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
>  * value
>  */
> - MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> - MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> - MPTCP_SKB_CB(skb)->offset = offset;
Hi Paolo:

Sorry for digging up this old thread.

Do you still plan to remove the offset field?

I've left some comments on Geliang's TLS patches [1], and the workaround patch needs
the offset field. Geliang and I would like to know whether you intend to drop it,
address it in the short term, or have no near-term plan for it.


[1] https://patchwork.kernel.org/project/mptcp/patch/6557d95ab11416b3e798781cf95811bd6dd60d9e.1779788090.git.tanggeliang@kylinos.cn/

Thanks
Gang

> + 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);
> @@ -377,8 +375,8 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
>  if (!skb)
>  return;
>  
> - MPTCP_SKB_CB(skb)->map_seq = msk->ack_seq - skb->len;
> - MPTCP_SKB_CB(skb)->end_seq = msk->ack_seq;
> + MPTCP_SKB_CB(skb)->map_seq = mptcp_iasn(msk) - skb->len;
> + 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)
> @@ -405,6 +403,7 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  }
>  
>  if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> +add_queue:
>  /* in sequence */
>  msk->bytes_received += copy_len;
>  WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);
> @@ -418,28 +417,16 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  } else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
>  mptcp_data_queue_ofo(msk, skb);
>  return false;
> + } else if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
> + /* Partial packet: map_seq < ack_seq < end_seq.*/
> + copy_len -= msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
> + goto add_queue;
>  }
>  
> - /* Completely old data? */
> - if (!after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
> - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
> - mptcp_drop(sk, skb);
> - return false;
> - }
> -
> - /* Partial packet: map_seq < ack_seq < end_seq.
> - * Skip the already-acked bytes and enqueue the new data.
> - */
> - 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;
> - msk->bytes_received += copy_len;
> - WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);
> -
> - skb_set_owner_r(skb, sk);
> - __skb_queue_tail(&sk->sk_receive_queue, skb);
> - return true;
> + /* Completely old data. */
> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
> + mptcp_drop(sk, skb);
> + return false;
>  }
>  
>  static void mptcp_stop_rtx_timer(struct sock *sk)
> @@ -783,7 +770,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);
> @@ -850,8 +837,6 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
>  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;
> @@ -2095,34 +2080,22 @@ 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;
> - u32 data_len = skb->len - offset;
> - u32 count;
> + u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
> + u32 count, data_len = skb->len - offset;
>  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)
> + continue;
>  
>  count = min_t(size_t, len - copied, data_len);
>  if (!(flags & MSG_TRUNC)) {
> @@ -2140,14 +2113,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 {
> @@ -2296,25 +2267,23 @@ static bool mptcp_move_skbs(struct sock *sk)
>  return enqueued;
>  }
>  
> -static unsigned int mptcp_inq_hint(const struct sock *sk)
> +static unsigned int mptcp_inq_hint(struct sock *sk)
>  {
>  const struct mptcp_sock *msk = mptcp_sk(sk);
> - const struct sk_buff *skb;
> -
> - skb = skb_peek(&sk->sk_receive_queue);
> - if (skb) {
> - u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> + u64 hint_val;
>  
> - if (hint_val >= INT_MAX)
> - return INT_MAX;
> -
> - return (unsigned int)hint_val;
> - }
> + /* Avoid races vs ack_seq updates. */
> + mptcp_data_lock(sk);
> + hint_val = msk->ack_seq - msk->copied_seq;
> + mptcp_data_unlock(sk);
> + if (hint_val >= INT_MAX)
> + return INT_MAX;
>  
> - if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> + if (!hint_val &&
> + (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
>  return 1;
>  
> - return 0;
> + return (unsigned int)hint_val;
>  }
>  
>  static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> @@ -2323,6 +2292,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;
>  
> @@ -2342,6 +2312,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;
> @@ -2351,7 +2326,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)
> @@ -2406,6 +2381,10 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  err = 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);
> @@ -3500,11 +3479,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  msk->bytes_retrans = 0;
>  msk->rcvspace_init = 0;
>  msk->fastclosing = 0;
> + msk->tfo_skb_len = 0;
>  mptcp_init_rtt_est(msk);
>  
>  /* for fallback's sake */
>  WRITE_ONCE(msk->ack_seq, 0);
>  atomic64_set(&msk->rcv_wnd_sent, 0);
> + msk->copied_seq = 0;
>  
>  WRITE_ONCE(sk->sk_shutdown, 0);
>  sk_error_report(sk);
> @@ -3729,8 +3710,10 @@ static void mptcp_release_cb(struct sock *sk)
>  __mptcp_error_report(sk);
>  if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
>  __mptcp_sync_sndbuf(sk);
> - 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 += mptcp_iasn(msk);
>  __mptcp_sync_rcv_sequence(sk);
> + }
>  }
>  }
>  
> @@ -4390,7 +4373,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;
> @@ -4432,11 +4415,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);
>  }
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 16a1f4531dad..f3d852e52982 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -129,7 +129,6 @@
>  struct mptcp_skb_cb {
>  u64 map_seq;
>  u64 end_seq;
> - u32 offset;
>  u8 has_rxtstamp;
>  };
>  
> @@ -289,6 +288,7 @@ struct mptcp_sock {
>  u64 bytes_sent;
>  u64 snd_nxt;
>  u64 bytes_received;
> + u64 copied_seq;
>  u64 ack_seq;
>  atomic64_t rcv_wnd_sent;
>  u64 rcv_data_fin_seq;
> @@ -308,6 +308,7 @@ struct mptcp_sock {
>  u32 last_ack_recv;
>  unsigned long timer_ival;
>  u32 token;
> + u32 tfo_skb_len;
>  unsigned long flags;
>  unsigned long cb_flags;
>  bool rcvd_dummy_seq;
> @@ -859,6 +860,11 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
>  int mptcp_sched_get_send(struct mptcp_sock *msk);
>  int mptcp_sched_get_retrans(struct mptcp_sock *msk);
>  
> +static inline u64 mptcp_iasn(const struct mptcp_sock *msk)
> +{
> + return msk->ack_seq - msk->bytes_received + msk->tfo_skb_len;
> +}
> +
>  static inline u64 mptcp_data_avail(const struct mptcp_sock *msk)
>  {
>  return READ_ONCE(msk->bytes_received) - READ_ONCE(msk->bytes_consumed);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 5f371bf773f8..c8ea876bdd03 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -499,10 +499,13 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  WRITE_ONCE(msk->can_ack, true);
>  atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
>  
> - if (!sock_owned_by_user(sk))
> + if (!sock_owned_by_user(sk)) {
> + /* User space could have already read partially the TFO skb */
> + 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,
> -- 
> 2.54.0
>