[PATCH resend mptcp-net 0/4] mptcp: a few fixes

[PATCH resend mptcp-net 0/4] mptcp: a few fixes

[Paolo Abeni]

This is mostly a follow-up to the recent OoO queue pruning series.
Explicitly targeting net as we have already a lot of patches pending for
net-next, and no need to rush IMHO.

First 2 patches addresses explicit comments from sashiko, 3rd one is a
somewhat unrelated cleanup I stumbled upon while implementing patch 4.

The last patch fixes another thing implided by sashiko while reviewing
the mentioned series.
---
resent to hopefully trigger sashiko


Paolo Abeni (4):
  mptcp: being below memory limit is a likely() condition
  mptcp: avoid pruning for OoW data
  mptcp: remove unneeded READ_ONCE() annotation
  mptcp: do not reschedule the RTX timer for fallback sockets

 net/mptcp/options.c  |  4 ++--
 net/mptcp/protocol.c | 29 +++++++++++++++++------------
 net/mptcp/protocol.h |  1 +
 3 files changed, 20 insertions(+), 14 deletions(-)

-- 
2.55.0

[PATCH resend mptcp-net 1/4] mptcp: being below memory limit is a likely() condition

[Paolo Abeni]

The current compiler hint annotation is wrong, due to inverted
logic in the previous revision of the relevant code.

Fixes: e468d371180d ("mptcp: implemented OoO queue pruning")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c2762d74f29d..f879b1061f2d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -289,8 +289,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
  */
 static bool mptcp_can_ingest(const struct sock *sk)
 {
-	return unlikely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
-			__mptcp_check_fallback(mptcp_sk(sk));
+	return likely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
+	       __mptcp_check_fallback(mptcp_sk(sk));
 }
 
 static bool mptcp_try_rmem_schedule(struct sock *sk, const struct sk_buff *skb)
-- 
2.55.0

[PATCH resend mptcp-net 2/4] mptcp: avoid pruning for OoW data

[Paolo Abeni]

Pruning is expansive and destructive, do it only when we expect
to accept the skb triggering the cleanup.

Fixes: e468d371180d ("mptcp: implemented OoO queue pruning")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f879b1061f2d..2ee23e9411be 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -313,12 +313,6 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	u64 seq, end_seq, max_seq;
 	struct sk_buff *skb1;
 
-	if (!mptcp_try_rmem_schedule(sk, skb)) {
-		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
-		mptcp_drop(sk, skb);
-		return;
-	}
-
 	seq = MPTCP_SKB_CB(skb)->map_seq;
 	end_seq = MPTCP_SKB_CB(skb)->end_seq;
 	max_seq = atomic64_read(&msk->rcv_wnd_sent);
@@ -335,6 +329,12 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 		return;
 	}
 
+	if (!mptcp_try_rmem_schedule(sk, skb)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
+		mptcp_drop(sk, skb);
+		return;
+	}
+
 	p = &msk->out_of_order_queue.rb_node;
 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUE);
 	if (RB_EMPTY_ROOT(&msk->out_of_order_queue)) {
-- 
2.55.0

[PATCH resend mptcp-net 3/4] mptcp: remove unneeded READ_ONCE() annotation

[Paolo Abeni]

The subflow->fully_established flag is always written under the subflow
socket lock. Reading such value under the same lock does not require any
ONCE annotation.

Fixes: 581c8cbfa934 ("mptcp: annotate data-races around subflow->fully_established")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/options.c  | 4 ++--
 net/mptcp/protocol.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1bb486daf56a..8a299a8ef5e0 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -529,7 +529,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 		return false;
 
 	/* MPC/MPJ needed only on 3rd ack packet, DATA_FIN and TCP shutdown take precedence */
-	if (READ_ONCE(subflow->fully_established) || snd_data_fin_enable ||
+	if (subflow->fully_established || snd_data_fin_enable ||
 	    subflow->snd_isn != TCP_SKB_CB(skb)->seq ||
 	    sk->sk_state != TCP_ESTABLISHED)
 		return false;
@@ -980,7 +980,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	/* here we can process OoO, in-window pkts, only in-sequence 4th ack
 	 * will make the subflow fully established
 	 */
-	if (likely(READ_ONCE(subflow->fully_established))) {
+	if (likely(subflow->fully_established)) {
 		/* on passive sockets, check for 3rd ack retransmission
 		 * note that msk is always set by subflow_syn_recv_sock()
 		 * for mp_join subflows
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2ee23e9411be..b311fe30c785 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3912,7 +3912,7 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
 	struct tcp_sock *tp = tcp_sk(ssk);
 	unsigned long timeout;
 
-	if (READ_ONCE(mptcp_subflow_ctx(ssk)->fully_established))
+	if (mptcp_subflow_ctx(ssk)->fully_established)
 		return;
 
 	/* reschedule with a timeout above RTT, as we must look only for drop */
-- 
2.55.0

[PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets

[Paolo Abeni]

On fallback socket the retrans timer is a quite convoluted no-op, but
currently nothing prevents the MPTCP core to keep rescheduling it.

Additionally gate RTX timer reset to the msk not being fallen back to
TCP yet. To avoid adding multiple tests in fast-path, use a new flags
bit for such condition.

Fixes: b51f9b80c032 ("mptcp: introduce MPTCP retransmission timer")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
In fact so convoluted that sashiko thinks otherwise. Sashiko:
in case of fallback there could be a single active subflow; it can't
become stale
---
 net/mptcp/protocol.c | 11 ++++++++---
 net/mptcp/protocol.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b311fe30c785..f22d64ab1c53 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -96,6 +96,7 @@ bool __mptcp_try_fallback(struct mptcp_sock *msk, int fb_mib)
 
 	msk->allow_subflows = false;
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
+	set_bit(MPTCP_RTX_DISABLED, &msk->flags);
 	__MPTCP_INC_STATS(net, fb_mib);
 	spin_unlock_bh(&msk->fallback_lock);
 	return true;
@@ -1121,13 +1122,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
 
 static void mptcp_reset_rtx_timer(struct sock *sk)
 {
+	struct mptcp_sock *msk = mptcp_sk(sk);
 	unsigned long tout;
 
-	/* prevent rescheduling on close */
-	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
+	/* Prevent rescheduling on close and in case of fallback. */
+	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
 		return;
 
-	tout = mptcp_sk(sk)->timer_ival;
+	tout = msk->timer_ival;
 	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
 }
 
@@ -3360,6 +3362,9 @@ void mptcp_set_state(struct sock *sk, int state)
 		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
 		 */
 		break;
+	case TCP_CLOSE:
+		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
+		fallthrough;
 	default:
 		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
 			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7e168e450fb0..f9300d644ff3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,6 +116,7 @@
 #define MPTCP_WORK_RTX		1
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
+#define MPTCP_RTX_DISABLED	4
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
-- 
2.55.0

Re: [PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The MPTCP_RTX_DISABLED flag makes the TCP_CLOSE state sticky for the RTX timer, permanently disabling retransmissions on reused connections and their accepted children.
--

--- Patch [4]: [PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets ---
commit 227375cb381dd3864273cd7170dd28e0f079ff8b
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: do not reschedule the RTX timer for fallback sockets
    
This patch introduces the MPTCP_RTX_DISABLED flag to prevent the RTX timer
from being rescheduled on fallback sockets and when the socket state is
TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b311fe30c7855..f22d64ab1c53b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1121,13 +1122,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
>  
>  static void mptcp_reset_rtx_timer(struct sock *sk)
>  {
> +	struct mptcp_sock *msk = mptcp_sk(sk);
>  	unsigned long tout;
>  
> -	/* prevent rescheduling on close */
> -	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
> +	/* Prevent rescheduling on close and in case of fallback. */
> +	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
>  		return;

[Severity: High]
Since this flag is checked here to prevent rescheduling, will the timer be
permanently disabled if the flag is never cleared?

[ ... ]
> @@ -3360,6 +3362,9 @@ void mptcp_set_state(struct sock *sk, int state)
>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>  		 */
>  		break;
> +	case TCP_CLOSE:
> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
> +		fallthrough;
>  	default:

[Severity: High]
Does setting MPTCP_RTX_DISABLED here without a corresponding clear logic make
the TCP_CLOSE state sticky for the RTX timer? 

If an MPTCP socket transitions to TCP_CLOSE (like a failed connect() attempt
or an explicit disconnect()), and the application then reuses the socket to 
connect() again, it appears the flag will remain set.

Furthermore, if listen() is called on such a reused socket, would
sk_clone_lock() copy the flags to all child sockets? This could cause every
accepted connection to inherit the MPTCP_RTX_DISABLED flag, permanently
breaking MPTCP-level retransmissions for all clients.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786713351.git.pabeni@redhat.com?part=4

Re: [PATCH resend mptcp-net 0/4] mptcp: a few fixes

[MPTCP CI]

Hi Paolo,

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): Unstable: 1 failed test(s): selftest_simult_flows ⚠️ 
- 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! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/31806665824

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


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 resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets

[Paolo Abeni]

On 8/14/26 4:03 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The MPTCP_RTX_DISABLED flag makes the TCP_CLOSE state sticky for the RTX timer, permanently disabling retransmissions on reused connections and their accepted children.
> --
> 
> --- Patch [4]: [PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets ---
> commit 227375cb381dd3864273cd7170dd28e0f079ff8b
> Author: Paolo Abeni <pabeni@redhat.com>
> 
> mptcp: do not reschedule the RTX timer for fallback sockets
>     
> This patch introduces the MPTCP_RTX_DISABLED flag to prevent the RTX timer
> from being rescheduled on fallback sockets and when the socket state is
> TCP_CLOSE.
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index b311fe30c7855..f22d64ab1c53b 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
> [ ... ]
>> @@ -1121,13 +1122,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
>>  
>>  static void mptcp_reset_rtx_timer(struct sock *sk)
>>  {
>> +	struct mptcp_sock *msk = mptcp_sk(sk);
>>  	unsigned long tout;
>>  
>> -	/* prevent rescheduling on close */
>> -	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
>> +	/* Prevent rescheduling on close and in case of fallback. */
>> +	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
>>  		return;
> 
> [Severity: High]
> Since this flag is checked here to prevent rescheduling, will the timer be
> permanently disabled if the flag is never cleared?
> 
> [ ... ]
>> @@ -3360,6 +3362,9 @@ void mptcp_set_state(struct sock *sk, int state)
>>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>>  		 */
>>  		break;
>> +	case TCP_CLOSE:
>> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
>> +		fallthrough;
>>  	default:
> 
> [Severity: High]
> Does setting MPTCP_RTX_DISABLED here without a corresponding clear logic make
> the TCP_CLOSE state sticky for the RTX timer? 
> 
> If an MPTCP socket transitions to TCP_CLOSE (like a failed connect() attempt
> or an explicit disconnect()), and the application then reuses the socket to 
> connect() again, it appears the flag will remain set.
> 
> Furthermore, if listen() is called on such a reused socket, would
> sk_clone_lock() copy the flags to all child sockets? This could cause every
> accepted connection to inherit the MPTCP_RTX_DISABLED flag, permanently
> breaking MPTCP-level retransmissions for all clients.

On nice, I did not read an hallucination this allucinated since a while...

without disconnect() a closed socket can not transition to other non
closed states, because the network-level ops check the 'struct socket'
state, which in turn is cleared only by disconnect().

mptcp_disconnect() explicitly clears msk->flags.

No real issues above.

/P