* [PATCH v2 net] soreuseport: Fix TCP listener hash collision
@ 2016-04-28 21:07 Craig Gallek
2016-04-28 21:59 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Craig Gallek @ 2016-04-28 21:07 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Craig Gallek <kraig@google.com>
I forgot to include a check for listener port equality when deciding
if two sockets should belong to the same reuseport group. This was
not caught previously because it's only necessary when two listening
sockets for the same user happen to hash to the same listener bucket.
This change also includes a check for network namespace equality.
The same error does not exist in the UDP path.
Fixes: c125e80b8868("soreuseport: fast reuseport TCP socket selection")
Signed-off-by: Craig Gallek <kraig@google.com>
---
v2 Changes
- Suggestions from Eric Dumazet to include network namespace equality
check and to avoid a dreference by simply checking inet_bind_bucket
pointer equality.
---
net/ipv4/inet_hashtables.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index bc68eced0105..5c5658268d5e 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -470,15 +470,19 @@ static int inet_reuseport_add_sock(struct sock *sk,
const struct sock *sk2,
bool match_wildcard))
{
+ struct inet_bind_bucket *tb = inet_csk(sk)->icsk_bind_hash;
+ struct net *net = sock_net(sk);
struct sock *sk2;
struct hlist_nulls_node *node;
kuid_t uid = sock_i_uid(sk);
sk_nulls_for_each_rcu(sk2, node, &ilb->head) {
- if (sk2 != sk &&
+ if (net_eq(sock_net(sk2), net) &&
+ sk2 != sk &&
sk2->sk_family == sk->sk_family &&
ipv6_only_sock(sk2) == ipv6_only_sock(sk) &&
sk2->sk_bound_dev_if == sk->sk_bound_dev_if &&
+ inet_csk(sk2)->icsk_bind_hash == tb &&
sk2->sk_reuseport && uid_eq(uid, sock_i_uid(sk2)) &&
saddr_same(sk, sk2, false))
return reuseport_add_sock(sk, sk2);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 net] soreuseport: Fix TCP listener hash collision
2016-04-28 21:07 [PATCH v2 net] soreuseport: Fix TCP listener hash collision Craig Gallek
@ 2016-04-28 21:59 ` Eric Dumazet
0 siblings, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2016-04-28 21:59 UTC (permalink / raw)
To: Craig Gallek; +Cc: davem, netdev
On Thu, 2016-04-28 at 17:07 -0400, Craig Gallek wrote:
> From: Craig Gallek <kraig@google.com>
>
> I forgot to include a check for listener port equality when deciding
> if two sockets should belong to the same reuseport group. This was
> not caught previously because it's only necessary when two listening
> sockets for the same user happen to hash to the same listener bucket.
> This change also includes a check for network namespace equality.
> The same error does not exist in the UDP path.
>
> Fixes: c125e80b8868("soreuseport: fast reuseport TCP socket selection")
> Signed-off-by: Craig Gallek <kraig@google.com>
> ---
> v2 Changes
> - Suggestions from Eric Dumazet to include network namespace equality
> check and to avoid a dreference by simply checking inet_bind_bucket
> pointer equality.
> ---
> net/ipv4/inet_hashtables.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index bc68eced0105..5c5658268d5e 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -470,15 +470,19 @@ static int inet_reuseport_add_sock(struct sock *sk,
> const struct sock *sk2,
> bool match_wildcard))
> {
> + struct inet_bind_bucket *tb = inet_csk(sk)->icsk_bind_hash;
> + struct net *net = sock_net(sk);
> struct sock *sk2;
> struct hlist_nulls_node *node;
> kuid_t uid = sock_i_uid(sk);
>
> sk_nulls_for_each_rcu(sk2, node, &ilb->head) {
> - if (sk2 != sk &&
> + if (net_eq(sock_net(sk2), net) &&
> + sk2 != sk &&
> sk2->sk_family == sk->sk_family &&
> ipv6_only_sock(sk2) == ipv6_only_sock(sk) &&
> sk2->sk_bound_dev_if == sk->sk_bound_dev_if &&
> + inet_csk(sk2)->icsk_bind_hash == tb &&
> sk2->sk_reuseport && uid_eq(uid, sock_i_uid(sk2)) &&
> saddr_same(sk, sk2, false))
> return reuseport_add_sock(sk, sk2);
Note that I suggested to only use "inet_csk(sk2)->icsk_bind_hash == tb"
If test is true, it means that sockets share same name space and same
port ;)
Therefore the added net_eq(sock_net(sk2), net) test is redundant.
No strong opinion, as this patch works, and this is not fast path
anyway.
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 net] soreuseport: Fix TCP listener hash collision
@ 2016-04-28 22:03 Craig Gallek
0 siblings, 0 replies; 3+ messages in thread
From: Craig Gallek @ 2016-04-28 22:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Thu, Apr 28, 2016 at 5:59 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-04-28 at 17:07 -0400, Craig Gallek wrote:
>> From: Craig Gallek <kraig@google.com>
>>
>> I forgot to include a check for listener port equality when deciding
>> if two sockets should belong to the same reuseport group. This was
>> not caught previously because it's only necessary when two listening
>> sockets for the same user happen to hash to the same listener bucket.
>> This change also includes a check for network namespace equality.
>> The same error does not exist in the UDP path.
>>
>> Fixes: c125e80b8868("soreuseport: fast reuseport TCP socket selection")
>> Signed-off-by: Craig Gallek <kraig@google.com>
>> ---
>> v2 Changes
>> - Suggestions from Eric Dumazet to include network namespace equality
>> check and to avoid a dreference by simply checking inet_bind_bucket
>> pointer equality.
>> ---
>> net/ipv4/inet_hashtables.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index bc68eced0105..5c5658268d5e 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -470,15 +470,19 @@ static int inet_reuseport_add_sock(struct sock *sk,
>> const struct sock *sk2,
>> bool match_wildcard))
>> {
>> + struct inet_bind_bucket *tb = inet_csk(sk)->icsk_bind_hash;
>> + struct net *net = sock_net(sk);
>> struct sock *sk2;
>> struct hlist_nulls_node *node;
>> kuid_t uid = sock_i_uid(sk);
>>
>> sk_nulls_for_each_rcu(sk2, node, &ilb->head) {
>> - if (sk2 != sk &&
>> + if (net_eq(sock_net(sk2), net) &&
>> + sk2 != sk &&
>> sk2->sk_family == sk->sk_family &&
>> ipv6_only_sock(sk2) == ipv6_only_sock(sk) &&
>> sk2->sk_bound_dev_if == sk->sk_bound_dev_if &&
>> + inet_csk(sk2)->icsk_bind_hash == tb &&
>> sk2->sk_reuseport && uid_eq(uid, sock_i_uid(sk2)) &&
>> saddr_same(sk, sk2, false))
>> return reuseport_add_sock(sk, sk2);
>
> Note that I suggested to only use "inet_csk(sk2)->icsk_bind_hash == tb"
>
> If test is true, it means that sockets share same name space and same
> port ;)
>
> Therefore the added net_eq(sock_net(sk2), net) test is redundant.
Thanks for the quick review Eric, sorry I miss read :\ I'll send a v3...
> No strong opinion, as this patch works, and this is not fast path
> anyway.
>
> Acked-by: Eric Dumazet <edumazet@google.com>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-28 22:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-28 21:07 [PATCH v2 net] soreuseport: Fix TCP listener hash collision Craig Gallek
2016-04-28 21:59 ` Eric Dumazet
-- strict thread matches above, loose matches on Subject: below --
2016-04-28 22:03 Craig Gallek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).