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
From: Josef Bacik <[email protected]>
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: [email protected]
Fixes: 637bc8bbe6c0 ("inet: reset tb->fastreuseport when adding a reuseport sk")
Reported-and-tested-by: Cole Robinson <[email protected]>
Signed-off-by: Josef Bacik <[email protected]>
---
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
From: Josef Bacik <[email protected]>
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: [email protected]
Fixes: b9470c27607b ("inet: kill smallest_size and smallest_port")
Reported-by: Cole Robinson <[email protected]>
Signed-off-by: Josef Bacik <[email protected]>
---
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
From: Josef Bacik <[email protected]>
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: [email protected]
Fixes: 637bc8bbe6c0 ("inet: reset tb->fastreuseport when adding a reuseport sk")
Reported-and-tested-by: Cole Robinson <[email protected]>
Signed-off-by: Josef Bacik <[email protected]>
---
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
On 09/18/2017 12:28 PM, [email protected] 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
From: [email protected]
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.
From: Josef Bacik <[email protected]>
Date: Fri, 22 Sep 2017 20:20:05 -0400
> I introduced a regression when reworking the fastreuse port stuff that allows
> bind conflicts to occur once a reuseaddr 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 could you please queue these changes up for -stable, I've run them through
> the net tests and added another test to check for this problem specifically.
Series applied and queued up for -stable, thanks.
On Tue, Sep 19, 2017 at 01:50:56PM -0700, David Miller wrote:
> From: [email protected]
> 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.
>
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