From 3fffa15bfef48b0ad6424779c03e68ae8ace5acb Mon Sep 17 00:00:00 2001 From: Paolo Abeni Date: Tue, 4 Jul 2023 22:44:33 +0200 Subject: [PATCH 1/9] mptcp: ensure subflow is unhashed before cleaning the backlog While tacking care of the mptcp-level listener I unintentionally moved the subflow level unhash after the subflow listener backlog cleanup. That could cause some nasty race and makes the code harder to read. Address the issue restoring the proper order of operations. Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status") Cc: stable@vger.kernel.org Signed-off-by: Paolo Abeni Reviewed-by: Matthieu Baerts Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- net/mptcp/protocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index e892673deb73b..489a3defdde5c 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -2909,10 +2909,10 @@ static void mptcp_check_listen_stop(struct sock *sk) return; lock_sock_nested(ssk, SINGLE_DEPTH_NESTING); + tcp_set_state(ssk, TCP_CLOSE); mptcp_subflow_queue_clean(sk, ssk); inet_csk_listen_stop(ssk); mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CLOSED); - tcp_set_state(ssk, TCP_CLOSE); release_sock(ssk); } From 0226436acf2495cde4b93e7400e5a87305c26054 Mon Sep 17 00:00:00 2001 From: Paolo Abeni Date: Tue, 4 Jul 2023 22:44:34 +0200 Subject: [PATCH 2/9] mptcp: do not rely on implicit state check in mptcp_listen() Since the blamed commit, closing the first subflow resets the first subflow socket state to SS_UNCONNECTED. The current mptcp listen implementation relies only on such state to prevent touching not-fully-disconnected sockets. Incoming mptcp fastclose (or paired endpoint removal) unconditionally closes the first subflow. All the above allows an incoming fastclose followed by a listen() call to successfully race with a blocking recvmsg(), potentially causing the latter to hit a divide by zero bug in cleanup_rbuf/__tcp_select_window(). Address the issue explicitly checking the msk socket state in mptcp_listen(). An alternative solution would be moving the first subflow socket state update into mptcp_disconnect(), but in the long term the first subflow socket should be removed: better avoid relaying on it for internal consistency check. Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation") Cc: stable@vger.kernel.org Reported-by: Christoph Paasch Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/414 Signed-off-by: Paolo Abeni Reviewed-by: Matthieu Baerts Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- net/mptcp/protocol.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 489a3defdde5c..3613489eb6e3b 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3703,6 +3703,11 @@ static int mptcp_listen(struct socket *sock, int backlog) pr_debug("msk=%p", msk); lock_sock(sk); + + err = -EINVAL; + if (sock->state != SS_UNCONNECTED || sock->type != SOCK_STREAM) + goto unlock; + ssock = __mptcp_nmpc_socket(msk); if (IS_ERR(ssock)) { err = PTR_ERR(ssock); From 221e4550454a822f9a11834e30694c7d1d65747c Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:35 +0200 Subject: [PATCH 3/9] selftests: mptcp: connect: fail if nft supposed to work In case of "external" errors when preparing the environment for the TProxy tests, the subtests were marked as skipped. This is fine but it means these errors are ignored. On MPTCP Public CI, we do want to catch such issues and mark the selftest as failed if there are such issues. We can then use mptcp_lib_fail_if_expected_feature() helper that has been recently added to fail if needed. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: 5fb62e9cd3ad ("selftests: mptcp: add tproxy test case") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh index 13561e5bc0cdb..bbae40882bfaf 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh @@ -718,6 +718,7 @@ table inet mangle { EOF if [ $? -ne 0 ]; then echo "SKIP: $msg, could not load nft ruleset" + mptcp_lib_fail_if_expected_feature "nft rules" return fi @@ -733,6 +734,7 @@ EOF if [ $? -ne 0 ]; then ip netns exec "$listener_ns" nft flush ruleset echo "SKIP: $msg, ip $r6flag rule failed" + mptcp_lib_fail_if_expected_feature "ip rule" return fi @@ -741,6 +743,7 @@ EOF ip netns exec "$listener_ns" nft flush ruleset ip -net "$listener_ns" $r6flag rule del fwmark 1 lookup 100 echo "SKIP: $msg, ip route add local $local_addr failed" + mptcp_lib_fail_if_expected_feature "ip route" return fi From a5a5990c099dd354e05e89ee77cd2dbf6655d4a1 Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:36 +0200 Subject: [PATCH 4/9] selftests: mptcp: sockopt: use 'iptables-legacy' if available IPTables commands using 'iptables-nft' fail on old kernels, at least on v5.15 because it doesn't see the default IPTables chains: $ iptables -L iptables/1.8.2 Failed to initialize nft: Protocol not supported As a first step before switching to NFTables, we can use iptables-legacy if available. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: dc65fe82fb07 ("selftests: mptcp: add packet mark test case") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- .../selftests/net/mptcp/mptcp_sockopt.sh | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh index f295a371ff148..c21bfd7f0c013 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh @@ -12,6 +12,8 @@ ksft_skip=4 timeout_poll=30 timeout_test=$((timeout_poll * 2 + 1)) mptcp_connect="" +iptables="iptables" +ip6tables="ip6tables" sec=$(date +%s) rndh=$(printf %x $sec)-$(mktemp -u XXXXXX) @@ -25,7 +27,7 @@ add_mark_rules() local m=$2 local t - for t in iptables ip6tables; do + for t in ${iptables} ${ip6tables}; do # just to debug: check we have multiple subflows connection requests ip netns exec $ns $t -A OUTPUT -p tcp --syn -m mark --mark $m -j ACCEPT @@ -95,14 +97,14 @@ if [ $? -ne 0 ];then exit $ksft_skip fi -iptables -V > /dev/null 2>&1 -if [ $? -ne 0 ];then +# Use the legacy version if available to support old kernel versions +if iptables-legacy -V &> /dev/null; then + iptables="iptables-legacy" + ip6tables="ip6tables-legacy" +elif ! iptables -V &> /dev/null; then echo "SKIP: Could not run all tests without iptables tool" exit $ksft_skip -fi - -ip6tables -V > /dev/null 2>&1 -if [ $? -ne 0 ];then +elif ! ip6tables -V &> /dev/null; then echo "SKIP: Could not run all tests without ip6tables tool" exit $ksft_skip fi @@ -112,10 +114,10 @@ check_mark() local ns=$1 local af=$2 - local tables=iptables + local tables=${iptables} if [ $af -eq 6 ];then - tables=ip6tables + tables=${ip6tables} fi local counters values From 9ac4c28eb70cd5ea5472a5e1c495dcdd597d4597 Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:37 +0200 Subject: [PATCH 5/9] selftests: mptcp: sockopt: return error if wrong mark When an error was detected when checking the marks, a message was correctly printed mentioning the error but followed by another one saying everything was OK and the selftest was not marked as failed as expected. Now the 'ret' variable is directly set to 1 in order to make sure the exit is done with an error, similar to what is done in other functions. While at it, the error is correctly propagated to the caller. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: dc65fe82fb07 ("selftests: mptcp: add packet mark test case") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh index c21bfd7f0c013..dc8d473fc82c8 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh @@ -128,6 +128,7 @@ check_mark() for v in $values; do if [ $v -ne 0 ]; then echo "FAIL: got $tables $values in ns $ns , not 0 - not all expected packets marked" 1>&2 + ret=1 return 1 fi done @@ -227,11 +228,11 @@ do_transfer() fi if [ $local_addr = "::" ];then - check_mark $listener_ns 6 - check_mark $connector_ns 6 + check_mark $listener_ns 6 || retc=1 + check_mark $connector_ns 6 || retc=1 else - check_mark $listener_ns 4 - check_mark $connector_ns 4 + check_mark $listener_ns 4 || retc=1 + check_mark $connector_ns 4 || retc=1 fi check_transfer $cin $sout "file received by server" From d8566d0e03922217f70d9be2d401fcb860986374 Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:38 +0200 Subject: [PATCH 6/9] selftests: mptcp: userspace_pm: use correct server port "server4_port" variable is not set but "app4_port" is the server port in v4 and the correct variable name to use. The port is optional so there was no visible impact. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: ca188a25d43f ("selftests: mptcp: userspace PM support for MP_PRIO signals") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh index 98d9e4d2d3fc2..841a67a7d5243 100755 --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh @@ -848,7 +848,7 @@ test_prio() local count # Send MP_PRIO signal from client to server machine - ip netns exec "$ns2" ./pm_nl_ctl set 10.0.1.2 port "$client4_port" flags backup token "$client4_token" rip 10.0.1.1 rport "$server4_port" + ip netns exec "$ns2" ./pm_nl_ctl set 10.0.1.2 port "$client4_port" flags backup token "$client4_token" rip 10.0.1.1 rport "$app4_port" sleep 0.5 # Check TX From 966c6c3adfb1257ea8a839cdfad2b74092cc5532 Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:39 +0200 Subject: [PATCH 7/9] selftests: mptcp: userspace_pm: report errors with 'remove' tests A message was mentioning an issue with the "remove" tests but the selftest was not marked as failed. Directly exit with an error like it is done everywhere else in this selftest. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: 259a834fadda ("selftests: mptcp: functional tests for the userspace PM type") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh index 841a67a7d5243..b180133a30af7 100755 --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh @@ -423,6 +423,7 @@ test_remove() stdbuf -o0 -e0 printf "[OK]\n" else stdbuf -o0 -e0 printf "[FAIL]\n" + exit 1 fi # RM_ADDR using an invalid addr id should result in no action @@ -437,6 +438,7 @@ test_remove() stdbuf -o0 -e0 printf "[OK]\n" else stdbuf -o0 -e0 printf "[FAIL]\n" + exit 1 fi # RM_ADDR from the client to server machine From 6c8880fcaa5c45355179b759c1d11737775e31fc Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:40 +0200 Subject: [PATCH 8/9] selftests: mptcp: depend on SYN_COOKIES MPTCP selftests are using TCP SYN Cookies for quite a while now, since v5.9. Some CIs don't have this config option enabled and this is causing issues in the tests: # ns1 MPTCP -> ns1 (10.0.1.1:10000 ) MPTCP (duration 167ms) sysctl: cannot stat /proc/sys/net/ipv4/tcp_syncookies: No such file or directory # [ OK ]./mptcp_connect.sh: line 554: [: -eq: unary operator expected There is no impact in the results but the test is not doing what it is supposed to do. Fixes: fed61c4b584c ("selftests: mptcp: make 2nd net namespace use tcp syn cookies unconditionally") Cc: stable@vger.kernel.org Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/config | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config index 6032f9b23c4c2..e317c2e44dae8 100644 --- a/tools/testing/selftests/net/mptcp/config +++ b/tools/testing/selftests/net/mptcp/config @@ -6,6 +6,7 @@ CONFIG_INET_DIAG=m CONFIG_INET_MPTCP_DIAG=m CONFIG_VETH=y CONFIG_NET_SCH_NETEM=m +CONFIG_SYN_COOKIES=y CONFIG_NETFILTER=y CONFIG_NETFILTER_ADVANCED=y CONFIG_NETFILTER_NETLINK=m From 61d9658050260dbcbf9055479b7ac5bbbe1e8831 Mon Sep 17 00:00:00 2001 From: Matthieu Baerts Date: Tue, 4 Jul 2023 22:44:41 +0200 Subject: [PATCH 9/9] selftests: mptcp: pm_nl_ctl: fix 32-bit support When using pm_nl_ctl to validate userspace path-manager's behaviours, it was failing on 32-bit architectures ~half of the time. pm_nl_ctl was not reporting any error but the command was not doing what it was expected to do. As a result, the expected linked event was not triggered after and the test failed. This is due to the fact the token given in argument to the application was parsed as an integer with atoi(): in a 32-bit arch, if the number was bigger than INT_MAX, 2147483647 was used instead. This can simply be fixed by using strtoul() instead of atoi(). The errors have been seen "by chance" when manually looking at the results from LKFT. Fixes: 9a0b36509df0 ("selftests: mptcp: support MPTCP_PM_CMD_ANNOUNCE") Cc: stable@vger.kernel.org Fixes: ecd2a77d672f ("selftests: mptcp: support MPTCP_PM_CMD_REMOVE") Fixes: cf8d0a6dfd64 ("selftests: mptcp: support MPTCP_PM_CMD_SUBFLOW_CREATE") Fixes: 57cc361b8d38 ("selftests: mptcp: support MPTCP_PM_CMD_SUBFLOW_DESTROY") Fixes: ca188a25d43f ("selftests: mptcp: userspace PM support for MP_PRIO signals") Signed-off-by: Matthieu Baerts Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c index abddf4c63e797..1887bd61bd9a5 100644 --- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c +++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c @@ -425,7 +425,7 @@ int dsf(int fd, int pm_family, int argc, char *argv[]) } /* token */ - token = atoi(params[4]); + token = strtoul(params[4], NULL, 10); rta = (void *)(data + off); rta->rta_type = MPTCP_PM_ATTR_TOKEN; rta->rta_len = RTA_LENGTH(4); @@ -551,7 +551,7 @@ int csf(int fd, int pm_family, int argc, char *argv[]) } /* token */ - token = atoi(params[4]); + token = strtoul(params[4], NULL, 10); rta = (void *)(data + off); rta->rta_type = MPTCP_PM_ATTR_TOKEN; rta->rta_len = RTA_LENGTH(4); @@ -598,7 +598,7 @@ int remove_addr(int fd, int pm_family, int argc, char *argv[]) if (++arg >= argc) error(1, 0, " missing token value"); - token = atoi(argv[arg]); + token = strtoul(argv[arg], NULL, 10); rta = (void *)(data + off); rta->rta_type = MPTCP_PM_ATTR_TOKEN; rta->rta_len = RTA_LENGTH(4); @@ -710,7 +710,7 @@ int announce_addr(int fd, int pm_family, int argc, char *argv[]) if (++arg >= argc) error(1, 0, " missing token value"); - token = atoi(argv[arg]); + token = strtoul(argv[arg], NULL, 10); } else error(1, 0, "unknown keyword %s", argv[arg]); } @@ -1347,7 +1347,7 @@ int set_flags(int fd, int pm_family, int argc, char *argv[]) error(1, 0, " missing token value"); /* token */ - token = atoi(argv[arg]); + token = strtoul(argv[arg], NULL, 10); } else if (!strcmp(argv[arg], "flags")) { char *tok, *str;