[PATCH 0/3] fix reuseaddr regression

STALE3274d

Revision v1 of 2 in this series.

7 messages, 3 authors, 2017-09-23 · open the first message on its own page

[PATCH 0/3] fix reuseaddr regression

From: josef@toxicpanda.com
Date: 2017-09-18 16:29:08

I introduced a regression when reworking the fastreuse port stuff that allows
bind conflicts to occur once a reuseaddr socket successfully opens on an
existing tb.  The root cause is I reversed an if statement which caused us to
set the tb as if there were no owners on the socket if there were, which
obviously is not correct.

Dave I have follow up patches that will add a selftest for this case and I ran
the other reuseport related tests as well.  These need to go in pretty quickly
as it breaks kvm, I've marked them for stable.  Sorry for the regression,

Josef

[PATCH 2/3] net: use inet6_rcv_saddr to compare sockets

From: josef@toxicpanda.com
Date: 2017-09-18 16:29:11

From: Josef Bacik <redacted>

In ipv6_rcv_saddr_equal() we need to use inet6_rcv_saddr(sk) for the
ipv6 compare with the fast socket information to make sure we're doing
the proper comparisons.

Cc: stable@vger.kernel.org
Fixes: 637bc8bbe6c0 ("inet: reset tb->fastreuseport when adding a reuseport sk")
Reported-and-tested-by: Cole Robinson <redacted>
Signed-off-by: Josef Bacik <redacted>
---
 net/ipv4/inet_connection_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index f87f4805e244..a1bf30438bc5 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -266,7 +266,7 @@ static inline int sk_reuseport_match(struct inet_bind_bucket *tb,
 #if IS_ENABLED(CONFIG_IPV6)
 	if (tb->fast_sk_family == AF_INET6)
 		return ipv6_rcv_saddr_equal(&tb->fast_v6_rcv_saddr,
-					    &sk->sk_v6_rcv_saddr,
+					    inet6_rcv_saddr(sk),
 					    tb->fast_rcv_saddr,
 					    sk->sk_rcv_saddr,
 					    tb->fast_ipv6_only,
-- 
2.7.4

[PATCH 3/3] inet: fix improper empty comparison

From: josef@toxicpanda.com
Date: 2017-09-18 16:29:35

From: Josef Bacik <redacted>

When doing my reuseport rework I screwed up and changed a

if (hlist_empty(&tb->owners))

to

if (!hlist_empty(&tb->owners))

This is obviously bad as all of the reuseport/reuse logic was reversed,
which caused weird problems like allowing an ipv4 bind conflict if we
opened an ipv4 only socket on a port followed by an ipv6 only socket on
the same port.

Cc: stable@vger.kernel.org
Fixes: b9470c27607b ("inet: kill smallest_size and smallest_port")
Reported-by: Cole Robinson <redacted>
Signed-off-by: Josef Bacik <redacted>
---
 net/ipv4/inet_connection_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index a1bf30438bc5..c039c937ba90 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -321,7 +321,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 			goto fail_unlock;
 	}
 success:
-	if (!hlist_empty(&tb->owners)) {
+	if (hlist_empty(&tb->owners)) {
 		tb->fastreuse = reuse;
 		if (sk->sk_reuseport) {
 			tb->fastreuseport = FASTREUSEPORT_ANY;
-- 
2.7.4

[PATCH 1/3] net: set tb->fast_sk_family

From: josef@toxicpanda.com
Date: 2017-09-18 16:30:14

From: Josef Bacik <redacted>

We need to set the tb->fast_sk_family properly so we can use the proper
comparison function for all subsequent reuseport bind requests.

Cc: stable@vger.kernel.org
Fixes: 637bc8bbe6c0 ("inet: reset tb->fastreuseport when adding a reuseport sk")
Reported-and-tested-by: Cole Robinson <redacted>
Signed-off-by: Josef Bacik <redacted>
---
 net/ipv4/inet_connection_sock.c | 2 ++
 1 file changed, 2 insertions(+)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index b9c64b40a83a..f87f4805e244 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -328,6 +328,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 			tb->fastuid = uid;
 			tb->fast_rcv_saddr = sk->sk_rcv_saddr;
 			tb->fast_ipv6_only = ipv6_only_sock(sk);
+			tb->fast_sk_family = sk->sk_family;
 #if IS_ENABLED(CONFIG_IPV6)
 			tb->fast_v6_rcv_saddr = sk->sk_v6_rcv_saddr;
 #endif
@@ -354,6 +355,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 				tb->fastuid = uid;
 				tb->fast_rcv_saddr = sk->sk_rcv_saddr;
 				tb->fast_ipv6_only = ipv6_only_sock(sk);
+				tb->fast_sk_family = sk->sk_family;
 #if IS_ENABLED(CONFIG_IPV6)
 				tb->fast_v6_rcv_saddr = sk->sk_v6_rcv_saddr;
 #endif
-- 
2.7.4

Re: [PATCH 0/3] fix reuseaddr regression

From: Cole Robinson <hidden>
Date: 2017-09-18 17:44:36

On 09/18/2017 12:28 PM, josef@toxicpanda.com wrote:
I introduced a regression when reworking the fastreuse port stuff that allows
bind conflicts to occur once a reuseaddr socket successfully opens on an
existing tb.  The root cause is I reversed an if statement which caused us to
set the tb as if there were no owners on the socket if there were, which
obviously is not correct.

Dave I have follow up patches that will add a selftest for this case and I ran
the other reuseport related tests as well.  These need to go in pretty quickly
as it breaks kvm, I've marked them for stable.  Sorry for the regression,
To clarify, it doesn't really break KVM specifically, but it breaks a
port collision detection idiom that libvirt depends on to successfully
launch qemu/xen/... VMs in certain cases.

Thanks,
Cole

Re: [PATCH 0/3] fix reuseaddr regression

From: David Miller <davem@davemloft.net>
Date: 2017-09-19 20:50:59

From: josef@toxicpanda.com
Date: Mon, 18 Sep 2017 12:28:54 -0400
I introduced a regression when reworking the fastreuse port stuff that allows
bind conflicts to occur once a reuseaddr socket successfully opens on an
existing tb.  The root cause is I reversed an if statement which caused us to
set the tb as if there were no owners on the socket if there were, which
obviously is not correct.

Dave I have follow up patches that will add a selftest for this case and I ran
the other reuseport related tests as well.  These need to go in pretty quickly
as it breaks kvm, I've marked them for stable.  Sorry for the regression,
First, please fix your "From: " field so that it actually has your full
name rather than just your email address.  This matter when I apply
your patches.

Second, remove the stable CC:.  For networking changes, you simply ask
me to queue the changes up for -stable.

Thanks.

Re: [PATCH 0/3] fix reuseaddr regression

From: Josef Bacik <josef@toxicpanda.com>
Date: 2017-09-23 00:28:49

On Tue, Sep 19, 2017 at 01:50:56PM -0700, David Miller wrote:
From: josef@toxicpanda.com
Date: Mon, 18 Sep 2017 12:28:54 -0400
quoted
I introduced a regression when reworking the fastreuse port stuff that allows
bind conflicts to occur once a reuseaddr socket successfully opens on an
existing tb.  The root cause is I reversed an if statement which caused us to
set the tb as if there were no owners on the socket if there were, which
obviously is not correct.

Dave I have follow up patches that will add a selftest for this case and I ran
the other reuseport related tests as well.  These need to go in pretty quickly
as it breaks kvm, I've marked them for stable.  Sorry for the regression,
First, please fix your "From: " field so that it actually has your full
name rather than just your email address.  This matter when I apply
your patches.

Second, remove the stable CC:.  For networking changes, you simply ask
me to queue the changes up for -stable.
Sorry Dave, I've fixed my git email settings and I droped the stable cc and sent
a new round.  Didn't see this until just now, my bad.

Josef
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help