[PATCH mptcp-next v6 0/7] prepare interfaces for kTLS and sockmap support

[PATCH mptcp-next v6 0/7] prepare interfaces for kTLS and sockmap support

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v6:
 - patch 2: add Fixes tag, and use '-net' prefix.
 - patch 3: remove poll() call, check SIOCINQ directly after accept().
 - patch 6: add READ_ONCE() for msk->flags and msk->cb_flags reads in
   mptcp_rate_check_app_limited() to annotate intentionally racy accesses.
 - update commit logs.

v5:
 - patch 2: new patch - rewrite mptcp_inq_hint() to use mptcp_data_avail()
   instead of ack_seq - map_seq, completely avoiding TFO overflow issues.
 - patch 3: new patch - add TFO SIOCINQ overflow detection in
   mptcp_connect test to catch INT_MAX returns on TFO connections.
 - patch 4: mptcp_inq() now returns INT_MAX instead of 0 when avail >=
   INT_MAX. Removed mptcp_inq_hint() changes (moved to patch 2).
 - patch 6: replace mptcp_rtx_head() check with MPTCP_WORK_RTX and
   MPTCP_RETRANSMIT flag checks in mptcp_rate_check_app_limited() to
   properly detect pending retransmissions without data race issues.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793676064.git.tanggeliang@kylinos.cn/

v4:
 - patch 2: fix mptcp_inq_hint() to return 0 instead of INT_MAX on
   passive TFO overflow (ack_seq - map_seq >= INT_MAX).
 - patch 4: add mptcp_rtx_head(sk) to the early-return check in
   mptcp_rate_check_app_limited() so pending retransmissions also
   prevent false app-limited marking.
 - patch 5: update comment message.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793588257.git.tanggeliang@kylinos.cn/

v3:
 - patch 2: drop DATA_FIN subtraction in mptcp_inq(), remove the
   intermediate 'answ' variable and return (unsigned int)avail
   directly. Update commit message accordingly.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793324316.git.tanggeliang@kylinos.cn/

v2:
 - drop the patch "prevent dummy DATA_FIN on RST in fallback mode".
 - keep mptcp_inq_hint in mptcp_ioctl unchanged.
 - use mptcp_data_avail in mptcp_inq.
 - add msk_owned_by_me check in mptcp_sendmsg_locked.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1793258749.git.tanggeliang@kylinos.cn/

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

This series was previously posted as part of "Reduce the differences
between TCP and MPTCP for TLS usage" v15. It creates the mptcp_inq()
and mptcp_sendmsg_locked() interfaces that are required for the upcoming
MPTCP kTLS and MPTCP sockmap implementations.

The series has been rebased and no longer depends on Paolo's CB layout
refactoring patches (mptcp: drop the mptcp_ooo_try_coalesce() helper,
mptcp: drop the cant_coalesce CB field, mptcp: remove CB offset field,
mptcp: sync mptcp skb cb layout with tcp one, mptcp: defer read_sock
cleanup to mptcp_worker). Those patches are more complex and will be
sent in a separate series.

Patch overview:

1. mptcp: update mptcp_check_readable
 - Align with TCP's sock_is_readable for better TLS compatibility

2. mptcp: prevent dummy DATA_FIN on RST in fallback mode
 - Fix semantic issue where RST on fallback connections would create
   dummy DATA_FIN, causing unnecessary state transitions
 - Add sk_err check to subflow_is_done() to distinguish abortive
   closes (RST) from clean closes (FIN)

3. mptcp: implement peek_len for proto_ops
 - Add mptcp_inq() to report readable bytes at MPTCP level
 - Wire into .peek_len proto_ops for both IPv4 and IPv6
 - Handle 32-bit torn reads by returning 0 instead of INT_MAX
 - Prepare for TLS strparser integration

4. mptcp: add sendmsg_locked to proto_ops
 - Split mptcp_sendmsg() into locked/unlocked variants
 - Expose .sendmsg_locked in proto_ops for TLS send path

5. mptcp: track app-limited state in mptcp_sendmsg
 - Add mptcp_rate_check_app_limited() to mark subflows as app-limited
 - Bail out when MPTCP has unsent data (write_seq != snd_nxt or
   mptcp_send_head() != NULL) to avoid false app-limited marking
 - Enable accurate delivery rate estimation for BBR

6. selftests: mptcp: sockopt: check app_limited
 - Add regression test for app-limited accounting
 - Verify tcpi_delivery_rate_app_limited == 1 after small transfers

Gang Yan (1):
  mptcp: update mptcp_check_readable

Geliang Tang (6):
  mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint
  selftests: mptcp: connect: check TFO SIOCINQ overflow
  mptcp: implement peek_len for proto_ops
  mptcp: add sendmsg_locked to proto_ops
  mptcp: track app-limited state in mptcp_sendmsg
  selftests: mptcp: sockopt: check app_limited

 net/mptcp/protocol.c                          | 96 +++++++++++++++----
 .../selftests/net/mptcp/mptcp_connect.c       | 11 +++
 .../selftests/net/mptcp/mptcp_sockopt.c       |  5 +
 3 files changed, 93 insertions(+), 19 deletions(-)

-- 
2.55.0

[PATCH mptcp-next v6 1/7] mptcp: update mptcp_check_readable

[Geliang Tang]

From: Gang Yan <yangang@kylinos.cn>

This patch makes mptcp_check_readable() aligned with TCP, and renames it to
mptcp_stream_is_readable(). It will be used in the case of KTLS, because
'prot' will be modified, tls_sw_sock_is_readable() is expected to be called
from prot->sock_is_readable().

Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/protocol.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..0caa31525b90 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3476,9 +3476,11 @@ void __mptcp_unaccepted_force_close(struct sock *sk)
 	__mptcp_destroy_sock(sk);
 }
 
-static __poll_t mptcp_check_readable(struct sock *sk)
+static bool mptcp_stream_is_readable(struct sock *sk)
 {
-	return mptcp_epollin_ready(sk) ? EPOLLIN | EPOLLRDNORM : 0;
+	if (mptcp_epollin_ready(sk))
+		return true;
+	return sk_is_readable(sk);
 }
 
 static void mptcp_check_listen_stop(struct sock *sk)
@@ -4580,7 +4582,8 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
 		mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
 
 	if (state != TCP_SYN_SENT && state != TCP_SYN_RECV) {
-		mask |= mptcp_check_readable(sk);
+		if (mptcp_stream_is_readable(sk))
+			mask |= EPOLLIN | EPOLLRDNORM;
 		if (shutdown & SEND_SHUTDOWN)
 			mask |= EPOLLOUT | EPOLLWRNORM;
 		else
-- 
2.55.0

[PATCH mptcp-net v6 2/7] mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Fix mptcp_inq_hint() to compute available bytes using mptcp_data_avail()
(bytes_received - bytes_consumed) instead of ack_seq minus the head skb's
map_seq.

On passive TFO connections, the TFO skb has map_seq set to -skb->len
(representing data before IASN). When the third ACK arrives,
subflow_set_remote_key() sets msk->ack_seq to subflow->iasn. Before
commit f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()"),
that function adjusted the TFO skb's map_seq. Since then, mptcp_inq_hint()
computes ack_seq - map_seq = iasn + len, which is almost always >= INT_MAX,
causing the function to return INT_MAX.

As a result, SIOCINQ/FIONREAD through mptcp_ioctl() and the TCP_CM_INQ
cmsg in mptcp_recvmsg() report INT_MAX instead of the actual byte count,
and mptcp_rcv_space_adjust() subtracts INT_MAX from copied. This lasts
until the TFO skb is fully consumed.

Using mptcp_data_avail() avoids this regression by tracking bytes directly
without depending on map_seq.

The EOF hint behavior (returning 1 when socket is closed with no data)
is preserved.

Fixes: f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0caa31525b90..1704e689bc93 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2402,22 +2402,16 @@ static bool mptcp_move_skbs(struct sock *sk)
 static unsigned int mptcp_inq_hint(const struct sock *sk)
 {
 	const struct mptcp_sock *msk = mptcp_sk(sk);
-	const struct sk_buff *skb;
+	u64 hint_val = mptcp_data_avail(msk);
 
-	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;
 
-		if (hint_val >= INT_MAX)
-			return INT_MAX;
-
-		return (unsigned int)hint_val;
-	}
-
-	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
+	if (!hint_val &&
+	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
 		return 1;
 
-	return 0;
+	return (unsigned int)hint_val;
 }
 
 static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
-- 
2.55.0

[PATCH mptcp-next v6 3/7] selftests: mptcp: connect: check TFO SIOCINQ overflow

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add a check in mptcp_connect's server mode to detect TFO SIOCINQ overflow.
When a TFO connection is accepted, poll for data to arrive, then check if
SIOCINQ returns INT_MAX. This detects the regression where mptcp_inq_hint()
incorrectly returns INT_MAX for passive TFO connections.

The check runs when the MPTFO sockopt is set. It polls the accepted socket
for up to one second, then uses ioctl(SIOCINQ) to verify the available
byte count. If SIOCINQ returns INT_MAX, the test fails.

This test verifies the fix in the preceding commit "mptcp: fix TFO SIOCINQ
overflow in mptcp_inq_hint".

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..83db495704fe 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1207,6 +1207,17 @@ int main_loop_s(int listensock)
 		check_sockaddr(pf, &ss, salen);
 		check_getpeername(remotesock, &ss, salen);
 
+		if (cfg_sockopt_types.mptfo) {
+			int avail;
+
+			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
+			    avail == INT_MAX) {
+				fprintf(stderr, "TFO SIOCINQ overflow\n");
+				close(remotesock);
+				return 1;
+			}
+		}
+
 		if (cfg_input) {
 			fd = open(cfg_input, O_RDONLY);
 			if (fd < 0)
-- 
2.55.0

[PATCH mptcp-next v6 4/7] mptcp: implement peek_len for proto_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add mptcp_inq() to compute readable bytes at the MPTCP level using
mptcp_data_avail() (bytes_received - bytes_consumed). This aligns with
TCP's design where tcp_inq() is used by peek_len and tcp_inq_hint() is
used by SIOCINQ.

Like tcp_inq(), mptcp_inq() returns 0 during handshake (TCPF_SYN_SENT/
TCPF_SYN_RECV), INT_MAX when avail >= INT_MAX, otherwise the actual byte
count. Unlike mptcp_inq_hint() (which mirrors tcp_inq_hint()), this helper
does not include the EOF hint (returning 1 when socket is closed with no
data) because peek_len is used by upper layers like strp that only need
the data length, not connection state information. It also does not drain
the backlog before reading counters, as peek_len provides a heuristic
value for rate limiting rather than an exact count for userspace queries.

Wire into mptcp_peek_len() and assign .peek_len in both mptcp_stream_ops
and mptcp_v6_stream_ops.

This prepares for upcoming kTLS and sockmap support.

Co-developed-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Zqiang <qiang.zhang@linux.dev>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1704e689bc93..aed25c901e4b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4791,6 +4791,26 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 	return ret;
 }
 
+static int mptcp_inq(struct sock *sk)
+{
+	const struct mptcp_sock *msk = mptcp_sk(sk);
+	u64 avail;
+
+	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
+		return 0;
+
+	avail = mptcp_data_avail(msk);
+	if (avail >= INT_MAX)
+		return INT_MAX;
+
+	return (unsigned int)avail;
+}
+
+static int mptcp_peek_len(struct socket *sock)
+{
+	return mptcp_inq(sock->sk);
+}
+
 static const struct proto_ops mptcp_stream_ops = {
 	.family		   = PF_INET,
 	.owner		   = THIS_MODULE,
@@ -4813,6 +4833,7 @@ static const struct proto_ops mptcp_stream_ops = {
 	.set_rcvlowat	   = mptcp_set_rcvlowat,
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
+	.peek_len	   = mptcp_peek_len,
 };
 
 static struct inet_protosw mptcp_protosw = {
@@ -4925,6 +4946,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
 	.set_rcvlowat	   = mptcp_set_rcvlowat,
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
+	.peek_len	   = mptcp_peek_len,
 };
 
 static struct proto mptcp_v6_prot;
-- 
2.55.0

[PATCH mptcp-next v6 5/7] mptcp: add sendmsg_locked to proto_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

MPTCP currently provides a standard sendmsg() implementation which
acquires and releases the socket lock internally. However, certain
upper layers need to call the sendmsg method while the socket lock
is already held.

Split the existing mptcp_sendmsg() into mptcp_sendmsg_locked() which
assumes the caller holds the socket lock, and a tiny wrapper
mptcp_sendmsg() that acquires the lock and calls the locked version.
Add msk_owned_by_me() assertion to enforce the locking requirement.
Expose .sendmsg_locked in both mptcp_stream_ops and mptcp_v6_stream_ops.

This prepares for upcoming kTLS and sockmap support.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index aed25c901e4b..b9c7b0bb6f74 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2038,7 +2038,7 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
 	}
 }
 
-static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
+static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct page_frag *pfrag;
@@ -2046,12 +2046,12 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	int ret = 0;
 	long timeo;
 
+	msk_owned_by_me(msk);
+
 	/* silently ignore everything else */
 	msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
 			  MSG_FASTOPEN | MSG_EOR;
 
-	lock_sock(sk);
-
 	mptcp_rps_record_subflows(msk);
 
 	if (unlikely(inet_test_bit(DEFER_CONNECT, sk) ||
@@ -2167,7 +2167,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	}
 
 out:
-	release_sock(sk);
 	return copied;
 
 do_error:
@@ -2180,6 +2179,17 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	goto out;
 }
 
+static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
+{
+	int ret;
+
+	lock_sock(sk);
+	ret = mptcp_sendmsg_locked(sk, msg, len);
+	release_sock(sk);
+
+	return ret;
+}
+
 static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied);
 
 static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
@@ -4834,6 +4844,7 @@ static const struct proto_ops mptcp_stream_ops = {
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
 	.peek_len	   = mptcp_peek_len,
+	.sendmsg_locked	   = mptcp_sendmsg_locked,
 };
 
 static struct inet_protosw mptcp_protosw = {
@@ -4947,6 +4958,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
 	.read_sock	   = mptcp_read_sock,
 	.splice_read	   = mptcp_splice_read,
 	.peek_len	   = mptcp_peek_len,
+	.sendmsg_locked	   = mptcp_sendmsg_locked,
 };
 
 static struct proto mptcp_v6_prot;
-- 
2.55.0

[PATCH mptcp-next v6 6/7] mptcp: track app-limited state in mptcp_sendmsg

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add mptcp_rate_check_app_limited() that walks every subflow and runs
tcp_rate_check_app_limited() under each subflow's socket lock. The
helper bails out early when MPTCP still has data to send: unsent data
(write_seq != snd_nxt), pending dfrags (mptcp_send_head), or
pending retransmission (MPTCP_WORK_RTX or MPTCP_RETRANSMIT flags).
In these cases marking subflows app-limited would mislead congestion
controllers.

Invoke from mptcp_sendmsg_locked() after send-side setup so BBR and
similar controllers can ignore rate samples taken while the connection
is genuinely application-limited.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b9c7b0bb6f74..889f244d9da0 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2038,6 +2038,30 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
 	}
 }
 
+static void mptcp_rate_check_app_limited(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct mptcp_subflow_context *subflow;
+	unsigned long flags, cb_flags;
+
+	flags = READ_ONCE(msk->flags);
+	cb_flags = READ_ONCE(msk->cb_flags);
+
+	if (msk->write_seq != msk->snd_nxt || mptcp_send_head(sk) ||
+	    test_bit(MPTCP_WORK_RTX, &flags) ||
+	    test_bit(MPTCP_RETRANSMIT, &cb_flags))
+		return;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		bool slow;
+
+		slow = lock_sock_fast_nested(ssk);
+		tcp_rate_check_app_limited(ssk);
+		unlock_sock_fast(ssk, slow);
+	}
+}
+
 static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -2068,6 +2092,9 @@ static int mptcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t len)
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
+	/* is sending application-limited? */
+	mptcp_rate_check_app_limited(sk);
+
 	if ((1 << sk->sk_state) & ~(TCPF_ESTABLISHED | TCPF_CLOSE_WAIT)) {
 		ret = sk_stream_wait_connect(sk, &timeo);
 		if (ret)
-- 
2.55.0

[PATCH mptcp-next v6 7/7] selftests: mptcp: sockopt: check app_limited

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

After a small data exchange, assert tcpi_delivery_rate_app_limited == 1.

rate_app_limited starts at 1 (tcp_init_sock) and is cleared only by
non-app-limited rate samples. The preceding commit makes
mptcp_rate_check_app_limited() set tp->app_limited on subflows when
MPTCP has no pending data, marking outgoing skbs as app-limited so rate
samples don't clear rate_app_limited.

This guards against regressions in subflow app-limited accounting.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b8158a1198eb..9517c530ba21 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -635,6 +635,11 @@ static void connect_one_server(int fd, int pipefd)
 	do_getsockopts(&s, fd, total, total);
 
 	assert(s.mptcpi_rcv_delta == (uint64_t)total);
+
+	/* app-limited accounting only checked under EXPECT_ALL_FEATURES */
+	if (expect_all_features())
+		assert(s.tcp_info.tcpi_delivery_rate_app_limited == 1);
+
 	close(fd);
 }
 
-- 
2.55.0

Re: [PATCH mptcp-next v6 3/7] selftests: mptcp: connect: check TFO SIOCINQ overflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: mptcp: commit message mismatch on polling remotesock in main_loop_s()
- [Medium] selftests: mptcp: SIOCINQ check in main_loop_s() races with TFO third ACK
--

commit d076cef4545cfcddd6e28fb675be9a4054df1b3b
Author: Geliang Tang <geliang@kernel.org>

selftests: mptcp: connect: check TFO SIOCINQ overflow

This commit adds a SIOCINQ check in mptcp_connect's server mode when the
MPTFO socket option is enabled to detect TFO SIOCINQ overflow regressions.

>     selftests: mptcp: connect: check TFO SIOCINQ overflow
>     
>     Add a check in mptcp_connect's server mode to detect TFO SIOCINQ overflow.
>     When a TFO connection is accepted, poll for data to arrive, then check if
>     SIOCINQ returns INT_MAX. This detects the regression where mptcp_inq_hint()
>     incorrectly returns INT_MAX for passive TFO connections.
>     
>     The check runs when the MPTFO sockopt is set. It polls the accepted socket
>     for up to one second, then uses ioctl(SIOCINQ) to verify the available
>     byte count. If SIOCINQ returns INT_MAX, the test fails.
>     
>     This test verifies the fix in the preceding commit "mptcp: fix TFO SIOCINQ
>     overflow in mptcp_inq_hint".
>     
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Medium]
Does the commit message match the actual code in main_loop_s()? The commit
message states in two places that the test polls the accepted socket for up to
one second before calling ioctl(SIOCINQ), whereas main_loop_s() in
tools/testing/selftests/net/mptcp/mptcp_connect.c invokes ioctl(remotesock,
SIOCINQ, &avail) directly without calling poll().

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..83db495704fec 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1207,6 +1207,17 @@ int main_loop_s(int listensock)
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
>  
> +		if (cfg_sockopt_types.mptfo) {
> +			int avail;
> +
> +			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
> +			    avail == INT_MAX) {
> +				fprintf(stderr, "TFO SIOCINQ overflow\n");
> +				close(remotesock);
> +				return 1;
> +			}
> +		}

[Severity: Medium]
Can this SIOCINQ check in main_loop_s() race with the TFO third ACK and miss
the overflow regression on unfixed kernels?

When a passive MPTCP Fast Open SYN+data packet with a valid cookie arrives in
tcp_conn_request(), the child msk is created via:

  tcp_try_fastopen()
    -> tcp_fastopen_create_child()
      -> subflow_syn_recv_sock()
        -> mptcp_sk_clone_init()

At that point mp_opt->suboptions only has OPTION_MPTCP_MPC_SYN (not
OPTION_MPTCP_MPC_ACK), leaving msk->ack_seq at 0. Next, af_ops->send_synack()
calls subflow_prep_synack() -> mptcp_fastopen_subflow_synack_set_params() in
net/mptcp/fastopen.c:

	MPTCP_SKB_CB(skb)->map_seq = -skb->len;
	MPTCP_SKB_CB(skb)->end_seq = 0;
	...
	__skb_queue_tail(&sk->sk_receive_queue, skb);

Then tcp_fastopen_create_child() immediately adds the child socket to the
listener accept queue via inet_csk_reqsk_queue_add() and wakes the listener via
sk->sk_data_ready().

Before the client receives the SYN-ACK and replies with the third ACK carrying
OPTION_MPTCP_MPC_ACK, msk->ack_seq is still 0. If main_loop_s() accepts the
socket and calls ioctl(remotesock, SIOCINQ, &avail) before the third ACK is
processed, even an unfixed mptcp_inq_hint() computes:

  READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq = 0 - (-skb->len) = skb->len

rather than overflowing to INT_MAX. The overflow to subflow->iasn + skb->len >=
INT_MAX only occurs after the server processes the third ACK in:

  tcp_rcv_state_process()
    -> tcp_data_queue()
      -> mptcp_incoming_options()
        -> check_fully_established()
          -> __mptcp_subflow_fully_established()
            -> subflow_set_remote_key()

which sets WRITE_ONCE(msk->ack_seq, subflow->iasn) in net/mptcp/subflow.c.
Note also that simply polling POLLIN on remotesock would return immediately
because mptcp_fastopen_subflow_synack_set_params() already queued the SYN data
onto sk_receive_queue prior to accept().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793860134.git.tanggeliang@kylinos.cn?part=3

Re: [PATCH mptcp-next v6 0/7] prepare interfaces for kTLS and sockmap support

[MPTCP CI]

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): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/41983965630

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/96ace799c7e3
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1202770


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)

Re: [PATCH mptcp-net v6 2/7] mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The author and the Signed-off-by line on commit "mptcp: fix TFO
  SIOCINQ overflow in mptcp_inq_hint" use different email addresses.

--

mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint

This commit changes mptcp_inq_hint() to compute the available bytes with
mptcp_data_avail() instead of ack_seq minus the head skb's map_seq. On
passive TFO connections, the old calculation could return INT_MAX until
the TFO skb was fully consumed. The EOF hint of returning 1 is kept.

> Fixes: f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Low]
This isn't a bug, but the Signed-off-by address doesn't match the author
address. The author is:

  Geliang Tang <geliang@kernel.org>

and the only sign-off is:

  Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

checkpatch reports this as FROM_SIGN_OFF_MISMATCH.

The previous patch in the series, "mptcp: update mptcp_check_readable",
is signed off with geliang@kernel.org. Could the sign-off here be changed
to match the author address, or could a matching Signed-off-by be added,
before this is applied?

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

Re: [PATCH mptcp-next v6 3/7] selftests: mptcp: connect: check TFO SIOCINQ overflow

[netdev-bot+sashiko]

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

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

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 for "selftests: mptcp: connect: check TFO
  SIOCINQ overflow" describes code that the patch does not contain.
- [Medium] selftests: mptcp: SIOCINQ check in main_loop_s() races with
  TFO third ACK

--

selftests: mptcp: connect: check TFO SIOCINQ overflow

This adds a check to the server side of mptcp_connect. When the MPTFO
sockopt is set, it reads ioctl(SIOCINQ) on the accepted socket and fails
the test if the value is INT_MAX. The aim is to catch mptcp_inq_hint()
returning INT_MAX for passive TFO connections.

> selftests: mptcp: connect: check TFO SIOCINQ overflow
>
> Add a check in mptcp_connect's server mode to detect TFO SIOCINQ overflow.
> When a TFO connection is accepted, poll for data to arrive, then check if
> SIOCINQ returns INT_MAX. This detects the regression where mptcp_inq_hint()
> incorrectly returns INT_MAX for passive TFO connections.
>
> The check runs when the MPTFO sockopt is set. It polls the accepted socket
> for up to one second, then uses ioctl(SIOCINQ) to verify the available
> byte count. If SIOCINQ returns INT_MAX, the test fails.

[Severity: Medium]
Does this description match the code? The commit message says the test
polls the accepted socket for up to one second before reading SIOCINQ.
The hunk below has no poll(), no timeout and no other wait.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..83db495704fec 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1207,6 +1207,17 @@ int main_loop_s(int listensock)
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
>
> +		if (cfg_sockopt_types.mptfo) {
> +			int avail;
> +
> +			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
> +			    avail == INT_MAX) {

[Severity: Medium]
Can this check miss the regression it is meant to catch? It reads
SIOCINQ once, right after accept(), and nothing guarantees that the
third ACK from the client has been processed at that point.

For a passive TFO connection, mptcp_sk_clone_init() creates the msk
from the SYN, where mp_opt->suboptions only has OPTION_MPTCP_MPC_SYN,
so msk->ack_seq stays 0. subflow_prep_synack() then calls
mptcp_fastopen_subflow_synack_set_params(), which queues the SYN
payload on sk_receive_queue with MPTCP_SKB_CB(skb)->map_seq set to
-skb->len. The child is put on the listener accept queue as soon as
the SYN-ACK goes out.

If accept() returns in that window, mptcp_inq_hint() on an unfixed
kernel computes 0 - (-skb->len), which is just skb->len, and the test
passes. The hint only reaches INT_MAX after the third ACK carrying
OPTION_MPTCP_MPC_ACK goes through check_fully_established() and
subflow_set_remote_key() sets msk->ack_seq to subflow->iasn. So
whether the test fails on a broken kernel depends on whether the
client ACK was processed before the server called accept().

The poll() described in the commit message would not close this
window either. The TFO data is already on sk_receive_queue before the
child is accepted, so POLLIN on remotesock is reported right away.

Would it work to wait for the msk to be fully established before
reading SIOCINQ, for example by polling getsockopt(MPTCP_INFO) until
MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED is set in mptcpi_flags? Or is
there a better way to make sure the third ACK has been seen before
the check runs?

In main_loop_s(), ioctl(remotesock, SIOCINQ, &avail) is called once,
right after accept(), check_sockaddr() and check_getpeername().

Later patches in the series do not touch mptcp_connect.c, so the
mismatch is still there at the end of the series.

As far as I can tell, the missing wait does not change the result. In
subflow_prep_synack(), mptcp_fastopen_subflow_synack_set_params() queues
the TFO skb on the msk before the child socket goes into the accept
queue. On an unfixed kernel, the old hint also comes out at INT_MAX or
more before the third ACK arrives. So only the description seems to be
off.

Could the commit message drop the poll description, or should the
described poll() be added to the check?

> +				fprintf(stderr, "TFO SIOCINQ overflow\n");
> +				close(remotesock);
> +				return 1;
> +			}
> +		}
> +

[ ... ]

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

Re: [PATCH mptcp-next v6 4/7] mptcp: implement peek_len for proto_ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "mptcp: implement peek_len for proto_ops"
  gets several of its TCP and mptcp_inq_hint() comparisons wrong.
- [Low] The commit message says the patch 'prepares for upcoming kTLS and
  sockmap support', but kTLS never calls sock->ops->peek_len.

--

mptcp: implement peek_len for proto_ops

This adds mptcp_inq(), which returns the readable bytes at the MPTCP level
based on mptcp_data_avail(). It is wired up through a new mptcp_peek_len()
callback, set as .peek_len in both mptcp_stream_ops and
mptcp_v6_stream_ops.

> Add mptcp_inq() to compute readable bytes at the MPTCP level using
> mptcp_data_avail() (bytes_received - bytes_consumed). This aligns with
> TCP's design where tcp_inq() is used by peek_len and tcp_inq_hint() is
> used by SIOCINQ.
>
> Like tcp_inq(), mptcp_inq() returns 0 during handshake (TCPF_SYN_SENT/
> TCPF_SYN_RECV), INT_MAX when avail >= INT_MAX, otherwise the actual byte
> count. Unlike mptcp_inq_hint() (which mirrors tcp_inq_hint()), this helper
> does not include the EOF hint (returning 1 when socket is closed with no
> data) because peek_len is used by upper layers like strp that only need
> the data length, not connection state information. It also does not drain
> the backlog before reading counters, as peek_len provides a heuristic
> value for rate limiting rather than an exact count for userspace queries.

[Severity: Low]
Are the comparisons to TCP and mptcp_inq_hint() here accurate?

On the TCP side, SIOCINQ in tcp_ioctl() calls tcp_inq(), which is the same
helper tcp_peek_len() uses:

net/ipv4/tcp.c:tcp_ioctl() {
    ...
		slow = lock_sock_fast(sk);
		answ = tcp_inq(sk);
		unlock_sock_fast(sk, slow);
    ...
}

int tcp_peek_len(struct socket *sock)
{
	return tcp_inq(sock->sk);
}

tcp_inq_hint() is only used for msg_inq / TCP_CM_INQ in tcp_recvmsg() and
for zc.inq in the zerocopy receive path. So TCP uses one helper for both
peek_len and SIOCINQ. With this patch, MPTCP uses mptcp_inq_hint() for
SIOCINQ and mptcp_inq() for peek_len, which does not match TCP.

Also, tcp_inq() has no INT_MAX clamp. It subtracts 1 when a FIN has been
received and handles urgent data, and mptcp_inq() does neither:

include/net/tcp.h:tcp_inq() {
    ...
		answ = tp->rcv_nxt - tp->copied_seq;

		/* Subtract 1, if FIN was received */
		if (answ && sock_flag(sk, SOCK_DONE))
			answ--;
	} else {
		answ = tp->urg_seq - tp->copied_seq;
	}
    ...
}

On the backlog point, mptcp_inq_hint() does not drain the backlog either.
That happens in its caller, mptcp_ioctl():

net/mptcp/protocol.c:mptcp_ioctl() {
    ...
		lock_sock(sk);
		if (mptcp_move_skbs(sk))
			mptcp_cleanup_rbuf(msk, 0);
		*karg = mptcp_inq_hint(sk);
    ...
}

Last, strparser does not seem to use peek_len for rate limiting.
strp_data_ready() and strp_recv() compare it against the bytes still
missing from the current message:

net/strparser/strparser.c:strp_data_ready() {
    ...
	if (strp->need_bytes) {
		if (strp_peek_len(strp) < strp->need_bytes)
			return;
	}
    ...
}

This checks whether the whole message is already queued, and decides
whether to set need_bytes and wait.

Could the commit message be reworded to describe these points accurately?

> This prepares for upcoming kTLS and sockmap support.

[Severity: Low]
Does this patch actually help kTLS?

The only caller of sock->ops->peek_len seems to be strp_peek_len() in
net/strparser/strparser.c, which sockmap and KCM use:

static inline int strp_peek_len(struct strparser *strp)
{
	struct socket *sock = strp->sk->sk_socket;

	return sock->ops->peek_len(sock);
}

The kTLS receive and device offload paths call tcp_inq() directly on the
socket, not through peek_len. For example:

net/tls/tls_strp.c:tls_strp_read_sock() {
    ...
	inq = tcp_inq(strp->sk);
	if (inq < 1)
		return 0;
    ...
}

There are similar direct tcp_inq() calls in tls_strp_check_queue_ok(),
tls_sw.c and tls_device.c. They are unchanged at the end of this series.

On an MPTCP msk, those tcp_inq() calls would read tcp_sock fields
(rcv_nxt, copied_seq, urg_data) from a struct mptcp_sock. Also,
mptcp_inq() is static, so later kTLS work cannot reuse it without further
changes.

Should the commit message mention only sockmap/strparser, since the new
.peek_len hook is not reached by kTLS?

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

Re: [PATCH mptcp-next v6 5/7] mptcp: add sendmsg_locked to proto_ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds `.sendmsg_locked = mptcp_sendmsg_locked` to
  `mptcp_stream_ops` and `mptcp_v6_stream_ops`, but nothing in the tree,…
- [Low] The commit author is "Geliang Tang <geliang@kernel.org>", but the
  only Signed-off-by is "Geliang Tang <tanggeliang@kylinos.cn>".

--

mptcp: add sendmsg_locked to proto_ops

This splits mptcp_sendmsg() into two functions. mptcp_sendmsg_locked()
expects the caller to already hold the msk socket lock, and a small
mptcp_sendmsg() wrapper takes the lock. The locked variant is exposed as
.sendmsg_locked in mptcp_stream_ops and mptcp_v6_stream_ops, as
preparation for kTLS and sockmap support.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Low]
This isn't a bug, but the commit author is
"Geliang Tang <geliang@kernel.org>" and the only Signed-off-by uses
<tanggeliang@kylinos.cn>. checkpatch reports this as
FROM_SIGN_OFF_MISMATCH.

Was an in-body From: line lost when the patch was applied? Or is a
sign-off from the author address missing?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index aed25c901e4b1..b9c7b0bb6f742 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4834,6 +4844,7 @@ static const struct proto_ops mptcp_stream_ops = {
>  	.read_sock	   = mptcp_read_sock,
>  	.splice_read	   = mptcp_splice_read,
>  	.peek_len	   = mptcp_peek_len,
> +	.sendmsg_locked	   = mptcp_sendmsg_locked,
>  };

[Severity: Low]
Can any path reach this op, or the same entry in mptcp_v6_stream_ops,
for an MPTCP socket?

The only generic user of proto_ops->sendmsg_locked is the static
sendmsg_locked() helper in net/core/skbuff.c. That helper is reached
only through skb_send_sock_locked() and
skb_send_sock_locked_with_flags(). Neither of their two callers can run
on an MPTCP socket:

- net/xfrm/espintcp.c is a TCP ULP, and mptcp_setsockopt_sol_tcp()
  rejects TCP_ULP:

	case TCP_ULP:
		return -EOPNOTSUPP;

- drivers/net/ovpn/tcp.c only attaches when
  sk_protocol == IPPROTO_TCP.

The commit message says this prepares for kTLS and sockmap. Neither of
them uses proto_ops->sendmsg_locked. tls_push_sg() in
net/tls/tls_main.c, tcp_bpf in net/ipv4/tcp_bpf.c, and espintcp all
call tcp_sendmsg_locked() directly:

	ret = tcp_sendmsg_locked(sk, &msg, size);

Would kTLS and sockmap each still need a separate refactor before this
hook is used? The later patches in the series ("mptcp: track
app-limited state in mptcp_sendmsg" and "selftests: mptcp: sockopt:
check app_limited") don't add a caller either. The op is still unused
at the end of the series.

The op also behaves differently from the TCP op it mirrors.
mptcp_sendmsg_locked() keeps the inherited flag mask:

	msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
			  MSG_FASTOPEN | MSG_EOR;

This mask drops MSG_SPLICE_PAGES. tcp_sendmsg_locked() honours that
flag, and the expected locked callers, __skb_send_sock() and
tls_push_sg(), set it. Data is still copied correctly.

Should the commit message mention this difference?

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

Re: [PATCH mptcp-next v6 6/7] mptcp: track app-limited state in mptcp_sendmsg

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit metadata does not match its sign-off.
- [Low] mptcp_rate_check_app_limited() (net/mptcp/protocol.c:2041-2063)
  runs on every mptcp_sendmsg_locked() call (line 2096).

--

mptcp: track app-limited state in mptcp_sendmsg

This adds mptcp_rate_check_app_limited(). It calls
tcp_rate_check_app_limited() on every subflow under that subflow's socket
lock, and skips this when the MPTCP socket still has unsent data or a
pending retransmission. mptcp_sendmsg_locked() calls it so that BBR and
similar congestion controllers can ignore rate samples taken while the
connection is application-limited.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Low]
This isn't a bug, but the Signed-off-by address doesn't match the author
address (geliang@kernel.org). The name is the same and only the address
differs, so checkpatch.pl reports FROM_SIGN_OFF_MISMATCH ("From:/Signed-off-by:
email address mismatch"). checkpatch.pl doesn't read .mailmap, so the
alias there doesn't stop the warning.

Another patch in the same series, "mptcp: update mptcp_check_readable",
uses the kernel.org address in its Signed-off-by.

Could the From: line or the Signed-off-by be changed so they match?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b9c7b0bb6f742..889f244d9da04 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2038,6 +2038,30 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
>  	}
>  }
>  
> +static void mptcp_rate_check_app_limited(struct sock *sk)
> +{

[ ... ]

> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		bool slow;
> +
> +		slow = lock_sock_fast_nested(ssk);
> +		tcp_rate_check_app_limited(ssk);
> +		unlock_sock_fast(ssk, slow);
> +	}
> +}

[Severity: Low]
How much does this cost on the send path? mptcp_sendmsg_locked() calls
mptcp_rate_check_app_limited() on every sendmsg(). Whenever the MPTCP
send queue is empty, it takes and releases the lock of every subflow on
msk->conn_list. An empty queue is the normal state for request/response
and small-write workloads.

On the TCP side, tcp_rate_check_app_limited() is a check without locking
on a socket the caller already owns. In MPTCP, __mptcp_push_pending()
then locks only the subflows that the scheduler picked:

net/mptcp/protocol.c:__mptcp_push_pending() {
    ...
		mptcp_for_each_subflow(msk, subflow) {
			if (READ_ONCE(subflow->scheduled)) {
				...
					lock_sock(ssk);
    ...
}

So each sendmsg() pays for N extra subflow lock round trips, on top of
the locking that transmission already needs. The loop also visits
subflows that can't send (closed, not yet established, or the listener
subflow). Marking those app-limited has no effect.

Could this check be done in the push path, where the ssk lock is already
held? Or could it be limited to subflows that can actually send? The
commit message mentions the per-subflow locking, but it gives no cost or
measurement.

[ ... ]

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

Re: [PATCH mptcp-next v6 7/7] selftests: mptcp: sockopt: check app_limited

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the new check "guards against regressions
  in subflow app-limited accounting", but the test covers only half of…

--

selftests: mptcp: sockopt: check app_limited

This commit adds an assertion to connect_one_server() in the mptcp_sockopt
selftest. After a small data exchange, it checks that
tcpi_delivery_rate_app_limited == 1. The check only runs when
expect_all_features() returns true.

> This guards against regressions in subflow app-limited accounting.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index b8158a1198eba..9517c530ba214 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -635,6 +635,11 @@ static void connect_one_server(int fd, int pipefd)
>  	do_getsockopts(&s, fd, total, total);
>  
>  	assert(s.mptcpi_rcv_delta == (uint64_t)total);
> +
> +	/* app-limited accounting only checked under EXPECT_ALL_FEATURES */
> +	if (expect_all_features())
> +		assert(s.tcp_info.tcpi_delivery_rate_app_limited == 1);

[Severity: Low]
This isn't a bug, but is the claim in the commit message broader than what
this assertion covers?

In connect_one_server(), the client does one write() of 128 to 4095 bytes,
with nothing else pending at the MPTCP level. That only exercises the path
in mptcp_rate_check_app_limited() where every subflow gets marked
app-limited. The early return is never exercised:

net/mptcp/protocol.c:mptcp_rate_check_app_limited() {
    ...
	if (msk->write_seq != msk->snd_nxt || mptcp_send_head(sk) ||
	    test_bit(MPTCP_WORK_RTX, &flags) ||
	    test_bit(MPTCP_RETRANSMIT, &cb_flags))
		return;
    ...
}

The parent commit, "mptcp: track app-limited state in mptcp_sendmsg", says
this bail-out is needed so congestion controllers are not misled.

Suppose a later change weakened those conditions so subflows were always
marked app-limited. tcp_init_sock() sets rate_app_limited to 1, and
app-limited samples keep it at 1. The assertion would still pass, so that
regression would go unnoticed.

The test does catch the opposite case, where subflows are never marked
app-limited. Without the parent commit, rate_app_limited drops to 0 here.

The check is also gated on SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES=1.
mptcp_lib.sh reads that variable but never sets it, so a default kselftest
run doesn't check anything new. The code comment mentions this gate, but
the commit message doesn't.

Could the commit message describe the narrower coverage and the opt-in
gate? Alternatively, could a case with data still pending be added, so
the early return in mptcp_rate_check_app_limited() gets tested too?

> +
>  	close(fd);
>  }

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