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

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

[Geliang Tang]

From: Geliang Tang <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 (5):
  mptcp: prevent dummy DATA_FIN on RST in fallback mode
  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                          | 86 +++++++++++++++++--
 net/mptcp/subflow.c                           |  3 +
 .../selftests/net/mptcp/mptcp_sockopt.c       |  5 ++
 3 files changed, 86 insertions(+), 8 deletions(-)

-- 
2.55.0

[PATCH mptcp-next 1/6] 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-next 2/6] mptcp: prevent dummy DATA_FIN on RST in fallback mode

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

When a fallback subflow receives a TCP RST, subflow_is_done() returns true
(because sk_state == TCP_CLOSE), causing subflow_sched_work_if_closed()
to create a dummy DATA_FIN. This is semantically incorrect - RST is an
abortive close, not a clean close with FIN.

While mptcp_recvmsg() checks sock_error() before RCV_SHUTDOWN (so the
error is correctly returned to userspace), creating a dummy DATA_FIN for
RST causes unnecessary state transitions and semantic confusion.

Add a check for sk_err in subflow_is_done() to prevent dummy DATA_FIN
creation when the subflow was aborted with an error.

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

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..a53c83644a40 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1327,6 +1327,9 @@ static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb,
 
 static bool subflow_is_done(const struct sock *sk)
 {
+	if (READ_ONCE(sk->sk_err))
+		return false;
+
 	return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
 }
 
-- 
2.55.0

[PATCH mptcp-next 3/6] mptcp: implement peek_len for proto_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add mptcp_inq() to compute the number of readable bytes at the MPTCP
level. It derives the count from ack_seq - copied_seq, returns 0 while
the connection is still handshaking (TCP_SYN_SENT/TCP_SYN_RECV), returns
0 when the connection is closed (TCP_CLOSE or RCV_SHUTDOWN), and
subtracts 1 once a FIN has been received, mirroring tcp_inq().

READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
ack_seq and copied_seq because mptcp_inq() will be called from TLS
contexts with the socket lock held - taking the spinlock would deadlock.

On 32-bit systems a torn read can produce a wrapped subtraction (close to
2^64). When hint_val >= INT_MAX, we return 0 instead of INT_MAX to avoid
reporting 2GB of readable data when there may be none. This is likely
caused by a torn read rather than a legitimate large receive window, as
normal receive queues should not exceed INT_MAX bytes.

Wire mptcp_inq() into mptcp_peek_len() and assign .peek_len in both
mptcp_stream_ops and mptcp_v6_stream_ops. This is preparatory work for
an upcoming kTLS-over-MPTCP series.

Note: mptcp_inq() intentionally differs from mptcp_inq_hint() when the
calculated value is 0. mptcp_inq_hint() returns 1 to signal EOF to
ioctl/cmsg users (mirroring tcp_inq_hint()), while mptcp_inq() returns
0 because peek_len should report the actual readable byte count
(mirroring tcp_inq()).

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 | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0caa31525b90..71a0a28b0d3c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -30,6 +30,7 @@
 #include "mib.h"
 
 static unsigned int mptcp_inq_hint(const struct sock *sk);
+static int mptcp_inq(struct sock *sk);
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/mptcp.h>
@@ -4188,7 +4189,7 @@ static int mptcp_ioctl(struct sock *sk, int cmd, int *karg)
 		lock_sock(sk);
 		if (mptcp_move_skbs(sk))
 			mptcp_cleanup_rbuf(msk, 0);
-		*karg = mptcp_inq_hint(sk);
+		*karg = mptcp_inq(sk);
 		release_sock(sk);
 		break;
 	case SIOCOUTQ:
@@ -4797,6 +4798,39 @@ 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);
+	const struct sk_buff *skb;
+	u64 hint_val;
+	int answ;
+
+	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
+		return 0;
+
+	skb = skb_peek(&sk->sk_receive_queue);
+	if (!skb)
+		return 0;
+
+	hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
+	if (hint_val >= INT_MAX)
+		return 0;
+
+	answ = (unsigned int)hint_val;
+
+	/* Subtract 1, if FIN was received. Mirror tcp_inq() */
+	if (answ &&
+	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
+		answ--;
+
+	return answ;
+}
+
+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,
@@ -4819,6 +4853,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 = {
@@ -4931,6 +4966,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 4/6] 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.

Expose .sendmsg_locked in both mptcp_stream_ops and mptcp_v6_stream_ops.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 71a0a28b0d3c..8aad55e0cce7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2039,7 +2039,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;
@@ -2051,8 +2051,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	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) ||
@@ -2168,7 +2166,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	}
 
 out:
-	release_sock(sk);
 	return copied;
 
 do_error:
@@ -2181,6 +2178,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)
@@ -4854,6 +4862,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 = {
@@ -4967,6 +4976,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 5/6] 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 of the
mptcp_sock and runs tcp_rate_check_app_limited() under each subflow's
socket lock. The helper bails out early when MPTCP still has unsent data:

 - msk->write_seq != msk->snd_nxt: new data not yet pushed
 - mptcp_send_head(sk) != NULL: re-injected data from rtx queue during
   failover or stale-subflow recovery

In these cases the subflows may look idle only because data has not been
pushed down yet, and marking them app-limited would be wrong.

Invoke mptcp_rate_check_app_limited() from mptcp_sendmsg_locked() right
after the send-side setup, so the delivery-rate app_limited state stays
in sync with what the application actually has to send across each
subflow.

With this in place, subflow rate samples taken while the MPTCP connection
is genuinely application-limited are flagged as such, allowing congestion
controllers like BBR to ignore them when estimating bandwidth.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8aad55e0cce7..568b6b52734b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2039,6 +2039,24 @@ 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;
+
+	if (msk->write_seq != msk->snd_nxt || mptcp_send_head(sk))
+		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);
@@ -2067,6 +2085,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 6/6] selftests: mptcp: sockopt: check app_limited

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

connect_one_server() in mptcp_sockopt exchanges only a few packets between
the client and server, then closes the socket. After such a small transfer
the application has nothing further to send, so the connection is, by
definition, application-limited.

Extend the TCP_INFO readback at the end of the function to assert
s.tcp_info.tcpi_delivery_rate_app_limited == 1.

The rate_app_limited field is initialized to 1 in tcp_init_sock() and is
cleared to 0 only by valid non-app-limited data samples. Without the
preceding commit, mptcp_sendmsg() never updates the per-subflow
app-limited state. In lossless veth environments, valid samples typically
clear the flag during data transfer, so the assertion would fail.

With the preceding commit, mptcp_rate_check_app_limited() re-sets
rate_app_limited to 1 when the MPTCP connection is application-limited,
so the assertion passes. This turns the test into a regression guard for
the subflow-side application-limited accounting.

Note: This assertion alone does not prove the new code is working, since
rate_app_limited starts at 1. However, in practice, valid samples clear
it during transfer, so seeing 1 afterward indicates our code re-set it.

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..f7a45b93cbe0 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 requires a recent kernel */
+	if (expect_all_features())
+		assert(s.tcp_info.tcpi_delivery_rate_app_limited == 1);
+
 	close(fd);
 }
 
-- 
2.55.0

Re: [PATCH mptcp-next 0/6] 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/40289414299

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


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-next 2/6] mptcp: prevent dummy DATA_FIN on RST in fallback mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: subflow_is_done() checks sk_err after sock_error() clears it
--

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

mptcp: prevent dummy DATA_FIN on RST in fallback mode

This commit checks sk->sk_err in subflow_is_done() to prevent creating a
dummy DATA_FIN when a fallback subflow is aborted with a TCP RST.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..a53c83644a40b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1327,6 +1327,9 @@ static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb,
>  
>  static bool subflow_is_done(const struct sock *sk)
>  {
> +	if (READ_ONCE(sk->sk_err))
> +		return false;

[Severity: Medium]
Can sk->sk_err already be cleared to 0 before subflow_sched_work_if_closed()
checks subflow_is_done()?

When a fallback subflow receives a TCP RST in softirq, tcp_reset() ->
tcp_done_with_error() sets sk->sk_err and invokes subflow_state_change():

net/mptcp/subflow.c:subflow_state_change() {
	if (mptcp_subflow_data_available(sk))
		mptcp_data_ready(parent, sk);
	else if (unlikely(READ_ONCE(sk->sk_err)))
		subflow_error_report(sk);

	subflow_sched_work_if_closed(mptcp_sk(parent), sk);
}

When the parent MPTCP socket is not owned by userspace
(!sock_owned_by_user(parent)), both subflow_error_report() (via
__mptcp_error_report()) and mptcp_data_ready() (via move_skbs_to_msk() after
draining the subflow receive queue) immediately call
__mptcp_subflow_error_report(parent, ssk):

net/mptcp/protocol.c:__mptcp_subflow_error_report() {
	if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
		goto out;

	err = sock_error(ssk);
	if (!err)
		goto out;
}

Because sock_error(ssk) executes xchg(&ssk->sk_err, 0), ssk->sk_err is
atomically reset to 0 while propagating the error to parent->sk_err.

When subflow_state_change() subsequently calls
subflow_sched_work_if_closed(mptcp_sk(parent), sk), won't subflow_is_done(ssk)
observe READ_ONCE(sk->sk_err) == 0, return true because RCV_SHUTDOWN and
TCP_CLOSE are set, and still generate the dummy DATA_FIN via
mptcp_update_rcv_data_fin()?

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

Re: [PATCH mptcp-next 3/6] mptcp: implement peek_len for proto_ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq() under-reports readable bytes by 1 without DATA_FIN
- [Medium] mptcp: mptcp_inq() returns bogus byte count for TFO data after 3rd ACK
- [Medium] mptcp: mptcp_inq() and mptcp_ioctl() mismatch commit message
--

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

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() and mptcp_peek_len() to report readable bytes on
an MPTCP socket, wires .peek_len into mptcp_stream_ops and
mptcp_v6_stream_ops, and switches SIOCINQ in mptcp_ioctl() to use mptcp_inq().

>     mptcp: implement peek_len for proto_ops
>     
>     Add mptcp_inq() to compute the number of readable bytes at the MPTCP
>     level. It derives the count from ack_seq - copied_seq, returns 0 while
>     the connection is still handshaking (TCP_SYN_SENT/TCP_SYN_RECV), returns
>     0 when the connection is closed (TCP_CLOSE or RCV_SHUTDOWN), and
>     subtracts 1 once a FIN has been received, mirroring tcp_inq().
>     
>     READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
>     ack_seq and copied_seq because mptcp_inq() will be called from TLS
>     contexts with the socket lock held - taking the spinlock would deadlock.
>     
>     On 32-bit systems a torn read can produce a wrapped subtraction (close to
>     2^64). When hint_val >= INT_MAX, we return 0 instead of INT_MAX to avoid
>     reporting 2GB of readable data when there may be none. This is likely
>     caused by a torn read rather than a legitimate large receive window, as
>     normal receive queues should not exceed INT_MAX bytes.
>     
>     Wire mptcp_inq() into mptcp_peek_len() and assign .peek_len in both
>     mptcp_stream_ops and mptcp_v6_stream_ops. This is preparatory work for
>     an upcoming kTLS-over-MPTCP series.
>     
>     Note: mptcp_inq() intentionally differs from mptcp_inq_hint() when the
>     calculated value is 0. mptcp_inq_hint() returns 1 to signal EOF to
>     ioctl/cmsg users (mirroring tcp_inq_hint()), while mptcp_inq() returns
>     0 because peek_len should report the actual readable byte count
>     (mirroring tcp_inq()).
>     
>     Co-developed-by: Zqiang <qiang.zhang@linux.dev>
>     Signed-off-by: Zqiang <qiang.zhang@linux.dev>
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0caa31525b907..71a0a28b0d3c9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4188,7 +4189,7 @@ static int mptcp_ioctl(struct sock *sk, int cmd, int *karg)
>  		lock_sock(sk);
>  		if (mptcp_move_skbs(sk))
>  			mptcp_cleanup_rbuf(msk, 0);
> -		*karg = mptcp_inq_hint(sk);
> +		*karg = mptcp_inq(sk);
>  		release_sock(sk);
>  		break;
>  	case SIOCOUTQ:

[Severity: Medium]
Does this change in mptcp_ioctl() contradict the commit message?

The commit message notes that mptcp_inq_hint() returns 1 to signal EOF to
ioctl/cmsg users, while mptcp_inq() returns 0 for peek_len. However, switching
SIOCINQ in mptcp_ioctl() from mptcp_inq_hint(sk) to mptcp_inq(sk) causes
ioctl(SIOCINQ) to return 0 instead of 1 at EOF when sk->sk_receive_queue is
empty and TCP_CLOSE or RCV_SHUTDOWN is set.

In addition, the commit message describes mptcp_inq() as reading a scalar
copied_seq with READ_ONCE(), returning 0 when TCP_CLOSE or RCV_SHUTDOWN is
set, and avoiding mptcp_data_lock() because taking it under the socket lock
would deadlock. In the implementation below, struct mptcp_sock has no
copied_seq field; mptcp_inq() instead peeks sk->sk_receive_queue and reads
MPTCP_SKB_CB(skb)->map_seq without READ_ONCE(), decrements answ rather than
returning 0 on TCP_CLOSE or RCV_SHUTDOWN when an skb is queued, and
mptcp_move_skbs() right above takes mptcp_data_lock() while holding
lock_sock(sk).

Should SIOCINQ in mptcp_ioctl() keep calling mptcp_inq_hint(sk), and could the
commit message be updated to match the implementation?

> @@ -4797,6 +4798,39 @@ 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);
> +	const struct sk_buff *skb;
> +	u64 hint_val;
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return 0;
> +
> +	skb = skb_peek(&sk->sk_receive_queue);
> +	if (!skb)
> +		return 0;
> +
> +	hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	if (hint_val >= INT_MAX)
> +		return 0;

[Severity: Medium]
Can this sequence subtraction return 0 (or a bogus byte count) when unread TCP
Fast Open data is still at the head of sk->sk_receive_queue after the 3rd ACK
arrives?

On a passive MPTCP Fast Open connection,
mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c queues the
SYN data skb onto sk->sk_receive_queue while msk->ack_seq is 0:

    /* Only the sequence delta is relevant */
    MPTCP_SKB_CB(skb)->map_seq = -skb->len;
    MPTCP_SKB_CB(skb)->end_seq = 0;
    MPTCP_SKB_CB(skb)->offset = 0;

Commit f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()") removed
the helper that rebased the queued TFO skb's map_seq when the 3rd ACK arrives.
When the 3rd ACK carrying MP_CAPABLE is processed, subflow_set_remote_key() in
net/mptcp/subflow.c sets msk->ack_seq to the 64-bit initial data sequence
number:

    WRITE_ONCE(msk->remote_key, subflow->remote_key);
    WRITE_ONCE(msk->ack_seq, subflow->iasn);

If the TFO skb at the head of sk->sk_receive_queue has not yet been fully
consumed when mptcp_inq() runs, READ_ONCE(msk->ack_seq) -
MPTCP_SKB_CB(skb)->map_seq evaluates to subflow->iasn + remaining_tfo_len
(plus any subsequent data). Because subflow->iasn is a 64-bit hash of the
remote key, hint_val will almost always be >= INT_MAX and cause mptcp_inq() to
return 0 (or a bogus value if it wraps below INT_MAX) even though unread TFO
data is queued.

Would using mptcp_data_avail(msk), i.e. READ_ONCE(msk->bytes_received) -
READ_ONCE(msk->bytes_consumed), avoid this issue?

> +
> +	answ = (unsigned int)hint_val;
> +
> +	/* Subtract 1, if FIN was received. Mirror tcp_inq() */
> +	if (answ &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> +		answ--;
> +
> +	return answ;
> +}

[Severity: Medium]
Won't subtracting 1 whenever sk->sk_state == TCP_CLOSE or RCV_SHUTDOWN is set
under-report the readable byte count by 1 whenever the socket enters those
states without processing an in-sequence DATA_FIN?

In TCP, tcp_inq() decrements answ only when sock_flag(sk, SOCK_DONE) is set,
which happens in tcp_fin() when an in-sequence FIN has been received and
tp->rcv_nxt has been incremented by 1.

In MPTCP, payload skbs queued onto sk->sk_receive_queue advance msk->ack_seq
by the exact payload length, and the only place that increments msk->ack_seq
by an extra 1 for DATA_FIN is mptcp_check_data_fin() in net/mptcp/protocol.c:

    if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
        WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
        WRITE_ONCE(msk->rcv_data_fin, 0);

        WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);

However, sk->sk_state == TCP_CLOSE or (sk->sk_shutdown & RCV_SHUTDOWN) can be
set while unread data remains in sk->sk_receive_queue without
mptcp_check_data_fin() ever incrementing msk->ack_seq:

1. A local user calls shutdown(fd, SHUT_RD) or shutdown(fd, SHUT_RDWR), where
   inet_shutdown() in net/ipv4/af_inet.c sets RCV_SHUTDOWN on sk->sk_shutdown
   without modifying msk->ack_seq or purging sk->sk_receive_queue:

    default:
        WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | how);
        if (sk->sk_prot->shutdown)
            sk->sk_prot->shutdown(sk, how);
        break;

2. The peer aborts the connection with MP_FASTCLOSE, where
   mptcp_check_fastclose() in net/mptcp/protocol.c sets TCP_CLOSE and
   SHUTDOWN_MASK without incrementing msk->ack_seq:

    mptcp_set_state(sk, TCP_CLOSE);
    WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);

3. A fallback subflow receives a TCP RST after data, where
   __mptcp_subflow_error_report() in net/mptcp/protocol.c transitions the
   MPTCP socket to TCP_CLOSE without incrementing msk->ack_seq (and commit
   f3589041bbf5, "mptcp: prevent dummy DATA_FIN on RST in fallback mode",
   explicitly avoided synthesizing a dummy DATA_FIN on RST):

    ssk_state = inet_sk_state_load(ssk);
    if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
        mptcp_set_state(sk, ssk_state);

4. All subflows close without DATA_FIN or time out in __mptcp_close_ssk() /
   mptcp_do_fastclose(), setting sk->sk_state to TCP_CLOSE without
   incrementing msk->ack_seq.

In all of these cases, READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq is
the exact number of unread payload bytes N in sk->sk_receive_queue, so
decrementing answ causes mptcp_inq(), mptcp_peek_len(), and
mptcp_ioctl(SIOCINQ) to return N - 1 (and 0 when 1 byte of unread data
remains), which can also stall strp_data_ready() when waiting for
strp_peek_len() >= strp->need_bytes.

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

Re: [PATCH mptcp-next 3/6] mptcp: implement peek_len for proto_ops

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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 patch changes the result of the SIOCINQ/FIONREAD ioctl on
  MPTCP sockets and does not say so.
- [Medium] mptcp_inq() (net/mptcp/protocol.c:4821-4824) subtracts 1 from
  the readable byte count whenever `sk->sk_state == TCP_CLOSE ||…
- [Medium] mptcp: mptcp_inq() returns bogus byte count for TFO data after
  3rd ACK
- [Low] The commit message describes mptcp_inq() differently from the
  code.

--

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq(), which computes the number of readable bytes
at the MPTCP level. It wires mptcp_inq() into a new mptcp_peek_len()
callback for both the IPv4 and IPv6 stream proto_ops, as preparation for
kTLS over MPTCP. It also switches the SIOCINQ ioctl path from
mptcp_inq_hint() to mptcp_inq().

> Add mptcp_inq() to compute the number of readable bytes at the MPTCP
> level. It derives the count from ack_seq - copied_seq, returns 0 while
> the connection is still handshaking (TCP_SYN_SENT/TCP_SYN_RECV), returns
> 0 when the connection is closed (TCP_CLOSE or RCV_SHUTDOWN), and
> subtracts 1 once a FIN has been received, mirroring tcp_inq().
>
> READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
> ack_seq and copied_seq because mptcp_inq() will be called from TLS
> contexts with the socket lock held - taking the spinlock would deadlock.

[Severity: Low]
Does this description match the code?

struct mptcp_sock has no copied_seq field. mptcp_inq() actually computes:

    hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

This uses the map_seq of the head skb in sk_receive_queue.

Also, mptcp_inq() never returns 0 because of TCP_CLOSE or RCV_SHUTDOWN. When
data is queued it decrements answ and returns the remaining count. It only
returns 0 for SYN_SENT/SYN_RECV, an empty receive queue, or
hint_val >= INT_MAX.

As written, the message describes two different behaviours for a closed
connection ("returns 0" and "subtracts 1"), and only the second one exists.
Could the description be updated to match the code?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0caa31525b907..71a0a28b0d3c9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4188,7 +4189,7 @@ static int mptcp_ioctl(struct sock *sk, int cmd, int *karg)
>  		lock_sock(sk);
>  		if (mptcp_move_skbs(sk))
>  			mptcp_cleanup_rbuf(msk, 0);
> -		*karg = mptcp_inq_hint(sk);
> +		*karg = mptcp_inq(sk);

[Severity: Medium]
Is this change to SIOCINQ/FIONREAD intended? The subject and body only
describe a new peek_len callback.

The note at the end of the commit message says:

    mptcp_inq_hint() returns 1 to signal EOF to ioctl/cmsg users
    (mirroring tcp_inq_hint())

After this hunk, ioctl users no longer go through mptcp_inq_hint().

Compared with the old mptcp_inq_hint(), ioctl(FIONREAD) on an MPTCP socket
now returns:

- 0 instead of 1 for an empty receive queue with TCP_CLOSE or
  RCV_SHUTDOWN set
- N instead of N+1 for queued data after a DATA_FIN, because
  mptcp_check_data_fin() advanced ack_seq by one
- N-1 instead of N when RCV_SHUTDOWN or TCP_CLOSE is set without a
  DATA_FIN (see the comment on mptcp_inq() below)
- 0 instead of INT_MAX when hint_val >= INT_MAX

Some of this brings MPTCP closer to tcp_ioctl(), which uses tcp_inq(), but
userspace can see the difference. Should this go in a separate patch, with
a commit message that describes the new ioctl behaviour and perhaps a
Fixes: tag?

No later patch in the series touches this code.

>  		release_sock(sk);
>  		break;
>  	case SIOCOUTQ:
> @@ -4797,6 +4798,39 @@ 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);
> +	const struct sk_buff *skb;
> +	u64 hint_val;
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return 0;
> +
> +	skb = skb_peek(&sk->sk_receive_queue);
> +	if (!skb)
> +		return 0;
> +
> +	hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	if (hint_val >= INT_MAX)
> +		return 0;
> +
> +	answ = (unsigned int)hint_val;
> +
> +	/* Subtract 1, if FIN was received. Mirror tcp_inq() */
> +	if (answ &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> +		answ--;

[Severity: Medium]
Can this return 0 while passive TFO data is still queued?

On the passive side, mptcp_fastopen_subflow_synack_set_params() queues
the SYN data skb on sk_receive_queue with a sequence that is relative
to an ack_seq of 0:

    MPTCP_SKB_CB(skb)->map_seq = -skb->len;
    MPTCP_SKB_CB(skb)->end_seq = 0;

When the third ACK carrying MP_CAPABLE arrives,
subflow_set_remote_key() does:

    WRITE_ONCE(msk->ack_seq, subflow->iasn);

Since commit f03afb3aeb9d ("mptcp: drop
__mptcp_fastopen_gen_msk_ackseq()"), nothing rebases the map_seq of an
skb that is already in the receive queue.

Suppose userspace has not consumed the SYN data by then, or strparser
has not, once kTLS uses this. The subtraction in mptcp_inq() then
yields roughly subflow->iasn plus the remaining TFO length. iasn is
derived from a hash of the remote key, so the result is almost always
>= INT_MAX, and mptcp_inq() returns 0 even though bytes are readable.
In the rare case where the value wraps below INT_MAX, it returns a
bogus positive count instead.

The commit message treats hint_val >= INT_MAX as a torn read on 32-bit.
Here, though, it is a stable state that holds until the TFO skb is
drained.

The old mptcp_inq_hint() used the same arithmetic but returned INT_MAX
in this case, which at least did not hide the data. With the ioctl hunk
in this patch, FIONREAD now reports 0. For peek_len, a 0 return would
leave a strparser waiting on need_bytes for data that is already
queued.

Would it be better to derive the count from mptcp_data_avail()? It uses
bytes_received - bytes_consumed and does not depend on the map_seq of
the head skb. Alternatively, should the queued TFO skb be rebased when
ack_seq is set from iasn?

[Severity: Medium]
Can this under-report the readable byte count by one?

msk->ack_seq only holds the extra unit after mptcp_check_data_fin() has
processed a DATA_FIN:

    if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
        WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
        ...
        WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);

Several other paths set TCP_CLOSE or RCV_SHUTDOWN without touching ack_seq,
while in-order data stays in sk_receive_queue:

- shutdown(fd, SHUT_RD): the .shutdown handler is inet_shutdown(), which
  sets RCV_SHUTDOWN, and mptcp_shutdown() only acts on SEND_SHUTDOWN.

- A received MP_FASTCLOSE: mptcp_check_fastclose() does

    mptcp_set_state(sk, TCP_CLOSE);
    WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);

  and neither changes ack_seq nor purges the queue.

- A reset or error on a fallback subflow: __mptcp_subflow_error_report()
  moves the msk to TCP_CLOSE. With the earlier "mptcp: prevent dummy
  DATA_FIN on RST in fallback mode" patch in this series, no dummy DATA_FIN
  bumps ack_seq on this path.

In these cases ack_seq - map_seq is exactly the queued length N, but
mptcp_inq() returns N-1. With a single byte queued it returns 0.
mptcp_recvmsg() still drains all N bytes before it checks sk_err,
RCV_SHUTDOWN or TCP_CLOSE.

mptcp_ioctl() now calls mptcp_inq(), so FIONREAD reports N-1 where
mptcp_inq_hint() used to return N. For the new peek_len, strp_data_ready()
compares strp_peek_len() against need_bytes, so a complete final message
could be treated as incomplete.

The comment says this mirrors tcp_inq(), but tcp_inq() only decrements when
SOCK_DONE is set:

    if (answ && sock_flag(sk, SOCK_DONE))
        answ--;

A processed DATA_FIN always sets RCV_SHUTDOWN, so the TCP_CLOSE check can
only cause undercounts.

Should the decrement depend on whether ack_seq actually includes the
DATA_FIN, for example a condition derived from rcv_data_fin_seq, rather
than on TCP_CLOSE or RCV_SHUTDOWN?

This code is unchanged at the end of the series.

> +
> +	return answ;
> +}

[ ... ]

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