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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does not prevent the dummy DATA_FIN in the common RST
  case.
- [Medium] subflow_is_done() now returns false whenever ssk->sk_err is
  non-zero, even when the subflow got a clean FIN (TCP_CLOSE_WAIT /…
- [Low] The From: and Signed-off-by: email addresses do not match.

--

mptcp: prevent dummy DATA_FIN on RST in fallback mode

This patch adds an sk_err check to subflow_is_done(). The goal is that
subflow_sched_work_if_closed() no longer creates a dummy DATA_FIN when a
fallback subflow is closed by a TCP RST, because a RST is an abortive close
and not a clean FIN.

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

[Severity: Low]
This isn't a bug, but the Author address (geliang@kernel.org) doesn't match
the Signed-off-by address (tanggeliang@kylinos.cn). checkpatch.pl reports
this as a From:/Signed-off-by: email address mismatch.

Could the two addresses be made to match, or could a Signed-off-by for the
author address be added?

> 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: High]
In the common RST case, is sk_err still set when the last
subflow_sched_work_if_closed() call runs?

sock_error() reads the error and clears it:

include/net/sock.h:sock_error() {
    ...
	err = xchg(&sk->sk_err, 0);
    ...
}

On a RST, tcp_done_with_error() sets ssk->sk_err. tcp_done() then moves the
subflow to TCP_CLOSE and calls 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);
}

If the rx queue is empty, mptcp_subflow_data_available() takes the no_data
path. That path calls subflow_sched_work_if_closed() once while sk_err is
still set, so the new check returns false there.

Next, subflow_error_report() runs. When msk->sk_socket is set and the msk
is not owned by user, the call chain is:

subflow_error_report()
  __mptcp_error_report()
    __mptcp_subflow_error_report()
      err = sock_error(ssk);          /* clears ssk->sk_err */
      mptcp_set_state(sk, ssk_state); /* msk -> TCP_CLOSE */

On a fallback msk this clears ssk->sk_err. The unconditional
subflow_sched_work_if_closed() call at the end of subflow_state_change()
then sees sk_err == 0 and sk_state == TCP_CLOSE. subflow_is_done() returns
true, and mptcp_update_rcv_data_fin() records the dummy DATA_FIN anyway.

When the rx queue is not empty, the result looks the same.
mptcp_data_ready()->move_skbs_to_msk() also clears the error:

	if (unlikely(READ_ONCE(ssk->sk_err)))
		__mptcp_subflow_error_report(sk, ssk);

Any later no_data->subflow_sched_work_if_closed() call then creates the
DATA_FIN.

The suppression seems to work only while the error is still pending. That
is the case when the msk is owned by user and the report is deferred via
MPTCP_ERROR_REPORT, or when msk->sk_socket is NULL. So whether a RST
produces a dummy DATA_FIN depends on msk lock ownership at the time the RST
arrives.

Would it be more reliable to record the abort in state that survives error
consumption, such as a subflow flag set at reset time, instead of relying
on sk_err?

[Severity: Medium]
Could this early return also suppress the legitimate dummy DATA_FIN after
a clean FIN, when an unrelated error is still pending on the subflow?

The new check returns false for any non-zero sk_err, even when RCV_SHUTDOWN
is set. Consider a fallback msk with IP_RECVERR set, which
mptcp_setsockopt_recverr() propagates to the subflows:

1. An ICMP error arrives on the established fallback subflow. tcp_v4_err()
   sets ssk->sk_err and calls subflow_error_report().

2. The msk is owned by user, so the report is deferred:

	if (!sock_owned_by_user(sk))
		__mptcp_error_report(sk);
	else
		__set_bit(MPTCP_ERROR_REPORT,  &mptcp_sk(sk)->cb_flags);

   The same happens if msk->sk_socket is NULL, because
   subflow_error_report() returns early. Either way ssk->sk_err stays set.

3. The peer's FIN arrives before release_sock(). The subflow moves to
   TCP_CLOSE_WAIT while the msk is still ESTABLISHED.
   subflow_sched_work_if_closed() passes the state and queue checks, but
   subflow_is_done() now returns false, so no dummy DATA_FIN is recorded.

4. Later, mptcp_release_cb() calls __mptcp_error_report(), which clears
   sk_err. Because ssk_state is TCP_CLOSE_WAIT, the msk state is left
   unchanged. Nothing calls subflow_sched_work_if_closed() again.

The worker doesn't seem to recover from this either, because
__mptcp_close_subflow() skips CLOSE_WAIT subflows on fallback sockets:

		if (ssk_state != TCP_CLOSE &&
		    (ssk_state != TCP_CLOSE_WAIT ||
		     inet_sk_state_load(sk) != TCP_ESTABLISHED ||
		     __mptcp_check_fallback(msk)))
			continue;

Would the msk then never get RCV_SHUTDOWN? That would leave recvmsg() and
poll() waiting for an EOF that has already arrived. Before this patch,
step 3 always recorded the dummy DATA_FIN.

>  	return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
>  }
>

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