netdev - Re: [PATCH v1 net-next 3/6] af_unix: Define a per-netns hash table.

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+JZT22NvQSiOY2X-XmBiOV4kPGohnpSDdjdptkUig6DQ@mail.gmail.com>
Date:   Fri, 17 Jun 2022 06:23:37 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Amit Shah <aams@...zon.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v1 net-next 3/6] af_unix: Define a per-netns hash table.

On Fri, Jun 17, 2022 at 1:48 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> This commit adds a per netns hash table for AF_UNIX.
>
> Note that its size is fixed as UNIX_HASH_SIZE for now.
>

Note: Please include memory costs for this table, including when LOCKDEP is on.


> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>  include/net/af_unix.h    |  5 +++++
>  include/net/netns/unix.h |  2 ++
>  net/unix/af_unix.c       | 40 ++++++++++++++++++++++++++++++++++------
>  3 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index acb56e463db1..0a17e49af0c9 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -24,6 +24,11 @@ extern unsigned int unix_tot_inflight;
>  extern spinlock_t unix_table_locks[UNIX_HASH_SIZE];
>  extern struct hlist_head unix_socket_table[UNIX_HASH_SIZE];
>
> +struct unix_hashbucket {
> +       spinlock_t              lock;
> +       struct hlist_head       head;
> +};
> +
>  struct unix_address {
>         refcount_t      refcnt;
>         int             len;
> diff --git a/include/net/netns/unix.h b/include/net/netns/unix.h
> index 91a3d7e39198..975c4e3f8a5b 100644
> --- a/include/net/netns/unix.h
> +++ b/include/net/netns/unix.h
> @@ -5,8 +5,10 @@
>  #ifndef __NETNS_UNIX_H__
>  #define __NETNS_UNIX_H__
>
> +struct unix_hashbucket;
>  struct ctl_table_header;
>  struct netns_unix {
> +       struct unix_hashbucket  *hash;
>         int                     sysctl_max_dgram_qlen;
>         struct ctl_table_header *ctl;
>  };
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index c0804ae9c96a..3c07702e2349 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -3559,7 +3559,7 @@ static const struct net_proto_family unix_family_ops = {
>
>  static int __net_init unix_net_init(struct net *net)
>  {
> -       int error = -ENOMEM;
> +       int i;
>
>         net->unx.sysctl_max_dgram_qlen = 10;
>         if (unix_sysctl_register(net))
> @@ -3567,18 +3567,35 @@ static int __net_init unix_net_init(struct net *net)
>
>  #ifdef CONFIG_PROC_FS
>         if (!proc_create_net("unix", 0, net->proc_net, &unix_seq_ops,
> -                       sizeof(struct seq_net_private))) {
> -               unix_sysctl_unregister(net);
> -               goto out;
> +                            sizeof(struct seq_net_private)))
> +               goto err_sysctl;
> +#endif
> +
> +       net->unx.hash = kmalloc(sizeof(struct unix_hashbucket) * UNIX_HASH_SIZE,
> +                               GFP_KERNEL);

This will fail under memory pressure.

Prefer kvmalloc_array()

> +       if (!net->unx.hash)
> +               goto err_proc;
> +
> +       for (i = 0; i < UNIX_HASH_SIZE; i++) {
> +               INIT_HLIST_HEAD(&net->unx.hash[i].head);
> +               spin_lock_init(&net->unx.hash[i].lock);
>         }
> +
> +       return 0;
> +
> +err_proc:
> +#ifdef CONFIG_PROC_FS
> +       remove_proc_entry("unix", net->proc_net);
>  #endif
> -       error = 0;
> +err_sysctl:
> +       unix_sysctl_unregister(net);
>  out:
> -       return error;
> +       return -ENOMEM;
>  }
>
>  static void __net_exit unix_net_exit(struct net *net)
>  {
> +       kfree(net->unx.hash);

kvfree()

>         unix_sysctl_unregister(net);
>         remove_proc_entry("unix", net->proc_net);
>  }
> @@ -3666,6 +3683,16 @@ static int __init af_unix_init(void)
>
>         BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof_field(struct sk_buff, cb));
>
> +       init_net.unx.hash = kmalloc(sizeof(struct unix_hashbucket) * UNIX_HASH_SIZE,
> +                                   GFP_KERNEL);

Why are you allocating the hash table twice ? It should be done
already in  unix_net_init() ?

> +       if (!init_net.unx.hash)
> +               goto out;
> +
> +       for (i = 0; i < UNIX_HASH_SIZE; i++) {
> +               INIT_HLIST_HEAD(&init_net.unx.hash[i].head);
> +               spin_lock_init(&init_net.unx.hash[i].lock);
> +       }
> +
>         for (i = 0; i < UNIX_HASH_SIZE; i++)
>                 spin_lock_init(&unix_table_locks[i]);
>
> @@ -3699,6 +3726,7 @@ static void __exit af_unix_exit(void)
>         proto_unregister(&unix_dgram_proto);
>         proto_unregister(&unix_stream_proto);
>         unregister_pernet_subsys(&unix_net_ops);
> +       kfree(init_net.unx.hash);

   Not needed.

>  }
>
>  /* Earlier than device_initcall() so that other drivers invoking
> --
> 2.30.2
>