* [PATCH 1/3] net: set tb->fast_sk_family
2017-09-18 16:28 [PATCH 0/3] fix reuseaddr regression josef
@ 2017-09-18 16:28 ` josef
2017-09-18 16:28 ` [PATCH 2/3] net: use inet6_rcv_saddr to compare sockets josef
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: josef @ 2017-09-18 16:28 UTC (permalink / raw)
To: davem, netdev, linux-kernel, crobinso, labbott, kernel-team
Cc: Josef Bacik, stable
From: Josef Bacik <jbacik@fb.com>
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 <crobinso@redhat.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] net: use inet6_rcv_saddr to compare sockets
2017-09-18 16:28 [PATCH 0/3] fix reuseaddr regression josef
2017-09-18 16:28 ` [PATCH 1/3] net: set tb->fast_sk_family josef
@ 2017-09-18 16:28 ` josef
2017-09-18 16:28 ` [PATCH 3/3] inet: fix improper empty comparison josef
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: josef @ 2017-09-18 16:28 UTC (permalink / raw)
To: davem, netdev, linux-kernel, crobinso, labbott, kernel-team
Cc: Josef Bacik, stable
From: Josef Bacik <jbacik@fb.com>
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 <crobinso@redhat.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] inet: fix improper empty comparison
2017-09-18 16:28 [PATCH 0/3] fix reuseaddr regression josef
2017-09-18 16:28 ` [PATCH 1/3] net: set tb->fast_sk_family josef
2017-09-18 16:28 ` [PATCH 2/3] net: use inet6_rcv_saddr to compare sockets josef
@ 2017-09-18 16:28 ` josef
2017-09-18 17:44 ` [PATCH 0/3] fix reuseaddr regression Cole Robinson
2017-09-19 20:50 ` David Miller
4 siblings, 0 replies; 8+ messages in thread
From: josef @ 2017-09-18 16:28 UTC (permalink / raw)
To: davem, netdev, linux-kernel, crobinso, labbott, kernel-team
Cc: Josef Bacik, stable
From: Josef Bacik <jbacik@fb.com>
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 <crobinso@redhat.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] fix reuseaddr regression
2017-09-18 16:28 [PATCH 0/3] fix reuseaddr regression josef
` (2 preceding siblings ...)
2017-09-18 16:28 ` [PATCH 3/3] inet: fix improper empty comparison josef
@ 2017-09-18 17:44 ` Cole Robinson
2017-09-19 20:50 ` David Miller
4 siblings, 0 replies; 8+ messages in thread
From: Cole Robinson @ 2017-09-18 17:44 UTC (permalink / raw)
To: josef, davem, netdev, linux-kernel, labbott, kernel-team
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] fix reuseaddr regression
2017-09-18 16:28 [PATCH 0/3] fix reuseaddr regression josef
` (3 preceding siblings ...)
2017-09-18 17:44 ` [PATCH 0/3] fix reuseaddr regression Cole Robinson
@ 2017-09-19 20:50 ` David Miller
2017-09-23 0:28 ` Josef Bacik
4 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2017-09-19 20:50 UTC (permalink / raw)
To: josef; +Cc: netdev, linux-kernel, crobinso, labbott, kernel-team
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] fix reuseaddr regression
2017-09-19 20:50 ` David Miller
@ 2017-09-23 0:28 ` Josef Bacik
0 siblings, 0 replies; 8+ messages in thread
From: Josef Bacik @ 2017-09-23 0:28 UTC (permalink / raw)
To: David Miller; +Cc: josef, netdev, linux-kernel, crobinso, labbott, kernel-team
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
>
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread