diff mbox series

[mptcp-net,v2,2/2] selftests: mptcp: join: add test-case for MPC attempt towards signl ep

Message ID 20241007-mpc-hs-port-v2-2-0c9e7827bd0f@kernel.org (mailing list archive)
State Superseded, archived
Headers show
Series mptcp: prevent MPC handshake on port-based signal endpoints | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch warning total: 0 errors, 3 warnings, 0 checks, 154 lines checked
matttbe/shellcheck success No ShellCheck issues
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf__only_bpftest_all_ success Success! ✅

Commit Message

Matthieu Baerts (NGI0) Oct. 7, 2024, 6:22 p.m. UTC 
From: Paolo Abeni <pabeni@redhat.com>

Explicitly verify that MPC connection attempts towards a port-based
signal endpoint fail with a reset.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 110 +++++++++++++++++++-----
 1 file changed, 87 insertions(+), 23 deletions(-)

Comments

Matthieu Baerts (NGI0) Oct. 8, 2024, 9:42 a.m. UTC | #1 
Hi Paolo,

(I guess you dropped the MPTCP ML from Cc by mistake, re-adding it)

On 08/10/2024 08:47, Paolo Abeni wrote:
> On 10/7/24 20:22, Matthieu Baerts (NGI0) wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>>
>> Explicitly verify that MPC connection attempts towards a port-based
>> signal endpoint fail with a reset.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Please put your co-dev tag above.

Sure, thank you, I can add that when applying the patch.

> [...]
>> @@ -895,32 +933,16 @@ do_transfer()
>>       local srv_proto="$4"
>>       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"}
>> +    local port
>> +    port=$(get_port)
> 
> At this point I would respect the reverse xmas tree order ;)

:)

Yes, probably best to move 'local port' above. Just to fix a
non-interesting warning from shellcheck.

> @@ -2963,6 +3008,25 @@ add_addr_ports_tests()
>>           chk_join_nr 2 2 2
>>           chk_add_nr 2 2 2
>>       fi
>> +
>> +    if reset "port-based signal endpoint must not accept mpc"; then
>> +        local port ret count
>> +        port=$(get_port)
>> +
>> +        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
>> +        ret=$?
>> +
>> +        cond_stop_capture
>> +        cat "$capout"
> 
> Perhaps worthy to move cat "$capout" into cond_stop_capture()

I should have mentioned that in the changelog: I wanted to, but then it
would change the behaviour for the other cases in do_transfer() where
'cat "$capout"' is used in 3 different places. I can suggest a patch for
net-next, not to add more complexity in this patch if that's OK.

Cheers,
Matt
Paolo Abeni Oct. 8, 2024, 2:20 p.m. UTC | #2 
On 10/8/24 11:42, Matthieu Baerts wrote:
>> @@ -2963,6 +3008,25 @@ add_addr_ports_tests()
>>>            chk_join_nr 2 2 2
>>>            chk_add_nr 2 2 2
>>>        fi
>>> +
>>> +    if reset "port-based signal endpoint must not accept mpc"; then
>>> +        local port ret count
>>> +        port=$(get_port)
>>> +
>>> +        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
>>> +        ret=$?
>>> +
>>> +        cond_stop_capture
>>> +        cat "$capout"
>>
>> Perhaps worthy to move cat "$capout" into cond_stop_capture()
> 
> I should have mentioned that in the changelog: I wanted to, but then it
> would change the behaviour for the other cases in do_transfer() where
> 'cat "$capout"' is used in 3 different places. 

Why would that change the behavior? AFAICS the 'cat "$capout" statement 
happend onece per do_transfer() invocation, just before 'return'. Moving 
it into cond_stop_capture would just reduce code duplication.

/P
Matthieu Baerts (NGI0) Oct. 8, 2024, 3:08 p.m. UTC | #3 
On 08/10/2024 16:20, Paolo Abeni wrote:
> On 10/8/24 11:42, Matthieu Baerts wrote:
>>> @@ -2963,6 +3008,25 @@ add_addr_ports_tests()
>>>>            chk_join_nr 2 2 2
>>>>            chk_add_nr 2 2 2
>>>>        fi
>>>> +
>>>> +    if reset "port-based signal endpoint must not accept mpc"; then
>>>> +        local port ret count
>>>> +        port=$(get_port)
>>>> +
>>>> +        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
>>>> +        ret=$?
>>>> +
>>>> +        cond_stop_capture
>>>> +        cat "$capout"
>>>
>>> Perhaps worthy to move cat "$capout" into cond_stop_capture()
>>
>> I should have mentioned that in the changelog: I wanted to, but then it
>> would change the behaviour for the other cases in do_transfer() where
>> 'cat "$capout"' is used in 3 different places. 
> 
> Why would that change the behavior? AFAICS the 'cat "$capout" statement
> happend onece per do_transfer() invocation, just before 'return'. Moving
> it into cond_stop_capture would just reduce code duplication.

Yes, but the info about the capture will be printed earlier. Will
tcpdump still have time to flush all the last info into this file,
before the 'cat'? I guess that's not a big deal to do that, and if there
are conflicts when doing backports, we can also drop the patch from the
backports.

Also, talking about backports, should we add the same 'Fixes' tag as for
the previous patch, and add this in the commit message?

  The 'Fixes' tag here below is the same as the one from the previous
  commit: this patch here is not fixing anything wrong in the selftests,
  but it validates the previous fix for an issue introduced by this
  commit ID.


Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e8d0a01b4144264615d92b953a69ebd934ce468e..518b81004df9d0e4568b88782836798e197f1427 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,43 @@  check_cestab()
 	fi
 }
 
+cond_start_capture()
+{
+	local ns="$1"
+
+	:> "$capout"
+
+	if $capture; then
+		local capuser capfile
+		if [ -z $SUDO_USER ]; then
+			capuser=""
+		else
+			capuser="-Z $SUDO_USER"
+		fi
+
+		capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "$ns")
+
+		echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile"
+		ip netns exec "$ns" 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
+}
+
+get_port()
+{
+	echo "$((10000 + MPTCP_LIB_TEST_COUNTER - 1))"
+}
+
 do_transfer()
 {
 	local listener_ns="$1"
@@ -895,32 +933,16 @@  do_transfer()
 	local srv_proto="$4"
 	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"}
+	local port
+	port=$(get_port)
 
 	:> "$cout"
 	:> "$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,10 +1029,7 @@  do_transfer()
 	wait $spid
 	local rets=$?
 
-	if $capture; then
-	    sleep 1
-	    kill $cappid
-	fi
+	cond_stop_capture
 
 	NSTAT_HISTORY=/tmp/${listener_ns}.nstat ip netns exec ${listener_ns} \
 		nstat | grep Tcp > /tmp/${listener_ns}.out
@@ -2873,6 +2892,32 @@  verify_listener_events()
 	fail_test
 }
 
+chk_mpc_endp_attempt()
+{
+	local ret=$1
+	local attempts=$2
+
+	print_check "Connect"
+
+	if [ ${ret} = 124 ]; then
+		fail_test "timeout on connect"
+	elif [ ${ret} = 0 ]; then
+		fail_test "unexpected successful connect"
+	else
+		print_ok
+
+		print_check "Attempts"
+		count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPCapableEndpAttempt")
+		if [ -z "$count" ]; then
+			print_skip
+		elif [ "$count" != "$attempts" ]; then
+			fail_test "got ${count} MPC attempt[s] on port-based endpoint, expected $attempts"
+		else
+			print_ok
+		fi
+	fi
+}
+
 add_addr_ports_tests()
 {
 	# signal address with port
@@ -2963,6 +3008,25 @@  add_addr_ports_tests()
 		chk_join_nr 2 2 2
 		chk_add_nr 2 2 2
 	fi
+
+	if reset "port-based signal endpoint must not accept mpc"; then
+		local port ret count
+		port=$(get_port)
+
+		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
+		ret=$?
+
+		cond_stop_capture
+		cat "$capout"
+
+		chk_mpc_endp_attempt ${ret} 1
+	fi
 }
 
 syncookies_tests()