Re: [PATCH mptcp-next 2/2] selftests: mptcp: join: validate extra bind cases

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

It took me some time to understand this test.

On Thu, 2025-10-09 at 19:33 +0200, Matthieu Baerts (NGI0) wrote:
> By design, an MPTCP connection will not accept extra subflows where
> no
> MPTCP listening sockets can accept such requests.
> 
> In other words, it means that if the 'server' listens on a specific
> address / device, it cannot accept MP_JOIN sent to a different
> address /
> device. Except if there is another MPTCP listening socket accepting
> them.
> 
> This is what the new tests are validating:
> 
>  - Forcing a bind on the main v4/v6 address, and checking that
> MP_JOIN
>    to announced addresses are not accepted.
> 
>  - Also forcing a bind on the main v4/v6 address, but before, another
>    listening socket is created to accept additional subflows. Note
> that
>    'mptcpize run nc -l' -- or something else only doing:
> socket(MPTCP),
>    bind(<IP>), listen(0) -- would be enough, but here mptcp_connect
> is
>    reused not to depend on another tool just for that.
> 
>  - Same as the previous one, but using v6 link-local addresses: this
> is
>    a bit particular because it is required to specify the outgoing
>    network interface when connecting to a link-local address
> announced
>    by the other peer. When using the routing rules, this doesn't work
>    (the outgoing interface is not known) ; but it does work with a
>    'laminar' endpoint having a specified interface.
> 
> Note that extra small modifications are needed for these tests to
> work:
> 
>  - mptcp_connect's check_getpeername_connect() check should strip the
>    specified interface when comparing addresses.
> 
>  - With IPv6 link-local addresses, it is required to wait for them to
>    be ready (no longer in 'tentative' mode) before using them,
> otherwise
>    the bind() will not be allowed.
> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/591
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c |  10 +-
>  tools/testing/selftests/net/mptcp/mptcp_join.sh   | 153
> +++++++++++++++++++++-
>  2 files changed, 161 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index b148cadb96d0..c030b08a7195 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1064,6 +1064,8 @@ static void check_getpeername_connect(int fd)
>  	socklen_t salen = sizeof(ss);
>  	char a[INET6_ADDRSTRLEN];
>  	char b[INET6_ADDRSTRLEN];
> +	const char *iface;
> +	size_t len;
>  
>  	if (getpeername(fd, (struct sockaddr *)&ss, &salen) < 0) {
>  		perror("getpeername");
> @@ -1073,7 +1075,13 @@ static void check_getpeername_connect(int fd)
>  	xgetnameinfo((struct sockaddr *)&ss, salen,
>  		     a, sizeof(a), b, sizeof(b));
>  
> -	if (strcmp(cfg_host, a) || strcmp(cfg_port, b))
> +	iface = strchr(cfg_host, '%');
> +	if (iface)
> +		len = iface - cfg_host;
> +	else
> +		len = strlen(cfg_host) + 1;

Why do we need to add 1 here? I tested it and it works without adding
1.

Other than this minor comment, everything looks good to me. No need for
a v2; it can be modified when applying if needed.

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> +
> +	if (strncmp(cfg_host, a, len) || strcmp(cfg_port, b))
>  		fprintf(stderr, "%s: %s vs %s, %s vs %s\n",
> __func__,
>  			cfg_host, a, cfg_port, b);
>  }
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c5169020a515..e323f81cdc02 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -62,6 +62,7 @@ unset sflags
>  unset fastclose
>  unset fullmesh
>  unset speed
> +unset bind_addr
>  unset join_syn_rej
>  unset join_csum_ns1
>  unset join_csum_ns2
> @@ -645,6 +646,27 @@ wait_mpj()
>  	done
>  }
>  
> +wait_ll_ready()
> +{
> +	local ns="${1}"
> +
> +	local i
> +	for i in $(seq 50); do
> +		ip -n "${ns}" -6 addr show scope link | grep "inet6
> fe80" |
> +			grep -qw "tentative" || break
> +		sleep 0.1
> +	done
> +}
> +
> +get_ll_addr()
> +{
> +	local ns="${1}"
> +	local iface="${2}"
> +
> +	ip -n "${ns}" -6 addr show dev "${iface}" scope link |
> +		grep "inet6 fe80" | sed 's#.*\(fe80::.*\)/.*#\1#'
> +}
> +
>  kill_events_pids()
>  {
>  	mptcp_lib_kill_wait $evts_ns1_pid
> @@ -952,6 +974,7 @@ do_transfer()
>  	local fastclose=${fastclose:-""}
>  	local speed=${speed:-"fast"}
>  	local in="${sin}"
> +	local bind_addr=${bind_addr:-"::"}
>  	port=$(get_port)
>  
>  	:> "$cout"
> @@ -1005,7 +1028,7 @@ do_transfer()
>  	timeout ${timeout_test} \
>  		ip netns exec ${listener_ns} \
>  			./mptcp_connect -t ${timeout_poll} -l -p
> ${port} -s ${srv_proto} \
> -				${extra_srv_args} "::" < "${in}" >
> "${sout}" &
> +				${extra_srv_args} "${bind_addr}" <
> "${in}" > "${sout}" &
>  	local spid=$!
>  
>  	mptcp_lib_wait_local_port_listen "${listener_ns}" "${port}"
> @@ -3230,6 +3253,133 @@ add_addr_ports_tests()
>  	fi
>  }
>  
> +bind_tests()
> +{
> +	# bind to one address should not allow extra subflows to
> other addresses
> +	if reset "bind main address v4, no join v4"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		bind_addr="10.0.1.1" \
> +			run_tests $ns1 $ns2 10.0.1.1
> +		join_syn_tx=1 \
> +			chk_join_nr 0 0 0
> +		chk_add_nr 1 1
> +	fi
> +
> +	# bind to one address should not allow extra subflows to
> other addresses
> +	if reset "bind main address v6, no join v6"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal
> +		bind_addr="dead:beef:1::1" \
> +			run_tests $ns1 $ns2 dead:beef:1::1
> +		join_syn_tx=1 \
> +			chk_join_nr 0 0 0
> +		chk_add_nr 1 1
> +	fi
> +
> +	# multiple binds to allow extra subflows to other addresses
> +	if reset "multiple bind to allow joins v4"; then
> +		local extra_bind
> +
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +
> +		# Launching another app listening on a different
> address
> +		# Note: it could be a totally different app, e.g.
> nc, socat, ...
> +		ip netns exec ${ns1} ./mptcp_connect -l -p
> "$(get_port)" \
> +			-s MPTCP 10.0.2.1 &
> +		extra_bind=$!
> +
> +		bind_addr="10.0.1.1" \
> +			run_tests $ns1 $ns2 10.0.1.1
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +
> +		kill ${extra_bind}
> +	fi
> +
> +	# multiple binds to allow extra subflows to other addresses
> +	if reset "multiple bind to allow joins v6"; then
> +		local extra_bind
> +
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal
> +
> +		# Launching another app listening on a different
> address
> +		# Note: it could be a totally different app, e.g.
> nc, socat, ...
> +		ip netns exec ${ns1} ./mptcp_connect -l -p
> "$(get_port)" \
> +			-s MPTCP dead:beef:2::1 &
> +		extra_bind=$!
> +
> +		bind_addr="dead:beef:1::1" \
> +			run_tests $ns1 $ns2 dead:beef:1::1
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +
> +		kill ${extra_bind}
> +	fi
> +
> +	# multiple binds to allow extra subflows to other addresses:
> v6 LL case
> +	if reset "multiple bind to allow joins v6 link-local
> routing"; then
> +		local extra_bind ns1ll1 ns1ll2
> +
> +		ns1ll1="$(get_ll_addr $ns1 ns1eth1)"
> +		ns1ll2="$(get_ll_addr $ns1 ns1eth2)"
> +
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 "${ns1ll2}" flags signal
> +
> +		wait_ll_ready $ns1 # to be able to bind
> +		wait_ll_ready $ns2 # also needed to bind on the
> client side
> +		ip netns exec ${ns1} ./mptcp_connect -l -p
> "$(get_port)" \
> +			-s MPTCP "${ns1ll2}%ns1eth2" &
> +		extra_bind=$!
> +
> +		bind_addr="${ns1ll1}%ns1eth1" \
> +			run_tests $ns1 $ns2 "${ns1ll1}%ns2eth1"
> +		# it is not possible to connect to the announced LL
> addr without
> +		# specifying the outgoing interface.
> +		join_connect_err=1 \
> +			chk_join_nr 0 0 0
> +		chk_add_nr 1 1
> +
> +		kill ${extra_bind}
> +	fi
> +
> +	# multiple binds to allow extra subflows to v6 LL addresses:
> laminar
> +	if reset "multiple bind to allow joins v6 link-local
> laminar" &&
> +	   continue_if mptcp_lib_kallsyms_has
> "mptcp_pm_get_endp_laminar_max$"; then
> +		local extra_bind ns1ll1 ns1ll2 ns2ll2
> +
> +		ns1ll1="$(get_ll_addr $ns1 ns1eth1)"
> +		ns1ll2="$(get_ll_addr $ns1 ns1eth2)"
> +		ns2ll2="$(get_ll_addr $ns2 ns2eth2)"
> +
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 2 2
> +		pm_nl_add_endpoint $ns1 "${ns1ll2}" flags signal
> +		pm_nl_add_endpoint $ns2 "${ns2ll2}" flags laminar
> dev ns2eth2
> +
> +		wait_ll_ready $ns1 # to be able to bind
> +		wait_ll_ready $ns2 # also needed to bind on the
> client side
> +		ip netns exec ${ns1} ./mptcp_connect -l -p
> "$(get_port)" \
> +			-s MPTCP "${ns1ll2}%ns1eth2" &
> +		extra_bind=$!
> +
> +		bind_addr="${ns1ll1}%ns1eth1" \
> +			run_tests $ns1 $ns2 "${ns1ll1}%ns2eth1"
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +
> +		kill ${extra_bind}
> +	fi
> +}
> +
>  syncookies_tests()
>  {
>  	# single subflow, syncookies
> @@ -4184,6 +4334,7 @@ all_tests_sorted=(
>  	M@mixed_tests
>  	b@backup_tests
>  	p@add_addr_ports_tests
> +	B@bind_tests
>  	k@syncookies_tests
>  	S@checksum_tests
>  	d@deny_join_id0_tests
>