1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-net v3] mptcp: don't account accept() of non-MPC client as fallback to TCP

Davide Caratti posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/a5f63fbcf13323d8cf293f42849673093d1bf45b.1710179839.git.dcaratti@redhat.com
There is a newer version of this series
net/mptcp/protocol.c                               |  2 --
net/mptcp/subflow.c                                |  2 ++
tools/testing/selftests/net/mptcp/mptcp_connect.sh | 12 ++++++++++++
3 files changed, 14 insertions(+), 2 deletions(-)
[PATCH mptcp-net v3] mptcp: don't account accept() of non-MPC client as fallback to TCP
Posted by Davide Caratti 1 month, 2 weeks ago 
Current MPTCP servers increment MPTcpExtMPCapableFallbackACK when they
accept non-MPC connections. As reported by Christoph, this is "surprising"
because the counter might become greater than MPTcpExtMPCapableSYNRX.

MPTcpExtMPCapableFallbackACK counter's name suggests it should only be
incremented when a connection was seen using MPTCP options, then a
fallback to TCP has been done. Let's do that by incrementing it when
the subflow context of an inbound MPC connection attempt is dropped.
Also, update mptcp_connect.sh kselftest, to ensure that the
above MIB does not increment in case a pure TCP client connects to a
MPTCP server.

Fixes: fc518953bc9c ("mptcp: add and use MIB counter infrastructure")
Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/449
Signed-off-by: Davide Caratti <dcaratti@redhat.com>

--
v3:
 - fix typo in the if() conditional (thanks Matthieu)
 - rephrase commit message to target "-net" tree (thanks Matthieu)
 - add kselftest (thanks Matthieu)
v2:
 - fix reporter's name
---
 net/mptcp/protocol.c                               |  2 --
 net/mptcp/subflow.c                                |  2 ++
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 12 ++++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cdf9ec67795e..556b3b95c537 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3937,8 +3937,6 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 				mptcp_set_state(newsk, TCP_CLOSE);
 		}
 	} else {
-		MPTCP_INC_STATS(sock_net(ssk),
-				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
 tcpfallback:
 		newsk->sk_kern_sock = kern;
 		lock_sock(newsk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1626dd20c68f..6042a47da61b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	return child;
 
 fallback:
+	if (fallback)
+		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
 	mptcp_subflow_drop_ctx(child);
 	return child;
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 4c4248554826..f05af523361a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -383,12 +383,14 @@ do_transfer()
 	local stat_cookierx_last
 	local stat_csum_err_s
 	local stat_csum_err_c
+	local stat_tcpfb_last_l
 	stat_synrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
 	stat_ackrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
 	stat_cookietx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
 	stat_cookierx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
 	stat_csum_err_s=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtDataCsumErr")
 	stat_csum_err_c=$(mptcp_lib_get_counter "${connector_ns}" "MPTcpExtDataCsumErr")
+	stat_tcpfb_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableFallbackACK")
 
 	timeout ${timeout_test} \
 		ip netns exec ${listener_ns} \
@@ -457,11 +459,13 @@ do_transfer()
 	local stat_cookietx_now
 	local stat_cookierx_now
 	local stat_ooo_now
+	local stat_tcpfb_now_l
 	stat_synrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
 	stat_ackrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
 	stat_cookietx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
 	stat_cookierx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
 	stat_ooo_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtTCPOFOQueue")
+	stat_tcpfb_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableFallbackACK")
 
 	expect_synrx=$((stat_synrx_last_l))
 	expect_ackrx=$((stat_ackrx_last_l))
@@ -508,6 +512,14 @@ do_transfer()
 		fi
 	fi
 
+	if [ ${cl_proto} = "TCP" ] && [ ${srv_proto} = "MPTCP" ]; then
+		if [ $stat_tcpfb_last_l -ne $stat_tcpfb_now_l ]; then
+			mptcp_lib_pr_fail "pure TCP counted as fallback (expect:%d, got:%d)" \
+					   $stat_tcpfb_last_l $stat_tcpfb_now_l 1>&2
+			rets=1
+		fi
+	fi
+
 	if [ $cookies -eq 2 ];then
 		if [ $stat_cookietx_last -ge $stat_cookietx_now ] ;then
 			extra+=" WARN: CookieSent: did not advance"
-- 
2.44.0
Re: [PATCH mptcp-net v3] mptcp: don't account accept() of non-MPC client as fallback to TCP
Posted by Mat Martineau 1 month, 2 weeks ago 
On Mon, 11 Mar 2024, Davide Caratti wrote:

> Current MPTCP servers increment MPTcpExtMPCapableFallbackACK when they
> accept non-MPC connections. As reported by Christoph, this is "surprising"
> because the counter might become greater than MPTcpExtMPCapableSYNRX.
>
> MPTcpExtMPCapableFallbackACK counter's name suggests it should only be
> incremented when a connection was seen using MPTCP options, then a
> fallback to TCP has been done. Let's do that by incrementing it when
> the subflow context of an inbound MPC connection attempt is dropped.
> Also, update mptcp_connect.sh kselftest, to ensure that the
> above MIB does not increment in case a pure TCP client connects to a
> MPTCP server.
>
> Fixes: fc518953bc9c ("mptcp: add and use MIB counter infrastructure")
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/449
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
>
> --
> v3:
> - fix typo in the if() conditional (thanks Matthieu)
> - rephrase commit message to target "-net" tree (thanks Matthieu)
> - add kselftest (thanks Matthieu)
> v2:
> - fix reporter's name
> ---
> net/mptcp/protocol.c                               |  2 --
> net/mptcp/subflow.c                                |  2 ++
> tools/testing/selftests/net/mptcp/mptcp_connect.sh | 12 ++++++++++++
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index cdf9ec67795e..556b3b95c537 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3937,8 +3937,6 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 				mptcp_set_state(newsk, TCP_CLOSE);
> 		}
> 	} else {
> -		MPTCP_INC_STATS(sock_net(ssk),
> -				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
> tcpfallback:
> 		newsk->sk_kern_sock = kern;
> 		lock_sock(newsk);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1626dd20c68f..6042a47da61b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 	return child;
>
> fallback:
> +	if (fallback)
> +		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);

Hi Davide -

Changes to the C code LGTM. See below regarding the self test.

> 	mptcp_subflow_drop_ctx(child);
> 	return child;
> }
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 4c4248554826..f05af523361a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -383,12 +383,14 @@ do_transfer()
> 	local stat_cookierx_last
> 	local stat_csum_err_s
> 	local stat_csum_err_c
> +	local stat_tcpfb_last_l
> 	stat_synrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
> 	stat_ackrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
> 	stat_cookietx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
> 	stat_cookierx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
> 	stat_csum_err_s=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtDataCsumErr")
> 	stat_csum_err_c=$(mptcp_lib_get_counter "${connector_ns}" "MPTcpExtDataCsumErr")
> +	stat_tcpfb_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableFallbackACK")
>
> 	timeout ${timeout_test} \
> 		ip netns exec ${listener_ns} \
> @@ -457,11 +459,13 @@ do_transfer()
> 	local stat_cookietx_now
> 	local stat_cookierx_now
> 	local stat_ooo_now
> +	local stat_tcpfb_now_l
> 	stat_synrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
> 	stat_ackrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
> 	stat_cookietx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
> 	stat_cookierx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
> 	stat_ooo_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtTCPOFOQueue")
> +	stat_tcpfb_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableFallbackACK")
>
> 	expect_synrx=$((stat_synrx_last_l))
> 	expect_ackrx=$((stat_ackrx_last_l))
> @@ -508,6 +512,14 @@ do_transfer()
> 		fi
> 	fi
>
> +	if [ ${cl_proto} = "TCP" ] && [ ${srv_proto} = "MPTCP" ]; then
> +		if [ $stat_tcpfb_last_l -ne $stat_tcpfb_now_l ]; then
> +			mptcp_lib_pr_fail "pure TCP counted as fallback (expect:%d, got:%d)" \
> +					   $stat_tcpfb_last_l $stat_tcpfb_now_l 1>&2

This prints:

[FAIL] pure TCP counted as fallback (expect:%d, got:%d) 0 1

which I don't think is what you intended


I confirmed the test does fail on a Fedora 6.7.9 kernel (as it should, it 
caught the old behavior).

- Mat

> +			rets=1
> +		fi
> +	fi
> +
> 	if [ $cookies -eq 2 ];then
> 		if [ $stat_cookietx_last -ge $stat_cookietx_now ] ;then
> 			extra+=" WARN: CookieSent: did not advance"
> -- 
> 2.44.0
>
>
>
Re: [PATCH mptcp-net v3] mptcp: don't account accept() of non-MPC client as fallback to TCP
Posted by Davide Caratti 1 month, 2 weeks ago 
hello Mat, thanks for reviewing!

On Thu, Mar 14, 2024 at 1:46 AM Mat Martineau <martineau@kernel.org> wrote:
>

[...]

> This prints:
>
> [FAIL] pure TCP counted as fallback (expect:%d, got:%d) 0 1
>
> which I don't think is what you intended

yes I forgot to convert the old argument I used with printf: will fix
that in v4.

Thinking out loud with Matthieu: those "if" tests on client and server
protocol can be removed, mptcp_connect is not expected to fallback to
TCP at all in that scenario. So, whatever 4 combinations you can make
out of $cl_proto and $srv_proto, that MIB shall not have an increment.

-- 
davide

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.