Re: [PATCH mptcp-next v20 6/6] mptcp: defer read_sock cleanup to mptcp_worker

[sashiko-bot@kernel.org]

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