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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Tue, 2025-10-14 at 11:57 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 14/10/2025 08:46, Geliang Tang wrote:
> > It took me some time to understand this test.
> Is there something I should add in the commit message to make this
> clearer?
> 
> > 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.
> 
> If I don't add 1, I will not include '\0' in the comparison with "a".
> 
> In other words, if you have:
> 
>   cfg_host = "abc";
>   a = "abc123";
>   len = strlen(cfg_host); /* = 3 */
> 
> Then strncmp(cfg_host, a, len) will return 0 because the 3 first
> chars
> are "abc". With 4 chars, they are different: "abc\0" vs "abc1".
> 
> So it is important to take the delimiter into account, just in case
> one
> is the prefix of the other one.

Sure, thanks for your explanation. I have no other comments, let's
apply this set.

Thanks,
-Geliang

> 
> Cheers,
> Matt