| Message ID | a5f63fbcf13323d8cf293f42849673093d1bf45b.1710179839.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | [mptcp-net,v3] mptcp: don't account accept() of non-MPC client as fallback to TCP | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
| matttbe/shellcheck | success | No ShellCheck issues |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | warning | Unstable: 1 failed test(s): packetdrill_mp_join |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
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 > > >
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.
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"
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(-)