[PATCH net-next v6 0/3] net: Avoid ehash lookup races

[PATCH net-next v6 0/3] net: Avoid ehash lookup races

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

After replacing R/W locks with RCU in commit 3ab5aee7fe84 ("net: Convert
TCP & DCCP hash tables to use RCU / hlist_nulls"), a race window emerged
during the switch from reqsk/sk to sk/tw.

Now that both timewait sock (tw) and full sock (sk) reside on the same
ehash chain, it is appropriate to introduce hlist_nulls replace
operations, to eliminate the race conditions caused by this window.

Before this series of patches, I previously sent another version of the
patch, attempting to avoid the issue using a lock mechanism. However, it
seems there are some problems with that approach now, so I've switched to
the "replace" method in the current patches to resolve the issue.
For details, refer to:
https://lore.kernel.org/netdev/20250903024406.2418362-1-xuanqiang.luo@linux.dev/

Before I encountered this type of issue recently, I found there had been
several historical discussions about it. Therefore, I'm adding this
background information for those interested to reference:
1. https://lore.kernel.org/lkml/20230118015941.1313-1-kerneljasonxing@gmail.com/
2. https://lore.kernel.org/netdev/20230606064306.9192-1-duanmuquan@baidu.com/

---

Changes:
  v6:
    * Patch 1
	* Send and CC to the RCU maintainers.
    * Patch 3
	* Remove the unused function inet_twsk_add_node_rcu() and variable
	  ehead fix build warnings.

  v5: https://lore.kernel.org/all/20250924015034.587056-1-xuanqiang.luo@linux.dev/
    * Patch 1
        * Rename __hlist_nulls_replace_rcu() to hlist_nulls_replace_rcu()
          and update the description of hlist_nulls_replace_init_rcu().
    * Patch 2
        * Remove __sk_nulls_replace_node_init_rcu() and inline it into
          sk_nulls_replace_node_init_rcu().
        * Use DEBUG_NET_WARN_ON_ONCE() instead of WARN_ON().
    * Patch 3
        * Move smp_wmb() after setting the refcount.

  v4: https://lore.kernel.org/all/20250920105945.538042-1-xuanqiang.luo@linux.dev/
    * Patch 1
        * Use WRITE_ONCE() for ->next in __hlist_nulls_replace_rcu(), and
          add why in the commit message.
        * Remove the node hash check in hlist_nulls_replace_init_rcu() to
          avoid redundancy. Also remove the return value, as it serves no
          purpose in this patch series.
    * Patch 3
        * Remove the check of hlist_nulls_replace_init_rcu() return value
          in inet_twsk_hashdance_schedule() as it is unnecessary.
          Thanks to Kuni for clarifying this.

  v3: https://lore.kernel.org/all/20250916103054.719584-1-xuanqiang.luo@linux.dev/
    * Add more background information on this type of issue to the letter
      cover.

  v2: https://lore.kernel.org/all/20250916064614.605075-1-xuanqiang.luo@linux.dev/
    * Patch 1
        * Use WRITE_ONCE() to initialize old->pprev.
    * Patch 2&3
        * Optimize sk hashed check. Thanks Kuni for pointing it out!

  v1: https://lore.kernel.org/all/20250915070308.111816-1-xuanqiang.luo@linux.dev/

Xuanqiang Luo (3):
  rculist: Add hlist_nulls_replace_rcu() and
    hlist_nulls_replace_init_rcu()
  inet: Avoid ehash lookup race in inet_ehash_insert()
  inet: Avoid ehash lookup race in inet_twsk_hashdance_schedule()

 include/linux/rculist_nulls.h | 52 +++++++++++++++++++++++++++++++++++
 include/net/sock.h            | 14 ++++++++++
 net/ipv4/inet_hashtables.c    |  4 ++-
 net/ipv4/inet_timewait_sock.c | 31 +++++++--------------
 4 files changed, 79 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH net-next v6 1/3] rculist: Add hlist_nulls_replace_rcu() and hlist_nulls_replace_init_rcu()

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Add two functions to atomically replace RCU-protected hlist_nulls entries.

Keep using WRITE_ONCE() to assign values to ->next and ->pprev, as
mentioned in the patch below:
commit efd04f8a8b45 ("rcu: Use WRITE_ONCE() for assignments to ->next for
rculist_nulls")
commit 860c8802ace1 ("rcu: Use WRITE_ONCE() for assignments to ->pprev for
hlist_nulls")

Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 include/linux/rculist_nulls.h | 52 +++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index 89186c499dd4..c3ba74b1890d 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -152,6 +152,58 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
 	n->next = (struct hlist_nulls_node *)NULLS_MARKER(NULL);
 }
 
+/**
+ * hlist_nulls_replace_rcu - replace an old entry by a new one
+ * @old: the element to be replaced
+ * @new: the new element to insert
+ *
+ * Description:
+ * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
+ * permitting racing traversals.
+ *
+ * The caller must take whatever precautions are necessary (such as holding
+ * appropriate locks) to avoid racing with another list-mutation primitive, such
+ * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
+ * list.  However, it is perfectly legal to run concurrently with the _rcu
+ * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
+ */
+static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
+					     struct hlist_nulls_node *new)
+{
+	struct hlist_nulls_node *next = old->next;
+
+	WRITE_ONCE(new->next, next);
+	WRITE_ONCE(new->pprev, old->pprev);
+	rcu_assign_pointer(*(struct hlist_nulls_node __rcu **)new->pprev, new);
+	if (!is_a_nulls(next))
+		WRITE_ONCE(new->next->pprev, &new->next);
+}
+
+/**
+ * hlist_nulls_replace_init_rcu - replace an old entry by a new one and
+ * initialize the old
+ * @old: the element to be replaced
+ * @new: the new element to insert
+ *
+ * Description:
+ * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
+ * permitting racing traversals, and reinitialize the old entry.
+ *
+ * Note: @old must be hashed.
+ *
+ * The caller must take whatever precautions are necessary (such as holding
+ * appropriate locks) to avoid racing with another list-mutation primitive, such
+ * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
+ * list. However, it is perfectly legal to run concurrently with the _rcu
+ * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
+ */
+static inline void hlist_nulls_replace_init_rcu(struct hlist_nulls_node *old,
+						struct hlist_nulls_node *new)
+{
+	hlist_nulls_replace_rcu(old, new);
+	WRITE_ONCE(old->pprev, NULL);
+}
+
 /**
  * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
  * @tpos:	the type * to use as a loop cursor.
-- 
2.25.1

[PATCH net-next v6 2/3] inet: Avoid ehash lookup race in inet_ehash_insert()

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.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")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 include/net/sock.h         | 14 ++++++++++++++
 net/ipv4/inet_hashtables.c |  4 +++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 0fd465935334..5d67f5cbae52 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -854,6 +854,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);
+		__sock_put(old);
+		return true;
+	}
+
+	return false;
+}
+
 static inline void __sk_add_node(struct sock *sk, struct hlist_head *list)
 {
 	hlist_add_head(&sk->sk_node, list);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ef4ccfd46ff6..83c9ec625419 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -685,7 +685,8 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
 	spin_lock(lock);
 	if (osk) {
 		WARN_ON_ONCE(sk->sk_hash != osk->sk_hash);
-		ret = sk_nulls_del_node_init_rcu(osk);
+		ret = sk_nulls_replace_node_init_rcu(osk, sk);
+		goto unlock;
 	} else if (found_dup_sk) {
 		*found_dup_sk = inet_ehash_lookup_by_sk(sk, list);
 		if (*found_dup_sk)
@@ -695,6 +696,7 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
 	if (ret)
 		__sk_nulls_add_node_rcu(sk, list);
 
+unlock:
 	spin_unlock(lock);
 
 	return ret;
-- 
2.25.1

[PATCH net-next v6 3/3] inet: Avoid ehash lookup race in inet_twsk_hashdance_schedule()

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Since ehash lookups are lockless, if another CPU is converting sk to tw
concurrently, fetching the newly inserted tw with tw->tw_refcnt == 0 cause
lookup failure.

The call trace map is drawn as follows:
   CPU 0                                CPU 1
   -----                                -----
				     inet_twsk_hashdance_schedule()
				     spin_lock()
				     inet_twsk_add_node_rcu(tw, ...)
__inet_lookup_established()
(find tw, failure due to tw_refcnt = 0)
				     __sk_nulls_del_node_init_rcu(sk)
				     refcount_set(&tw->tw_refcnt, 3)
				     spin_unlock()

By replacing sk with tw atomically via hlist_nulls_replace_init_rcu() after
setting tw_refcnt, we ensure that tw is either fully initialized or not
visible to other CPUs, eliminating the race.

It's worth noting that we held lock_sock() before the replacement, so
there's no need to check if sk is hashed. Thanks to Kuniyuki Iwashima!

Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 net/ipv4/inet_timewait_sock.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index 5b5426b8ee92..89dc0a5d7248 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -87,12 +87,6 @@ void inet_twsk_put(struct inet_timewait_sock *tw)
 }
 EXPORT_SYMBOL_GPL(inet_twsk_put);
 
-static void inet_twsk_add_node_rcu(struct inet_timewait_sock *tw,
-				   struct hlist_nulls_head *list)
-{
-	hlist_nulls_add_head_rcu(&tw->tw_node, list);
-}
-
 static void inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo)
 {
 	__inet_twsk_schedule(tw, timeo, false);
@@ -112,11 +106,10 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
 {
 	const struct inet_sock *inet = inet_sk(sk);
 	const struct inet_connection_sock *icsk = inet_csk(sk);
-	struct inet_ehash_bucket *ehead = inet_ehash_bucket(hashinfo, sk->sk_hash);
 	spinlock_t *lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
 	struct inet_bind_hashbucket *bhead, *bhead2;
 
-	/* Step 1: Put TW into bind hash. Original socket stays there too.
+	/* Put TW into bind hash. Original socket stays there too.
 	   Note, that any socket with inet->num != 0 MUST be bound in
 	   binding cache, even if it is closed.
 	 */
@@ -140,19 +133,6 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
 
 	spin_lock(lock);
 
-	/* Step 2: Hash TW into tcp ehash chain */
-	inet_twsk_add_node_rcu(tw, &ehead->chain);
-
-	/* Step 3: Remove SK from hash chain */
-	if (__sk_nulls_del_node_init_rcu(sk))
-		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
-
-
-	/* Ensure above writes are committed into memory before updating the
-	 * refcount.
-	 * Provides ordering vs later refcount_inc().
-	 */
-	smp_wmb();
 	/* tw_refcnt is set to 3 because we have :
 	 * - one reference for bhash chain.
 	 * - one reference for ehash chain.
@@ -162,6 +142,15 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
 	 */
 	refcount_set(&tw->tw_refcnt, 3);
 
+	/* Ensure tw_refcnt has been set before tw is published.
+	 * smp_wmb() provides the necessary memory barrier to enforce this
+	 * ordering.
+	 */
+	smp_wmb();
+
+	hlist_nulls_replace_init_rcu(&sk->sk_nulls_node, &tw->tw_node);
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+
 	inet_twsk_schedule(tw, timeo);
 
 	spin_unlock(lock);
-- 
2.25.1

Re: [PATCH net-next v6 1/3] rculist: Add hlist_nulls_replace_rcu() and hlist_nulls_replace_init_rcu()

[Kuniyuki Iwashima]

On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>
> Add two functions to atomically replace RCU-protected hlist_nulls entries.
>
> Keep using WRITE_ONCE() to assign values to ->next and ->pprev, as
> mentioned in the patch below:
> commit efd04f8a8b45 ("rcu: Use WRITE_ONCE() for assignments to ->next for
> rculist_nulls")
> commit 860c8802ace1 ("rcu: Use WRITE_ONCE() for assignments to ->pprev for
> hlist_nulls")
>
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
>  include/linux/rculist_nulls.h | 52 +++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index 89186c499dd4..c3ba74b1890d 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -152,6 +152,58 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
>         n->next = (struct hlist_nulls_node *)NULLS_MARKER(NULL);
>  }
>
> +/**
> + * hlist_nulls_replace_rcu - replace an old entry by a new one
> + * @old: the element to be replaced
> + * @new: the new element to insert
> + *
> + * Description:
> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
> + * permitting racing traversals.
> + *
> + * The caller must take whatever precautions are necessary (such as holding
> + * appropriate locks) to avoid racing with another list-mutation primitive, such
> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
> + * list.  However, it is perfectly legal to run concurrently with the _rcu
> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
> + */
> +static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
> +                                            struct hlist_nulls_node *new)

nit: checkpatch complains here..
https://patchwork.kernel.org/project/netdevbpf/patch/20250925021628.886203-2-xuanqiang.luo@linux.dev/

CHECK: Alignment should match open parenthesis
#46: FILE: include/linux/rculist_nulls.h:171:
+static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
+     struct hlist_nulls_node *new)


> +{
> +       struct hlist_nulls_node *next = old->next;
> +
> +       WRITE_ONCE(new->next, next);
> +       WRITE_ONCE(new->pprev, old->pprev);
> +       rcu_assign_pointer(*(struct hlist_nulls_node __rcu **)new->pprev, new);

nit: define hlist_nulls_prev_rcu() like hlist_nulls_next_rcu().


> +       if (!is_a_nulls(next))
> +               WRITE_ONCE(new->next->pprev, &new->next);

nit: s/new->next->pprev/next->pprev/

> +}
> +
> +/**
> + * hlist_nulls_replace_init_rcu - replace an old entry by a new one and
> + * initialize the old
> + * @old: the element to be replaced
> + * @new: the new element to insert
> + *
> + * Description:
> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
> + * permitting racing traversals, and reinitialize the old entry.
> + *
> + * Note: @old must be hashed.
> + *
> + * The caller must take whatever precautions are necessary (such as holding
> + * appropriate locks) to avoid racing with another list-mutation primitive, such
> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
> + * list. However, it is perfectly legal to run concurrently with the _rcu
> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
> + */
> +static inline void hlist_nulls_replace_init_rcu(struct hlist_nulls_node *old,
> +                                               struct hlist_nulls_node *new)
> +{
> +       hlist_nulls_replace_rcu(old, new);
> +       WRITE_ONCE(old->pprev, NULL);
> +}
> +
>  /**
>   * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
>   * @tpos:      the type * to use as a loop cursor.
> --
> 2.25.1
>

Re: [PATCH net-next v6 2/3] inet: Avoid ehash lookup race in inet_ehash_insert()

[Kuniyuki Iwashima]

On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@kylinos.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")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
>  include/net/sock.h         | 14 ++++++++++++++
>  net/ipv4/inet_hashtables.c |  4 +++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 0fd465935334..5d67f5cbae52 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -854,6 +854,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);
> +               __sock_put(old);
> +               return true;
> +       }
> +
> +       return false;
> +}
> +
>  static inline void __sk_add_node(struct sock *sk, struct hlist_head *list)
>  {
>         hlist_add_head(&sk->sk_node, list);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index ef4ccfd46ff6..83c9ec625419 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -685,7 +685,8 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
>         spin_lock(lock);
>         if (osk) {
>                 WARN_ON_ONCE(sk->sk_hash != osk->sk_hash);
> -               ret = sk_nulls_del_node_init_rcu(osk);
> +               ret = sk_nulls_replace_node_init_rcu(osk, sk);
> +               goto unlock;
>         } else if (found_dup_sk) {

As you need another iteration, remove else here since
we bail out above with goto:

    goto unlock;
}

if (found_dup_sk) {

Other than that

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thanks!


>                 *found_dup_sk = inet_ehash_lookup_by_sk(sk, list);
>                 if (*found_dup_sk)
> @@ -695,6 +696,7 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
>         if (ret)
>                 __sk_nulls_add_node_rcu(sk, list);
>
> +unlock:
>         spin_unlock(lock);
>
>         return ret;
> --
> 2.25.1
>

Re: [PATCH net-next v6 3/3] inet: Avoid ehash lookup race in inet_twsk_hashdance_schedule()

[Kuniyuki Iwashima]

On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>
> Since ehash lookups are lockless, if another CPU is converting sk to tw
> concurrently, fetching the newly inserted tw with tw->tw_refcnt == 0 cause
> lookup failure.
>
> The call trace map is drawn as follows:
>    CPU 0                                CPU 1
>    -----                                -----
>                                      inet_twsk_hashdance_schedule()
>                                      spin_lock()
>                                      inet_twsk_add_node_rcu(tw, ...)
> __inet_lookup_established()
> (find tw, failure due to tw_refcnt = 0)
>                                      __sk_nulls_del_node_init_rcu(sk)
>                                      refcount_set(&tw->tw_refcnt, 3)
>                                      spin_unlock()
>
> By replacing sk with tw atomically via hlist_nulls_replace_init_rcu() after
> setting tw_refcnt, we ensure that tw is either fully initialized or not
> visible to other CPUs, eliminating the race.
>
> It's worth noting that we held lock_sock() before the replacement, so
> there's no need to check if sk is hashed. Thanks to Kuniyuki Iwashima!
>
> Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
>  net/ipv4/inet_timewait_sock.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index 5b5426b8ee92..89dc0a5d7248 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -87,12 +87,6 @@ void inet_twsk_put(struct inet_timewait_sock *tw)
>  }
>  EXPORT_SYMBOL_GPL(inet_twsk_put);
>
> -static void inet_twsk_add_node_rcu(struct inet_timewait_sock *tw,
> -                                  struct hlist_nulls_head *list)
> -{
> -       hlist_nulls_add_head_rcu(&tw->tw_node, list);
> -}
> -
>  static void inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo)
>  {
>         __inet_twsk_schedule(tw, timeo, false);
> @@ -112,11 +106,10 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>  {
>         const struct inet_sock *inet = inet_sk(sk);
>         const struct inet_connection_sock *icsk = inet_csk(sk);
> -       struct inet_ehash_bucket *ehead = inet_ehash_bucket(hashinfo, sk->sk_hash);
>         spinlock_t *lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
>         struct inet_bind_hashbucket *bhead, *bhead2;
>
> -       /* Step 1: Put TW into bind hash. Original socket stays there too.
> +       /* Put TW into bind hash. Original socket stays there too.
>            Note, that any socket with inet->num != 0 MUST be bound in
>            binding cache, even if it is closed.

While at it, could you update the comment style at these 2 line above too ?

/* Put ..
 * Note, ...
 * binding ...
 */

Otherwise looks good, thanks.

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>




>          */
> @@ -140,19 +133,6 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>
>         spin_lock(lock);
>
> -       /* Step 2: Hash TW into tcp ehash chain */
> -       inet_twsk_add_node_rcu(tw, &ehead->chain);
> -
> -       /* Step 3: Remove SK from hash chain */
> -       if (__sk_nulls_del_node_init_rcu(sk))
> -               sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> -
> -
> -       /* Ensure above writes are committed into memory before updating the
> -        * refcount.
> -        * Provides ordering vs later refcount_inc().
> -        */
> -       smp_wmb();
>         /* tw_refcnt is set to 3 because we have :
>          * - one reference for bhash chain.
>          * - one reference for ehash chain.
> @@ -162,6 +142,15 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>          */
>         refcount_set(&tw->tw_refcnt, 3);
>
> +       /* Ensure tw_refcnt has been set before tw is published.
> +        * smp_wmb() provides the necessary memory barrier to enforce this
> +        * ordering.
> +        */
> +       smp_wmb();
> +
> +       hlist_nulls_replace_init_rcu(&sk->sk_nulls_node, &tw->tw_node);
> +       sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> +
>         inet_twsk_schedule(tw, timeo);
>
>         spin_unlock(lock);
> --
> 2.25.1
>

Re: [PATCH net-next v6 1/3] rculist: Add hlist_nulls_replace_rcu() and hlist_nulls_replace_init_rcu()

[luoxuanqiang]

在 2025/9/26 01:56, Kuniyuki Iwashima 写道:
> On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> Add two functions to atomically replace RCU-protected hlist_nulls entries.
>>
>> Keep using WRITE_ONCE() to assign values to ->next and ->pprev, as
>> mentioned in the patch below:
>> commit efd04f8a8b45 ("rcu: Use WRITE_ONCE() for assignments to ->next for
>> rculist_nulls")
>> commit 860c8802ace1 ("rcu: Use WRITE_ONCE() for assignments to ->pprev for
>> hlist_nulls")
>>
>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>> ---
>>   include/linux/rculist_nulls.h | 52 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
>> index 89186c499dd4..c3ba74b1890d 100644
>> --- a/include/linux/rculist_nulls.h
>> +++ b/include/linux/rculist_nulls.h
>> @@ -152,6 +152,58 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
>>          n->next = (struct hlist_nulls_node *)NULLS_MARKER(NULL);
>>   }
>>
>> +/**
>> + * hlist_nulls_replace_rcu - replace an old entry by a new one
>> + * @old: the element to be replaced
>> + * @new: the new element to insert
>> + *
>> + * Description:
>> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
>> + * permitting racing traversals.
>> + *
>> + * The caller must take whatever precautions are necessary (such as holding
>> + * appropriate locks) to avoid racing with another list-mutation primitive, such
>> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
>> + * list.  However, it is perfectly legal to run concurrently with the _rcu
>> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
>> + */
>> +static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
>> +                                            struct hlist_nulls_node *new)
> nit: checkpatch complains here..
> https://patchwork.kernel.org/project/netdevbpf/patch/20250925021628.886203-2-xuanqiang.luo@linux.dev/
>
> CHECK: Alignment should match open parenthesis
> #46: FILE: include/linux/rculist_nulls.h:171:
> +static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
> +     struct hlist_nulls_node *new)

Thanks for pointing it out, I'll fix it in the next version.

Appreciate it!

>
>> +{
>> +       struct hlist_nulls_node *next = old->next;
>> +
>> +       WRITE_ONCE(new->next, next);
>> +       WRITE_ONCE(new->pprev, old->pprev);
>> +       rcu_assign_pointer(*(struct hlist_nulls_node __rcu **)new->pprev, new);
> nit: define hlist_nulls_prev_rcu() like hlist_nulls_next_rcu().

I'm wondering if defining a macro called hlist_nulls_prev_rcu() might
be controversial, since it should actually be getting the prev->next
rather than the prev itself.

Would it be more appropriate to rename it to hlist_nulls_prev_next_rcu()
instead?

Like this:

diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index c3ba74b1890d..9399ec9dc82d 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -52,6 +52,14 @@ static inline void hlist_nulls_del_init_rcu(struct hlist_nulls_node *n)
  #define hlist_nulls_next_rcu(node) \
         (*((struct hlist_nulls_node __rcu __force **)&(node)->next))
+/**
+ * hlist_nulls_prev_next_rcu - returns the next pointer of the previous
+ * element.
+ * @node: element of the list.
+ */
+#define hlist_nulls_prev_next_rcu(node) \
+       (*((struct hlist_nulls_node __rcu __force **)(node)->pprev))
+
  /**
   * hlist_nulls_del_rcu - deletes entry from hash list without re-initialization
   * @n: the element to delete from the hash list.

However, I noticed that in the definition of hlist_pprev_rcu(), it directly
uses pprev:

#define hlist_pprev_rcu(node)    (*((struct hlist_node __rcu **)((node)->pprev)))

This part seems a bit confusing to me. It's possible I'm missing some
historical context here. If so, please feel free to point it out.

Thanks!

>
>> +       if (!is_a_nulls(next))
>> +               WRITE_ONCE(new->next->pprev, &new->next);
> nit: s/new->next->pprev/next->pprev/

I'll update it in the next version.

Thank you!

>> +}
>> +
>> +/**
>> + * hlist_nulls_replace_init_rcu - replace an old entry by a new one and
>> + * initialize the old
>> + * @old: the element to be replaced
>> + * @new: the new element to insert
>> + *
>> + * Description:
>> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
>> + * permitting racing traversals, and reinitialize the old entry.
>> + *
>> + * Note: @old must be hashed.
>> + *
>> + * The caller must take whatever precautions are necessary (such as holding
>> + * appropriate locks) to avoid racing with another list-mutation primitive, such
>> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
>> + * list. However, it is perfectly legal to run concurrently with the _rcu
>> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
>> + */
>> +static inline void hlist_nulls_replace_init_rcu(struct hlist_nulls_node *old,
>> +                                               struct hlist_nulls_node *new)
>> +{
>> +       hlist_nulls_replace_rcu(old, new);
>> +       WRITE_ONCE(old->pprev, NULL);
>> +}
>> +
>>   /**
>>    * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
>>    * @tpos:      the type * to use as a loop cursor.
>> --
>> 2.25.1
>>

Re: [PATCH net-next v6 2/3] inet: Avoid ehash lookup race in inet_ehash_insert()

[luoxuanqiang]

在 2025/9/26 02:00, Kuniyuki Iwashima 写道:
> On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.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")
>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>> ---
>>   include/net/sock.h         | 14 ++++++++++++++
>>   net/ipv4/inet_hashtables.c |  4 +++-
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 0fd465935334..5d67f5cbae52 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -854,6 +854,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);
>> +               __sock_put(old);
>> +               return true;
>> +       }
>> +
>> +       return false;
>> +}
>> +
>>   static inline void __sk_add_node(struct sock *sk, struct hlist_head *list)
>>   {
>>          hlist_add_head(&sk->sk_node, list);
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index ef4ccfd46ff6..83c9ec625419 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -685,7 +685,8 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
>>          spin_lock(lock);
>>          if (osk) {
>>                  WARN_ON_ONCE(sk->sk_hash != osk->sk_hash);
>> -               ret = sk_nulls_del_node_init_rcu(osk);
>> +               ret = sk_nulls_replace_node_init_rcu(osk, sk);
>> +               goto unlock;
>>          } else if (found_dup_sk) {
> As you need another iteration, remove else here since
> we bail out above with goto:
>
>      goto unlock;
> }
>
> if (found_dup_sk) {
>
> Other than that
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
>
> Thanks!
>
Thank you for the review!

I'll fix it in the next version.

>>                  *found_dup_sk = inet_ehash_lookup_by_sk(sk, list);
>>                  if (*found_dup_sk)
>> @@ -695,6 +696,7 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
>>          if (ret)
>>                  __sk_nulls_add_node_rcu(sk, list);
>>
>> +unlock:
>>          spin_unlock(lock);
>>
>>          return ret;
>> --
>> 2.25.1
>>

Re: [PATCH net-next v6 3/3] inet: Avoid ehash lookup race in inet_twsk_hashdance_schedule()

[luoxuanqiang]

在 2025/9/26 02:22, Kuniyuki Iwashima 写道:
> On Wed, Sep 24, 2025 at 7:18 PM <xuanqiang.luo@linux.dev> wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> Since ehash lookups are lockless, if another CPU is converting sk to tw
>> concurrently, fetching the newly inserted tw with tw->tw_refcnt == 0 cause
>> lookup failure.
>>
>> The call trace map is drawn as follows:
>>     CPU 0                                CPU 1
>>     -----                                -----
>>                                       inet_twsk_hashdance_schedule()
>>                                       spin_lock()
>>                                       inet_twsk_add_node_rcu(tw, ...)
>> __inet_lookup_established()
>> (find tw, failure due to tw_refcnt = 0)
>>                                       __sk_nulls_del_node_init_rcu(sk)
>>                                       refcount_set(&tw->tw_refcnt, 3)
>>                                       spin_unlock()
>>
>> By replacing sk with tw atomically via hlist_nulls_replace_init_rcu() after
>> setting tw_refcnt, we ensure that tw is either fully initialized or not
>> visible to other CPUs, eliminating the race.
>>
>> It's worth noting that we held lock_sock() before the replacement, so
>> there's no need to check if sk is hashed. Thanks to Kuniyuki Iwashima!
>>
>> Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>> ---
>>   net/ipv4/inet_timewait_sock.c | 31 ++++++++++---------------------
>>   1 file changed, 10 insertions(+), 21 deletions(-)
>>
>> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
>> index 5b5426b8ee92..89dc0a5d7248 100644
>> --- a/net/ipv4/inet_timewait_sock.c
>> +++ b/net/ipv4/inet_timewait_sock.c
>> @@ -87,12 +87,6 @@ void inet_twsk_put(struct inet_timewait_sock *tw)
>>   }
>>   EXPORT_SYMBOL_GPL(inet_twsk_put);
>>
>> -static void inet_twsk_add_node_rcu(struct inet_timewait_sock *tw,
>> -                                  struct hlist_nulls_head *list)
>> -{
>> -       hlist_nulls_add_head_rcu(&tw->tw_node, list);
>> -}
>> -
>>   static void inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo)
>>   {
>>          __inet_twsk_schedule(tw, timeo, false);
>> @@ -112,11 +106,10 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>>   {
>>          const struct inet_sock *inet = inet_sk(sk);
>>          const struct inet_connection_sock *icsk = inet_csk(sk);
>> -       struct inet_ehash_bucket *ehead = inet_ehash_bucket(hashinfo, sk->sk_hash);
>>          spinlock_t *lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
>>          struct inet_bind_hashbucket *bhead, *bhead2;
>>
>> -       /* Step 1: Put TW into bind hash. Original socket stays there too.
>> +       /* Put TW into bind hash. Original socket stays there too.
>>             Note, that any socket with inet->num != 0 MUST be bound in
>>             binding cache, even if it is closed.
> While at it, could you update the comment style at these 2 line above too ?
>
> /* Put ..
>   * Note, ...
>   * binding ...
>   */
>
> Otherwise looks good, thanks.
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
>
I'll fix the comment style in the next version.

Thanks for the review!

>
>
>>           */
>> @@ -140,19 +133,6 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>>
>>          spin_lock(lock);
>>
>> -       /* Step 2: Hash TW into tcp ehash chain */
>> -       inet_twsk_add_node_rcu(tw, &ehead->chain);
>> -
>> -       /* Step 3: Remove SK from hash chain */
>> -       if (__sk_nulls_del_node_init_rcu(sk))
>> -               sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>> -
>> -
>> -       /* Ensure above writes are committed into memory before updating the
>> -        * refcount.
>> -        * Provides ordering vs later refcount_inc().
>> -        */
>> -       smp_wmb();
>>          /* tw_refcnt is set to 3 because we have :
>>           * - one reference for bhash chain.
>>           * - one reference for ehash chain.
>> @@ -162,6 +142,15 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>>           */
>>          refcount_set(&tw->tw_refcnt, 3);
>>
>> +       /* Ensure tw_refcnt has been set before tw is published.
>> +        * smp_wmb() provides the necessary memory barrier to enforce this
>> +        * ordering.
>> +        */
>> +       smp_wmb();
>> +
>> +       hlist_nulls_replace_init_rcu(&sk->sk_nulls_node, &tw->tw_node);
>> +       sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>> +
>>          inet_twsk_schedule(tw, timeo);
>>
>>          spin_unlock(lock);
>> --
>> 2.25.1
>>

Re: [PATCH net-next v6 1/3] rculist: Add hlist_nulls_replace_rcu() and hlist_nulls_replace_init_rcu()

[Kuniyuki Iwashima]

On Thu, Sep 25, 2025 at 8:23 PM luoxuanqiang <xuanqiang.luo@linux.dev> wrote:
[...]
> >> +{
> >> +       struct hlist_nulls_node *next = old->next;
> >> +
> >> +       WRITE_ONCE(new->next, next);
> >> +       WRITE_ONCE(new->pprev, old->pprev);
> >> +       rcu_assign_pointer(*(struct hlist_nulls_node __rcu **)new->pprev, new);
> > nit: define hlist_nulls_prev_rcu() like hlist_nulls_next_rcu().
>
> I'm wondering if defining a macro called hlist_nulls_prev_rcu() might
> be controversial, since it should actually be getting the prev->next
> rather than the prev itself.

See hlist_add_before_rcu() for an example:

    rcu_assign_pointer(hlist_pprev_rcu(n), n);

You can define hlist_nulls_pprev_rcu() and use it alike.

    rcu_assign_pointer(hlist_nulls_pprev_rcu(new), new);


[...]
> However, I noticed that in the definition of hlist_pprev_rcu(), it directly
> uses pprev:
>
> #define hlist_pprev_rcu(node)    (*((struct hlist_node __rcu **)((node)->pprev)))

Note it dereferences *((node)->pprev).  The macro is not to iterate the
lengthy cast.

Re: [PATCH net-next v6 1/3] rculist: Add hlist_nulls_replace_rcu() and hlist_nulls_replace_init_rcu()

[luoxuanqiang]

在 2025/9/26 12:02, Kuniyuki Iwashima 写道:
> On Thu, Sep 25, 2025 at 8:23 PM luoxuanqiang <xuanqiang.luo@linux.dev> wrote:
> [...]
>>>> +{
>>>> +       struct hlist_nulls_node *next = old->next;
>>>> +
>>>> +       WRITE_ONCE(new->next, next);
>>>> +       WRITE_ONCE(new->pprev, old->pprev);
>>>> +       rcu_assign_pointer(*(struct hlist_nulls_node __rcu **)new->pprev, new);
>>> nit: define hlist_nulls_prev_rcu() like hlist_nulls_next_rcu().
>> I'm wondering if defining a macro called hlist_nulls_prev_rcu() might
>> be controversial, since it should actually be getting the prev->next
>> rather than the prev itself.
> See hlist_add_before_rcu() for an example:
>
>      rcu_assign_pointer(hlist_pprev_rcu(n), n);
>
> You can define hlist_nulls_pprev_rcu() and use it alike.
>
>      rcu_assign_pointer(hlist_nulls_pprev_rcu(new), new);
>
>
> [...]
>> However, I noticed that in the definition of hlist_pprev_rcu(), it directly
>> uses pprev:
>>
>> #define hlist_pprev_rcu(node)    (*((struct hlist_node __rcu **)((node)->pprev)))
> Note it dereferences *((node)->pprev).  The macro is not to iterate the
> lengthy cast.

Thank you for the quick and clear explanation!

I'll apply these changes in the next version.

Appreciate it!