In case multiple subflows race to update the mptcp-level receive
window, the subflow losing the race should use the window value
provided by the "winning" subflow to update it's own tcp-level
rcv_wnd.

To such goal, the current code bogusly uses the mptcp-level rcv_wnd
value as observed before the update attempt. On unluckly circumstances
that may lead to TCP-level window shrinkage, and stall the other end.

Address the issue feeding to the rcv wnd update the correct value.

Fixes: f3589be0c420 ("mptcp: never shrink offered window")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/options.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c254accb14de..cd15ec73073e 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1269,12 +1269,13 @@ static void mptcp_set_rwin(struct tcp_sock *tp, struct tcphdr *th)
 
 			if (rcv_wnd == rcv_wnd_old)
 				break;
-			if (before64(rcv_wnd_new, rcv_wnd)) {
+
+			rcv_wnd_old = rcv_wnd;
+			if (before64(rcv_wnd_new, rcv_wnd_old)) {
 				MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_RCVWNDCONFLICTUPDATE);
 				goto raise_win;
 			}
 			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_RCVWNDCONFLICT);
-			rcv_wnd_old = rcv_wnd;
 		}
 		return;
 	}
-- 
2.41.0

Hi Paolo, Mat,

On 23/08/2023 10:18, Paolo Abeni wrote:
> In case multiple subflows race to update the mptcp-level receive
> window, the subflow losing the race should use the window value
> provided by the "winning" subflow to update it's own tcp-level
> rcv_wnd.
> 
> To such goal, the current code bogusly uses the mptcp-level rcv_wnd
> value as observed before the update attempt. On unluckly circumstances
> that may lead to TCP-level window shrinkage, and stall the other end.
> 
> Address the issue feeding to the rcv wnd update the correct value.

Thank you for the patch and the review!

Sorry for the delay but this is now in our tree (fixes for -net), with
the "Closes" tag:

New patches for t/upstream-net and t/upstream:
- 59d3c92821f2: mptcp: fix bogus receive window shrinkage with multiple
subflows
- 490c75fb2e65: tg:msg: add Closes tag + typo
- Results: 33d85a2cd769..fa580ac35599 (export-net)
- Results: a96bdbce1428..54ea4fcf4272 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230829T165049
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230829T165049

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Wed, 23 Aug 2023, Paolo Abeni wrote:

> In case multiple subflows race to update the mptcp-level receive
> window, the subflow losing the race should use the window value
> provided by the "winning" subflow to update it's own tcp-level
> rcv_wnd.
>
> To such goal, the current code bogusly uses the mptcp-level rcv_wnd
> value as observed before the update attempt. On unluckly circumstances
> that may lead to TCP-level window shrinkage, and stall the other end.
>
> Address the issue feeding to the rcv wnd update the correct value.
>
> Fixes: f3589be0c420 ("mptcp: never shrink offered window")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/options.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>

Good catch, thanks Paolo!

Reviewed-by: Mat Martineau <martineau@kernel.org>


> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index c254accb14de..cd15ec73073e 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1269,12 +1269,13 @@ static void mptcp_set_rwin(struct tcp_sock *tp, struct tcphdr *th)
>
> 			if (rcv_wnd == rcv_wnd_old)
> 				break;
> -			if (before64(rcv_wnd_new, rcv_wnd)) {
> +
> +			rcv_wnd_old = rcv_wnd;
> +			if (before64(rcv_wnd_new, rcv_wnd_old)) {
> 				MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_RCVWNDCONFLICTUPDATE);
> 				goto raise_win;
> 			}
> 			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_RCVWNDCONFLICT);
> -			rcv_wnd_old = rcv_wnd;
> 		}
> 		return;
> 	}
> -- 
> 2.41.0
>
>
>