net/mptcp/protocol.c | 2 -- net/mptcp/subflow.c | 2 ++ tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++++ 3 files changed, 11 insertions(+), 2 deletions(-)
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>
---
v4:
- improve kselftest (thanks Mat + Matthieu)
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 | 9 +++++++++
3 files changed, 11 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..4131f3263a48 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,11 @@ do_transfer()
fi
fi
+ if [ ${stat_ooo_now} -eq 0 ] && [ ${stat_tcpfb_last_l} -ne ${stat_tcpfb_now_l} ]; then
+ mptcp_lib_pr_fail "unexpected fallback to TCP"
+ rets=1
+ fi
+
if [ $cookies -eq 2 ];then
if [ $stat_cookietx_last -ge $stat_cookietx_now ] ;then
extra+=" WARN: CookieSent: did not advance"
--
2.44.0Hi Davide, Mat, On 20/03/2024 09:39, 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. Thank you for the patch and the review! Now in our tree (fix for -net): New patches for t/upstream-net and t/upstream: - 36d719290a00: mptcp: don't account accept() of non-MPC client as fallback to TCP - Results: 94df68ee41a5..101f38e8fc45 (export-net) - Results: 0dd5003ed1e8..ff567d2891c6 (export) Tests are now in progress: - export-net: https://github.com/multipath-tcp/mptcp_net-next/commit/677687ee9a13acdc4a61ba905a4b8191508c6da4/checks - export: https://github.com/multipath-tcp/mptcp_net-next/commit/78d5a97a72615fb1a2a1b09f10496b6737376506/checks Cheers, Matt -- Sponsored by the NGI0 Core fund.
On Wed, 20 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>
>
> ---
> v4:
> - improve kselftest (thanks Mat + Matthieu)
Selftest changes LGTM, thanks Davide:
Reviewed-by: Mat Martineau <martineau@kernel.org>
> 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 | 9 +++++++++
> 3 files changed, 11 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..4131f3263a48 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,11 @@ do_transfer()
> fi
> fi
>
> + if [ ${stat_ooo_now} -eq 0 ] && [ ${stat_tcpfb_last_l} -ne ${stat_tcpfb_now_l} ]; then
> + mptcp_lib_pr_fail "unexpected fallback to TCP"
> + rets=1
> + fi
> +
> if [ $cookies -eq 2 ];then
> if [ $stat_cookietx_last -ge $stat_cookietx_now ] ;then
> extra+=" WARN: CookieSent: did not advance"
> --
> 2.44.0
>
>
Hi Davide,
On 20/03/2024 09:39, 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>
>
> ---
> v4:
> - improve kselftest (thanks Mat + Matthieu)
Thank you for the v4! It looks good to me:
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
I will wait for Mat before applying this patch as he reviewed the
previous version.
Also, just to know what are the next steps: do you still plan to add a
quick test using packetdrill?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.