[PATCH v1 net 0/2] tcp: Fix bind() regression for dual-stack wildcard address.

[PATCH v1 net 0/2] tcp: Fix bind() regression for dual-stack wildcard address.

[Kuniyuki Iwashima]

The first patch fixes the regression reported in [0], and the second
patch adds a test for similar cases to catch future regression.

[0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/


Kuniyuki Iwashima (2):
  tcp: Fix bind() conflict check for dual-stack wildcard address.
  selftest: Add test for bind() conflicts.

 net/ipv4/inet_hashtables.c                  |   8 +-
 tools/testing/selftests/net/.gitignore      |   1 +
 tools/testing/selftests/net/Makefile        |   1 +
 tools/testing/selftests/net/bind_wildcard.c | 114 ++++++++++++++++++++
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/bind_wildcard.c

-- 
2.30.2

[PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Kuniyuki Iwashima]

Paul Holzinger reported [0] that commit 5456262d2baa ("net: Fix
incorrect address comparison when searching for a bind2 bucket")
introduced a bind() regression.  Paul also gave a nice repro that
calls two types of bind() on the same port, both of which now
succeed, but the second call should fail:

  bind(fd1, ::, port) + bind(fd2, 127.0.0.1, port)

The cited commit added address family tests in three functions to
fix the uninit-value KMSAN report. [1]  However, the test added to
inet_bind2_bucket_match_addr_any() removed a necessary conflict
check; the dual-stack wildcard address no longer conflicts with
an IPv4 non-wildcard address.

If tb->family is AF_INET6 and sk->sk_family is AF_INET in
inet_bind2_bucket_match_addr_any(), we still need to check
if tb has the dual-stack wildcard address.

Note that the IPv4 wildcard address does not conflict with
IPv6 non-wildcard addresses.

[0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
[1]: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/

Fixes: 5456262d2baa ("net: Fix incorrect address comparison when searching for a bind2 bucket")
Reported-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Martin KaFai Lau <martin.lau@kernel.org>
---
Some cleanup patches will be posted against net-next later:

  * s/addr_any/in6addr_any/
  * Remove duplicated tests for net, port, and l3mdev.
---
 net/ipv4/inet_hashtables.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index e41fdc38ce19..6edae3886885 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -828,8 +828,14 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const
 #if IS_ENABLED(CONFIG_IPV6)
 	struct in6_addr addr_any = {};
 
-	if (sk->sk_family != tb->family)
+	if (sk->sk_family != tb->family) {
+		if (sk->sk_family == AF_INET)
+			return net_eq(ib2_net(tb), net) && tb->port == port &&
+				tb->l3mdev == l3mdev &&
+				ipv6_addr_equal(&tb->v6_rcv_saddr, &addr_any);
+
 		return false;
+	}
 
 	if (sk->sk_family == AF_INET6)
 		return net_eq(ib2_net(tb), net) && tb->port == port &&
-- 
2.30.2

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Linux regression tracking (Thorsten Leemhuis)]

Thx for working on this. There is one small detail to improve:

On 12.03.23 04:19, Kuniyuki Iwashima wrote:
> Paul Holzinger reported [0] that commit 5456262d2baa ("net: Fix
> incorrect address comparison when searching for a bind2 bucket")
> introduced a bind() regression.  Paul also gave a nice repro that
> calls two types of bind() on the same port, both of which now
> succeed, but the second call should fail:
> 
>   bind(fd1, ::, port) + bind(fd2, 127.0.0.1, port)
> 
> The cited commit added address family tests in three functions to
> fix the uninit-value KMSAN report. [1]  However, the test added to
> inet_bind2_bucket_match_addr_any() removed a necessary conflict
> check; the dual-stack wildcard address no longer conflicts with
> an IPv4 non-wildcard address.
> 
> If tb->family is AF_INET6 and sk->sk_family is AF_INET in
> inet_bind2_bucket_match_addr_any(), we still need to check
> if tb has the dual-stack wildcard address.
> 
> Note that the IPv4 wildcard address does not conflict with
> IPv6 non-wildcard addresses.
> 
> [0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> [1]: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
> 
> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when searching for a bind2 bucket")
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

The links above should use proper link tags, this thus ideally should
look something like this:

Link:
https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
[1]
Fixes: 5456262d2baa ("net: Fix incorrect address comparison when
searching for a bind2 bucket")
Reported-by: Paul Holzinger <pholzing@redhat.com>
Link:
https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
[0]

[placing the link [1] at the end would be fine, too]

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

P.S.: While at it:

#regzbot ^backmonitor:
https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Jakub Kicinski]

On Sun, 12 Mar 2023 12:42:48 +0100 Linux regression tracking (Thorsten
Leemhuis) wrote:
> Link:
> https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
> [1]
> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when
> searching for a bind2 bucket")
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Link:
> https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> [0]

I tried to fix this manually when applying but:
 - your email client wraps your replies
 - please don't reply to patches with tags which will look to scripts
   and patchwork like tags it should pull into the submission
   (Reported-by in particular, here)

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Thorsten Leemhuis]

On 15.03.23 08:26, Jakub Kicinski wrote:
> On Sun, 12 Mar 2023 12:42:48 +0100 Linux regression tracking (Thorsten
> Leemhuis) wrote:
>> Link:
>> https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
>> [1]
>> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when
>> searching for a bind2 bucket")
>> Reported-by: Paul Holzinger <pholzing@redhat.com>
>> Link:
>> https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
>> [0]
> 
> I tried to fix this manually when applying but:
>  - your email client wraps your replies
>  - please don't reply to patches with tags which will look to scripts
>    and patchwork like tags it should pull into the submission
>    (Reported-by in particular, here)

Sorry for the mixup and thx for letting me know, will simply quote my
suggestion next time, that should avoid both problems.

Ciao, Thorsten

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Jakub Kicinski]

On Wed, 15 Mar 2023 09:05:29 +0100 Thorsten Leemhuis wrote:
> On 15.03.23 08:26, Jakub Kicinski wrote:
> > On Sun, 12 Mar 2023 12:42:48 +0100 Linux regression tracking (Thorsten
> > Leemhuis) wrote:  
> >> Link:
> >> https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
> >> [1]
> >> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when
> >> searching for a bind2 bucket")
> >> Reported-by: Paul Holzinger <pholzing@redhat.com>
> >> Link:
> >> https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> >> [0]  
> > 
> > I tried to fix this manually when applying but:
> >  - your email client wraps your replies
> >  - please don't reply to patches with tags which will look to scripts
> >    and patchwork like tags it should pull into the submission
> >    (Reported-by in particular, here)  
> 
> Sorry for the mixup and thx for letting me know, will simply quote my
> suggestion next time, that should avoid both problems.

FWIW indenting with spaces would work too, for our scripts at least.
We match on tags only at start of the line.

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Eric Dumazet]

On Sat, Mar 11, 2023 at 7:20 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Paul Holzinger reported [0] that commit 5456262d2baa ("net: Fix
> incorrect address comparison when searching for a bind2 bucket")
> introduced a bind() regression.  Paul also gave a nice repro that
> calls two types of bind() on the same port, both of which now
> succeed, but the second call should fail:
>
>   bind(fd1, ::, port) + bind(fd2, 127.0.0.1, port)
>
> The cited commit added address family tests in three functions to
> fix the uninit-value KMSAN report. [1]  However, the test added to
> inet_bind2_bucket_match_addr_any() removed a necessary conflict
> check; the dual-stack wildcard address no longer conflicts with
> an IPv4 non-wildcard address.
>
> If tb->family is AF_INET6 and sk->sk_family is AF_INET in
> inet_bind2_bucket_match_addr_any(), we still need to check
> if tb has the dual-stack wildcard address.
>
> Note that the IPv4 wildcard address does not conflict with
> IPv6 non-wildcard addresses.
>
> [0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> [1]: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
>
> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when searching for a bind2 bucket")
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
>

SGTM, thanks.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Paul Holzinger]

Thanks Kuniyuki, patch works for me.

On 12/03/2023 04:19, Kuniyuki Iwashima wrote:
> Paul Holzinger reported [0] that commit 5456262d2baa ("net: Fix
> incorrect address comparison when searching for a bind2 bucket")
> introduced a bind() regression.  Paul also gave a nice repro that
> calls two types of bind() on the same port, both of which now
> succeed, but the second call should fail:
>
>    bind(fd1, ::, port) + bind(fd2, 127.0.0.1, port)
>
> The cited commit added address family tests in three functions to
> fix the uninit-value KMSAN report. [1]  However, the test added to
> inet_bind2_bucket_match_addr_any() removed a necessary conflict
> check; the dual-stack wildcard address no longer conflicts with
> an IPv4 non-wildcard address.
>
> If tb->family is AF_INET6 and sk->sk_family is AF_INET in
> inet_bind2_bucket_match_addr_any(), we still need to check
> if tb has the dual-stack wildcard address.
>
> Note that the IPv4 wildcard address does not conflict with
> IPv6 non-wildcard addresses.
>
> [0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> [1]: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
>
> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when searching for a bind2 bucket")
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Cc: Martin KaFai Lau <martin.lau@kernel.org>
> ---
> Some cleanup patches will be posted against net-next later:
>
>    * s/addr_any/in6addr_any/
>    * Remove duplicated tests for net, port, and l3mdev.
> ---
>   net/ipv4/inet_hashtables.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index e41fdc38ce19..6edae3886885 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -828,8 +828,14 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const
>   #if IS_ENABLED(CONFIG_IPV6)
>   	struct in6_addr addr_any = {};
>   
> -	if (sk->sk_family != tb->family)
> +	if (sk->sk_family != tb->family) {
> +		if (sk->sk_family == AF_INET)
> +			return net_eq(ib2_net(tb), net) && tb->port == port &&
> +				tb->l3mdev == l3mdev &&
> +				ipv6_addr_equal(&tb->v6_rcv_saddr, &addr_any);
> +
>   		return false;
> +	}
>   
>   	if (sk->sk_family == AF_INET6)
>   		return net_eq(ib2_net(tb), net) && tb->port == port &&
Tested-by: Paul Holzinger <pholzing@redhat.com>

Re: [PATCH v1 net 1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.

[Martin KaFai Lau]

On 3/11/23 7:19 PM, Kuniyuki Iwashima wrote:
> Paul Holzinger reported [0] that commit 5456262d2baa ("net: Fix
> incorrect address comparison when searching for a bind2 bucket")
> introduced a bind() regression.  Paul also gave a nice repro that
> calls two types of bind() on the same port, both of which now
> succeed, but the second call should fail:
> 
>    bind(fd1, ::, port) + bind(fd2, 127.0.0.1, port)
> 
> The cited commit added address family tests in three functions to
> fix the uninit-value KMSAN report. [1]  However, the test added to
> inet_bind2_bucket_match_addr_any() removed a necessary conflict
> check; the dual-stack wildcard address no longer conflicts with
> an IPv4 non-wildcard address.
> 
> If tb->family is AF_INET6 and sk->sk_family is AF_INET in
> inet_bind2_bucket_match_addr_any(), we still need to check
> if tb has the dual-stack wildcard address.
> 
> Note that the IPv4 wildcard address does not conflict with
> IPv6 non-wildcard addresses.
> 
> [0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> [1]: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
> 
> Fixes: 5456262d2baa ("net: Fix incorrect address comparison when searching for a bind2 bucket")
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks for the fix.

Reviewed-by: Martin KaFai Lau <martin.lau@kernel.org>

[PATCH v1 net 2/2] selftest: Add test for bind() conflicts.

[Kuniyuki Iwashima]

The test checks if (IPv4, IPv6) address pair properly conflict or not.

  * IPv4
    * 0.0.0.0
    * 127.0.0.1

  * IPv6
    * ::
    * ::1

If the IPv6 address is [::], the second bind() always fails.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 tools/testing/selftests/net/.gitignore      |   1 +
 tools/testing/selftests/net/Makefile        |   1 +
 tools/testing/selftests/net/bind_wildcard.c | 114 ++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 tools/testing/selftests/net/bind_wildcard.c

diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index a6911cae368c..80f06aa62034 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 bind_bhash
 bind_timewait
+bind_wildcard
 csum
 cmsg_sender
 diag_uid
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 6cd8993454d7..80fbfe0330f6 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -80,6 +80,7 @@ TEST_GEN_FILES += sctp_hello
 TEST_GEN_FILES += csum
 TEST_GEN_FILES += nat6to4.o
 TEST_GEN_FILES += ip_local_port_range
+TEST_GEN_FILES += bind_wildcard
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/net/bind_wildcard.c b/tools/testing/selftests/net/bind_wildcard.c
new file mode 100644
index 000000000000..58edfc15d28b
--- /dev/null
+++ b/tools/testing/selftests/net/bind_wildcard.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Amazon.com Inc. or its affiliates. */
+
+#include <sys/socket.h>
+#include <netinet/in.h>
+
+#include "../kselftest_harness.h"
+
+FIXTURE(bind_wildcard)
+{
+	struct sockaddr_in addr4;
+	struct sockaddr_in6 addr6;
+	int expected_errno;
+};
+
+FIXTURE_VARIANT(bind_wildcard)
+{
+	const __u32 addr4_const;
+	const struct in6_addr *addr6_const;
+};
+
+FIXTURE_VARIANT_ADD(bind_wildcard, v4_any_v6_any)
+{
+	.addr4_const = INADDR_ANY,
+	.addr6_const = &in6addr_any,
+};
+
+FIXTURE_VARIANT_ADD(bind_wildcard, v4_any_v6_local)
+{
+	.addr4_const = INADDR_ANY,
+	.addr6_const = &in6addr_loopback,
+};
+
+FIXTURE_VARIANT_ADD(bind_wildcard, v4_local_v6_any)
+{
+	.addr4_const = INADDR_LOOPBACK,
+	.addr6_const = &in6addr_any,
+};
+
+FIXTURE_VARIANT_ADD(bind_wildcard, v4_local_v6_local)
+{
+	.addr4_const = INADDR_LOOPBACK,
+	.addr6_const = &in6addr_loopback,
+};
+
+FIXTURE_SETUP(bind_wildcard)
+{
+	self->addr4.sin_family = AF_INET;
+	self->addr4.sin_port = htons(0);
+	self->addr4.sin_addr.s_addr = htonl(variant->addr4_const);
+
+	self->addr6.sin6_family = AF_INET6;
+	self->addr6.sin6_port = htons(0);
+	self->addr6.sin6_addr = *variant->addr6_const;
+
+	if (variant->addr6_const == &in6addr_any)
+		self->expected_errno = EADDRINUSE;
+	else
+		self->expected_errno = 0;
+}
+
+FIXTURE_TEARDOWN(bind_wildcard)
+{
+}
+
+void bind_sockets(struct __test_metadata *_metadata,
+		  FIXTURE_DATA(bind_wildcard) *self,
+		  struct sockaddr *addr1, socklen_t addrlen1,
+		  struct sockaddr *addr2, socklen_t addrlen2)
+{
+	int fd[2];
+	int ret;
+
+	fd[0] = socket(addr1->sa_family, SOCK_STREAM, 0);
+	ASSERT_GT(fd[0], 0);
+
+	ret = bind(fd[0], addr1, addrlen1);
+	ASSERT_EQ(ret, 0);
+
+	ret = getsockname(fd[0], addr1, &addrlen1);
+	ASSERT_EQ(ret, 0);
+
+	((struct sockaddr_in *)addr2)->sin_port = ((struct sockaddr_in *)addr1)->sin_port;
+
+	fd[1] = socket(addr2->sa_family, SOCK_STREAM, 0);
+	ASSERT_GT(fd[1], 0);
+
+	ret = bind(fd[1], addr2, addrlen2);
+	if (self->expected_errno) {
+		ASSERT_EQ(ret, -1);
+		ASSERT_EQ(errno, self->expected_errno);
+	} else {
+		ASSERT_EQ(ret, 0);
+	}
+
+	close(fd[1]);
+	close(fd[0]);
+}
+
+TEST_F(bind_wildcard, v4_v6)
+{
+	bind_sockets(_metadata, self,
+		     (struct sockaddr *)&self->addr4, sizeof(self->addr6),
+		     (struct sockaddr *)&self->addr6, sizeof(self->addr6));
+}
+
+TEST_F(bind_wildcard, v6_v4)
+{
+	bind_sockets(_metadata, self,
+		     (struct sockaddr *)&self->addr6, sizeof(self->addr6),
+		     (struct sockaddr *)&self->addr4, sizeof(self->addr4));
+}
+
+TEST_HARNESS_MAIN
-- 
2.30.2

Re: [PATCH v1 net 0/2] tcp: Fix bind() regression for dual-stack wildcard address.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 11 Mar 2023 19:19:02 -0800 you wrote:
> The first patch fixes the regression reported in [0], and the second
> patch adds a test for similar cases to catch future regression.
> 
> [0]: https://lore.kernel.org/netdev/e21bf153-80b0-9ec0-15ba-e04a4ad42c34@redhat.com/
> 
> 
> Kuniyuki Iwashima (2):
>   tcp: Fix bind() conflict check for dual-stack wildcard address.
>   selftest: Add test for bind() conflicts.
> 
> [...]

Here is the summary with links:
  - [v1,net,1/2] tcp: Fix bind() conflict check for dual-stack wildcard address.
    https://git.kernel.org/netdev/net/c/d9ba99342855
  - [v1,net,2/2] selftest: Add test for bind() conflicts.
    https://git.kernel.org/netdev/net/c/13715acf8ab5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html