netdev - Re: [PATCH net-next v2 1/3] net: Add a bhash2 table hashed by port + address

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 15 Jul 2022 11:20:08 -0700
From:   Joanne Koong <joannelkoong@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 1/3] net: Add a bhash2 table hashed by port + address

On Thu, Jul 14, 2022 at 2:08 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Tue, 2022-07-12 at 16:53 -0700, Joanne Koong wrote:
> > @@ -238,12 +331,23 @@ inet_csk_find_open_port(struct sock *sk, struct inet_bind_bucket **tb_ret, int *
> >                       continue;
> >               head = &hinfo->bhash[inet_bhashfn(net, port,
> >                                                 hinfo->bhash_size)];
> > +             head2 = inet_bhashfn_portaddr(hinfo, sk, net, port);
> > +
> >               spin_lock_bh(&head->lock);
> > +
> > +             if (inet_use_bhash2_on_bind(sk)) {
> > +                     if (inet_bhash2_addr_any_conflict(sk, port, l3mdev, relax, false))
> > +                             goto next_port;
> > +             }
> > +
> > +             spin_lock(&head2->lock);
>
> Minor nit: it looks like you can compute hash2 but not use it if the
> inet_bhash2_addr_any_conflict() call above is unsuccesful. You can move
> the inet_bhashfn_portaddr() down.
I will move this down.
>
>
> [...]
>
> > @@ -675,6 +785,112 @@ void inet_unhash(struct sock *sk)
> >  }
> >  EXPORT_SYMBOL_GPL(inet_unhash);
> >
> > +static bool inet_bind2_bucket_match(const struct inet_bind2_bucket *tb,
> > +                                 const struct net *net, unsigned short port,
> > +                                 int l3mdev, const struct sock *sk)
> > +{
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +     if (sk->sk_family == AF_INET6)
> > +             return net_eq(ib2_net(tb), net) && tb->port == port &&
> > +                     tb->l3mdev == l3mdev &&
> > +                     ipv6_addr_equal(&tb->v6_rcv_saddr, &sk->sk_v6_rcv_saddr);
> > +     else
> > +#endif
> > +             return net_eq(ib2_net(tb), net) && tb->port == port &&
> > +                     tb->l3mdev == l3mdev && tb->rcv_saddr == sk->sk_rcv_saddr;
> > +}
> > +
> > +bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const struct net *net,
> > +                                   unsigned short port, int l3mdev, const struct sock *sk)
> > +{
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +     struct in6_addr addr_any = {};
> > +
> > +     if (sk->sk_family == AF_INET6)
> > +             return net_eq(ib2_net(tb), net) && tb->port == port &&
> > +                     tb->l3mdev == l3mdev &&
> > +                     ipv6_addr_equal(&tb->v6_rcv_saddr, &addr_any);
> > +     else
> > +#endif
> > +             return net_eq(ib2_net(tb), net) && tb->port == port &&
> > +                     tb->l3mdev == l3mdev && tb->rcv_saddr == 0;
> > +}
> > +
> > +/* The socket's bhash2 hashbucket spinlock must be held when this is called */
> > +struct inet_bind2_bucket *
> > +inet_bind2_bucket_find(const struct inet_bind_hashbucket *head, const struct net *net,
> > +                    unsigned short port, int l3mdev, const struct sock *sk)
> > +{
> > +     struct inet_bind2_bucket *bhash2 = NULL;
> > +
> > +     inet_bind_bucket_for_each(bhash2, &head->chain)
> > +             if (inet_bind2_bucket_match(bhash2, net, port, l3mdev, sk))
> > +                     break;
> > +
> > +     return bhash2;
> > +}
> > +
> > +struct inet_bind_hashbucket *
> > +inet_bhash2_addr_any_hashbucket(const struct sock *sk, const struct net *net, int port)
> > +{
> > +     struct inet_hashinfo *hinfo = sk->sk_prot->h.hashinfo;
> > +     u32 hash;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +     struct in6_addr addr_any = {};
> > +
> > +     if (sk->sk_family == AF_INET6)
> > +             hash = ipv6_portaddr_hash(net, &addr_any, port);
> > +     else
> > +#endif
> > +             hash = ipv4_portaddr_hash(net, 0, port);
> > +
> > +     return &hinfo->bhash2[hash & (hinfo->bhash_size - 1)];
> > +}
> > +
> > +int inet_bhash2_update_saddr(struct inet_bind_hashbucket *prev_saddr, struct sock *sk)
> > +{
> > +     struct inet_hashinfo *hinfo = sk->sk_prot->h.hashinfo;
> > +     struct inet_bind_hashbucket *head, *head2;
> > +     struct inet_bind2_bucket *tb2, *new_tb2;
> > +     int l3mdev = inet_sk_bound_l3mdev(sk);
> > +     int port = inet_sk(sk)->inet_num;
> > +     struct net *net = sock_net(sk);
> > +
> > +     /* Allocate a bind2 bucket ahead of time to avoid permanently putting
> > +      * the bhash2 table in an inconsistent state if a new tb2 bucket
> > +      * allocation fails.
> > +      */
> > +     new_tb2 = kmem_cache_alloc(hinfo->bind2_bucket_cachep, GFP_ATOMIC);
> > +     if (!new_tb2)
> > +             return -ENOMEM;
> > +
> > +     head = &hinfo->bhash[inet_bhashfn(net, port,
> > +                                       hinfo->bhash_size)];
>
> Here 'head' is unused, you can avoid computing the related hash.
>
Ah yes,  you're right. We don't need head here since we already pass
in prev_saddr. Thanks for catching this, I will remove this.
>
> Cheers,
>
> Paolo
Thanks for taking a look, Paolo!
>