| Message ID | 66cfbfdf241fb43c51bf3c7d35c92f79a92b025d.1727974826.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-net,1/2] mptcp: prevent MPC handshake on port-based signal endpoints | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 5 warnings, 0 checks, 116 lines checked |
| matttbe/shellcheck | fail | ShellCheck issues: mptcp_join.sh |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11166495934
Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/fc448b976b8c
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=895272
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Paolo, On 03/10/2024 19:02, Paolo Abeni wrote: > Explicitly verify that MPC connection attempts towards a port-based > signal endpoint fail with a reset. Thank you for this fix! Same here, the code looks good to me, but I have some small suggestions, and I can send them in a v2. > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > .../testing/selftests/net/mptcp/mptcp_join.sh | 80 ++++++++++++++----- > 1 file changed, 58 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh > index e8d0a01b4144..5e2ab7dfb182 100755 > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh > @@ -41,6 +41,7 @@ evts_ns2_pid=0 > last_test_failed=0 > last_test_skipped=0 > last_test_ignored=1 > +cappid="" > > declare -A all_tests > declare -a only_tests_ids > @@ -887,6 +888,34 @@ check_cestab() > fi > } > > +cond_start_capture() > +{ > + if $capture; then > + local capuser > + if [ -z $SUDO_USER ] ; then > + capuser="" > + else > + capuser="-Z $SUDO_USER" > + fi > + > + local capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}") Shellcheck is complaining here: > SC2155 (warning): Declare and assign separately to avoid masking return values. So 'local capfile' should be declared before. > + > + echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile" > + ip netns exec $1 tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 & > + cappid=$! > + > + sleep 1 > + fi > +} > + > +cond_stop_capture() > +{ > + if $capture; then > + sleep 1 > + kill $cappid (while at it, we can fix the indentation using tabs) > + fi > +} > + > do_transfer() > { > local listener_ns="$1" > @@ -896,7 +925,6 @@ do_transfer() > local connect_addr="$5" > > local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1)) > - local cappid > local FAILING_LINKS=${FAILING_LINKS:-""} > local fastclose=${fastclose:-""} > local speed=${speed:-"fast"} > @@ -905,22 +933,7 @@ do_transfer() > :> "$sout" > :> "$capout" This line should be moved too I suppose, and the 'cat' should be done at the end. > > - if $capture; then > - local capuser > - if [ -z $SUDO_USER ] ; then > - capuser="" > - else > - capuser="-Z $SUDO_USER" > - fi > - > - capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}") > - > - echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile" > - ip netns exec ${listener_ns} tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 & > - cappid=$! > - > - sleep 1 > - fi > + cond_start_capture ${listener_ns} > > NSTAT_HISTORY=/tmp/${listener_ns}.nstat ip netns exec ${listener_ns} \ > nstat -n > @@ -1007,11 +1020,6 @@ do_transfer() > wait $spid > local rets=$? > > - if $capture; then > - sleep 1 > - kill $cappid > - fi I guess you still want to stop the capture here. cond_stop_capture > - > NSTAT_HISTORY=/tmp/${listener_ns}.nstat ip netns exec ${listener_ns} \ > nstat | grep Tcp > /tmp/${listener_ns}.out > NSTAT_HISTORY=/tmp/${connector_ns}.nstat ip netns exec ${connector_ns} \ > @@ -2160,6 +2168,34 @@ signal_address_tests() > chk_add_nr 3 3 > fi > fi > + > + if reset "port-based signal endpoint must not accept mpc"; then Can we move this test at the end of the add_addr_ports_tests section? > + local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1)) (detail: maybe we can have a helper for this, just to show we use the same one as in do_transfer()) > + > + cond_start_capture ${ns1} > + > + pm_nl_add_endpoint ${ns1} 10.0.2.1 flags signal port ${port} > + mptcp_lib_wait_local_port_listen ${ns1} ${port} > + > + timeout 1 ip netns exec ${ns2} \ > + ./mptcp_connect -t ${timeout_poll} -p $port -s MPTCP 10.0.2.1 >/dev/null 2>&1 > + local ret=$? > + > + cond_stop_capture > + > + if [ ${ret} = 124 ]; then > + fail_test "timeout on connect" A check name is missing here (print_check()) like in the other validations. > + elif [ ! ${ret} ]; then > + fail_test "unexpected successful connect" > + else > + local count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPCAttemptOnEndpoint") (shellcheck is complaining about the same error here) > + if [ "${count}" != 1 ]; then > + fail_test "got ${count} MPC attempts on port-based enpoint, expected 1" We should skip the test if the counter is not available, like in the other validations. > + else > + print_ok > + fi > + fi Maybe we should move the validation to another helper, just to keep only the declaration here, and the validation elsewhere, just like other tests (even here it is not going to be used by other tests)? > + fi > } > > link_failure_tests() Cheers, Matt
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index e8d0a01b4144..5e2ab7dfb182 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -41,6 +41,7 @@ evts_ns2_pid=0 last_test_failed=0 last_test_skipped=0 last_test_ignored=1 +cappid="" declare -A all_tests declare -a only_tests_ids @@ -887,6 +888,34 @@ check_cestab() fi } +cond_start_capture() +{ + if $capture; then + local capuser + if [ -z $SUDO_USER ] ; then + capuser="" + else + capuser="-Z $SUDO_USER" + fi + + local capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}") + + echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile" + ip netns exec $1 tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 & + cappid=$! + + sleep 1 + fi +} + +cond_stop_capture() +{ + if $capture; then + sleep 1 + kill $cappid + fi +} + do_transfer() { local listener_ns="$1" @@ -896,7 +925,6 @@ do_transfer() local connect_addr="$5" local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1)) - local cappid local FAILING_LINKS=${FAILING_LINKS:-""} local fastclose=${fastclose:-""} local speed=${speed:-"fast"} @@ -905,22 +933,7 @@ do_transfer() :> "$sout" :> "$capout" - if $capture; then - local capuser - if [ -z $SUDO_USER ] ; then - capuser="" - else - capuser="-Z $SUDO_USER" - fi - - capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}") - - echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile" - ip netns exec ${listener_ns} tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 & - cappid=$! - - sleep 1 - fi + cond_start_capture ${listener_ns} NSTAT_HISTORY=/tmp/${listener_ns}.nstat ip netns exec ${listener_ns} \ nstat -n @@ -1007,11 +1020,6 @@ do_transfer() wait $spid local rets=$? - if $capture; then - sleep 1 - kill $cappid - fi - NSTAT_HISTORY=/tmp/${listener_ns}.nstat ip netns exec ${listener_ns} \ nstat | grep Tcp > /tmp/${listener_ns}.out NSTAT_HISTORY=/tmp/${connector_ns}.nstat ip netns exec ${connector_ns} \ @@ -2160,6 +2168,34 @@ signal_address_tests() chk_add_nr 3 3 fi fi + + if reset "port-based signal endpoint must not accept mpc"; then + local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1)) + + cond_start_capture ${ns1} + + pm_nl_add_endpoint ${ns1} 10.0.2.1 flags signal port ${port} + mptcp_lib_wait_local_port_listen ${ns1} ${port} + + timeout 1 ip netns exec ${ns2} \ + ./mptcp_connect -t ${timeout_poll} -p $port -s MPTCP 10.0.2.1 >/dev/null 2>&1 + local ret=$? + + cond_stop_capture + + if [ ${ret} = 124 ]; then + fail_test "timeout on connect" + elif [ ! ${ret} ]; then + fail_test "unexpected successful connect" + else + local count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPCAttemptOnEndpoint") + if [ "${count}" != 1 ]; then + fail_test "got ${count} MPC attempts on port-based enpoint, expected 1" + else + print_ok + fi + fi + fi } link_failure_tests()
Explicitly verify that MPC connection attempts towards a port-based signal endpoint fail with a reset. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- .../testing/selftests/net/mptcp/mptcp_join.sh | 80 ++++++++++++++----- 1 file changed, 58 insertions(+), 22 deletions(-)