netdev - Re: [RESEND PATCH net-next v7 2/3] inet: Avoid ehash lookup race in inet_ehash

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] [day] [month] [year] [list]

Message-ID: <CANn89iJyKo0xLh5EE1hTJQF8vmqKT-yeK1oWokRpguEdjF-eoA@mail.gmail.com>
Date: Tue, 14 Oct 2025 06:59:39 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: xuanqiang.luo@...ux.dev
Cc: kuniyu@...gle.com, pabeni@...hat.com, kerneljasonxing@...il.com, 
	davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org, 
	horms@...nel.org, jiayuan.chen@...ux.dev, 
	Xuanqiang Luo <luoxuanqiang@...inos.cn>
Subject: Re: [RESEND PATCH net-next v7 2/3] inet: Avoid ehash lookup race in inet_ehash_insert()

On Mon, Oct 13, 2025 at 7:28 PM <xuanqiang.luo@...ux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@...inos.cn>
>
> Since ehash lookups are lockless, if one CPU performs a lookup while
> another concurrently deletes and inserts (removing reqsk and inserting sk),
> the lookup may fail to find the socket, an RST may be sent.
>
> The call trace map is drawn as follows:
>    CPU 0                           CPU 1
>    -----                           -----
>                                 inet_ehash_insert()
>                                 spin_lock()
>                                 sk_nulls_del_node_init_rcu(osk)
> __inet_lookup_established()
>         (lookup failed)
>                                 __sk_nulls_add_node_rcu(sk, list)
>                                 spin_unlock()
>
> As both deletion and insertion operate on the same ehash chain, this patch
> introduces a new sk_nulls_replace_node_init_rcu() helper functions to
> implement atomic replacement.
>
> Fixes: 5e0724d027f0 ("tcp/dccp: fix hashdance race for passive sessions")
> Reviewed-by: Kuniyuki Iwashima <kuniyu@...gle.com>
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@...inos.cn>
> ---
>  include/net/sock.h         | 14 ++++++++++++++
>  net/ipv4/inet_hashtables.c |  8 ++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 60bcb13f045c..5d299a954859 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -858,6 +858,20 @@ static inline bool sk_nulls_del_node_init_rcu(struct sock *sk)
>         return rc;
>  }
>
> +static inline bool sk_nulls_replace_node_init_rcu(struct sock *old,
> +                                                 struct sock *new)
> +{
> +       if (sk_hashed(old)) {
> +               hlist_nulls_replace_init_rcu(&old->sk_nulls_node,
> +                                            &new->sk_nulls_node);
> +               DEBUG_NET_WARN_ON_ONCE(refcount_read(&old->sk_refcnt) == 1);

This  DEBUG_NET_WARN_ON_ONCE() is not needed.

You copied a WARN_ON(refcount_read(&sk->sk_refcnt) == 1) which
predates the conversion
of sk_refcnt from atomic_t to refcount_t

refcount_dec() has a check which will complain just the same.

I will send a patch  removing the leftovers:

diff --git a/include/net/sock.h b/include/net/sock.h
index 60bcb13f045c..596302bffedf 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -828,11 +828,9 @@ static inline bool sk_del_node_init(struct sock *sk)
 {
        bool rc = __sk_del_node_init(sk);

-       if (rc) {
-               /* paranoid for a while -acme */
-               WARN_ON(refcount_read(&sk->sk_refcnt) == 1);
+       if (rc)
                __sock_put(sk);
-       }
+
        return rc;
 }
 #define sk_del_node_init_rcu(sk)       sk_del_node_init(sk)
@@ -850,11 +848,9 @@ static inline bool
sk_nulls_del_node_init_rcu(struct sock *sk)
 {
        bool rc = __sk_nulls_del_node_init_rcu(sk);

-       if (rc) {
-               /* paranoid for a while -acme */
-               WARN_ON(refcount_read(&sk->sk_refcnt) == 1);
+       if (rc)
                __sock_put(sk);
-       }
+
        return rc;
 }

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 2b46c0cd752a..687a84c48882 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -596,10 +596,8 @@ static void netlink_remove(struct sock *sk)

        table = &nl_table[sk->sk_protocol];
        if (!rhashtable_remove_fast(&table->hash, &nlk_sk(sk)->node,
-                                   netlink_rhashtable_params)) {
-               WARN_ON(refcount_read(&sk->sk_refcnt) == 1);
+                                   netlink_rhashtable_params))
                __sock_put(sk);
-       }

        netlink_table_grab();
        if (nlk_sk(sk)->subscriptions) {
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 1574a83384f8..bc614a1f019c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -3031,10 +3031,8 @@ static void tipc_sk_remove(struct tipc_sock *tsk)
        struct sock *sk = &tsk->sk;
        struct tipc_net *tn = net_generic(sock_net(sk), tipc_net_id);

-       if (!rhashtable_remove_fast(&tn->sk_rht, &tsk->node, tsk_rht_params)) {
-               WARN_ON(refcount_read(&sk->sk_refcnt) == 1);
+       if (!rhashtable_remove_fast(&tn->sk_rht, &tsk->node, tsk_rht_params))
                __sock_put(sk);
-       }
 }

 static const struct rhashtable_params tsk_rht_params = {