[PATCH net] inet: Avoid established lookup missing active sk

[PATCH net] inet: Avoid established lookup missing active sk

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Since the lookup of sk in ehash is lockless, when one CPU is performing a
lookup while another CPU is executing delete and insert operations
(deleting reqsk and inserting sk), the lookup CPU may miss either of
them, if sk cannot be found, an RST may be sent.

The call trace map is drawn as follows:
   CPU 0                           CPU 1
   -----                           -----
                                spin_lock()
                                sk_nulls_del_node_init_rcu(osk)
__inet_lookup_established()
                                __sk_nulls_add_node_rcu(sk, list)
                                spin_unlock()

We can try using spin_lock()/spin_unlock() to wait for ehash updates
(ensuring all deletions and insertions are completed) after a failed
lookup in ehash, then lookup sk again after the update. Since the sk
expected to be found is unlikely to encounter the aforementioned scenario
multiple times consecutively, we only need one update.

Similarly, an issue occurs in tw hashdance. Try adjusting the order in
which it operates on ehash: remove sk first, then add tw. If sk is missed
during lookup, it will likewise wait for the update to find tw, without
worrying about the skc_refcnt issue that would arise if tw were found
first.

Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 net/ipv4/inet_hashtables.c    | 12 ++++++++++++
 net/ipv4/inet_timewait_sock.c |  9 ++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ceeeec9b7290..4eb3a55b855b 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -505,6 +505,7 @@ struct sock *__inet_lookup_established(const struct net *net,
 	unsigned int hash = inet_ehashfn(net, daddr, hnum, saddr, sport);
 	unsigned int slot = hash & hashinfo->ehash_mask;
 	struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
+	bool try_lock = true;
 
 begin:
 	sk_nulls_for_each_rcu(sk, node, &head->chain) {
@@ -528,6 +529,17 @@ struct sock *__inet_lookup_established(const struct net *net,
 	 */
 	if (get_nulls_value(node) != slot)
 		goto begin;
+
+	if (try_lock) {
+		spinlock_t *lock = inet_ehash_lockp(hashinfo, hash);
+
+		try_lock = false;
+		spin_lock(lock);
+		/* Ensure ehash ops under spinlock complete. */
+		spin_unlock(lock);
+		goto begin;
+	}
+
 out:
 	sk = NULL;
 found:
diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index 875ff923a8ed..a91e02e19c53 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -139,14 +139,10 @@ 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 */
+	/* Step 2: 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().
@@ -161,6 +157,9 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
 	 */
 	refcount_set(&tw->tw_refcnt, 3);
 
+	/* Step 3: Hash TW into tcp ehash chain */
+	inet_twsk_add_node_rcu(tw, &ehead->chain);
+
 	inet_twsk_schedule(tw, timeo);
 
 	spin_unlock(lock);
-- 
2.25.1

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Kuniyuki Iwashima]

On Tue, Sep 2, 2025 at 7:45 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>
> Since the lookup of sk in ehash is lockless, when one CPU is performing a
> lookup while another CPU is executing delete and insert operations
> (deleting reqsk and inserting sk), the lookup CPU may miss either of
> them, if sk cannot be found, an RST may be sent.
>
> The call trace map is drawn as follows:
>    CPU 0                           CPU 1
>    -----                           -----
>                                 spin_lock()
>                                 sk_nulls_del_node_init_rcu(osk)
> __inet_lookup_established()
>                                 __sk_nulls_add_node_rcu(sk, list)
>                                 spin_unlock()

This usually does not happen except for local communication, and
retrying on the client side is much better than penalising all lookups
for SYN.

>
> We can try using spin_lock()/spin_unlock() to wait for ehash updates
> (ensuring all deletions and insertions are completed) after a failed
> lookup in ehash, then lookup sk again after the update. Since the sk
> expected to be found is unlikely to encounter the aforementioned scenario
> multiple times consecutively, we only need one update.
>
> Similarly, an issue occurs in tw hashdance. Try adjusting the order in
> which it operates on ehash: remove sk first, then add tw. If sk is missed
> during lookup, it will likewise wait for the update to find tw, without
> worrying about the skc_refcnt issue that would arise if tw were found
> first.
>
> Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
>  net/ipv4/inet_hashtables.c    | 12 ++++++++++++
>  net/ipv4/inet_timewait_sock.c |  9 ++++-----
>  2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index ceeeec9b7290..4eb3a55b855b 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -505,6 +505,7 @@ struct sock *__inet_lookup_established(const struct net *net,
>         unsigned int hash = inet_ehashfn(net, daddr, hnum, saddr, sport);
>         unsigned int slot = hash & hashinfo->ehash_mask;
>         struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
> +       bool try_lock = true;
>
>  begin:
>         sk_nulls_for_each_rcu(sk, node, &head->chain) {
> @@ -528,6 +529,17 @@ struct sock *__inet_lookup_established(const struct net *net,
>          */
>         if (get_nulls_value(node) != slot)
>                 goto begin;
> +
> +       if (try_lock) {
> +               spinlock_t *lock = inet_ehash_lockp(hashinfo, hash);
> +
> +               try_lock = false;
> +               spin_lock(lock);
> +               /* Ensure ehash ops under spinlock complete. */
> +               spin_unlock(lock);
> +               goto begin;
> +       }
> +
>  out:
>         sk = NULL;
>  found:
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index 875ff923a8ed..a91e02e19c53 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -139,14 +139,10 @@ 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);

You are adding a new RST scenario where the corresponding
socket is not found and a listener or no socket is found.

The try_lock part is not guaranteed to happen after twsk
insertion below.


> -
> -       /* Step 3: Remove SK from hash chain */
> +       /* Step 2: 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().
> @@ -161,6 +157,9 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>          */
>         refcount_set(&tw->tw_refcnt, 3);
>
> +       /* Step 3: Hash TW into tcp ehash chain */
> +       inet_twsk_add_node_rcu(tw, &ehead->chain);
> +
>         inet_twsk_schedule(tw, timeo);
>
>         spin_unlock(lock);
> --
> 2.25.1
>

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Kuniyuki Iwashima]

On Tue, Sep 2, 2025 at 10:16 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> On Tue, Sep 2, 2025 at 7:45 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
> >
> > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> >
> > Since the lookup of sk in ehash is lockless, when one CPU is performing a
> > lookup while another CPU is executing delete and insert operations
> > (deleting reqsk and inserting sk), the lookup CPU may miss either of
> > them, if sk cannot be found, an RST may be sent.
> >
> > The call trace map is drawn as follows:
> >    CPU 0                           CPU 1
> >    -----                           -----
> >                                 spin_lock()
> >                                 sk_nulls_del_node_init_rcu(osk)
> > __inet_lookup_established()
> >                                 __sk_nulls_add_node_rcu(sk, list)
> >                                 spin_unlock()
>
> This usually does not happen except for local communication, and
> retrying on the client side is much better than penalising all lookups
> for SYN.
>
> >
> > We can try using spin_lock()/spin_unlock() to wait for ehash updates
> > (ensuring all deletions and insertions are completed) after a failed
> > lookup in ehash, then lookup sk again after the update. Since the sk
> > expected to be found is unlikely to encounter the aforementioned scenario
> > multiple times consecutively, we only need one update.
> >
> > Similarly, an issue occurs in tw hashdance. Try adjusting the order in
> > which it operates on ehash: remove sk first, then add tw. If sk is missed
> > during lookup, it will likewise wait for the update to find tw, without
> > worrying about the skc_refcnt issue that would arise if tw were found
> > first.
> >
> > Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
> > Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> > ---
> >  net/ipv4/inet_hashtables.c    | 12 ++++++++++++
> >  net/ipv4/inet_timewait_sock.c |  9 ++++-----
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > index ceeeec9b7290..4eb3a55b855b 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -505,6 +505,7 @@ struct sock *__inet_lookup_established(const struct net *net,
> >         unsigned int hash = inet_ehashfn(net, daddr, hnum, saddr, sport);
> >         unsigned int slot = hash & hashinfo->ehash_mask;
> >         struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
> > +       bool try_lock = true;
> >
> >  begin:
> >         sk_nulls_for_each_rcu(sk, node, &head->chain) {
> > @@ -528,6 +529,17 @@ struct sock *__inet_lookup_established(const struct net *net,
> >          */
> >         if (get_nulls_value(node) != slot)
> >                 goto begin;
> > +
> > +       if (try_lock) {
> > +               spinlock_t *lock = inet_ehash_lockp(hashinfo, hash);
> > +
> > +               try_lock = false;
> > +               spin_lock(lock);
> > +               /* Ensure ehash ops under spinlock complete. */
> > +               spin_unlock(lock);
> > +               goto begin;
> > +       }
> > +
> >  out:
> >         sk = NULL;
> >  found:
> > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> > index 875ff923a8ed..a91e02e19c53 100644
> > --- a/net/ipv4/inet_timewait_sock.c
> > +++ b/net/ipv4/inet_timewait_sock.c
> > @@ -139,14 +139,10 @@ 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);
>
> You are adding a new RST scenario where the corresponding
> socket is not found and a listener or no socket is found.
>
> The try_lock part is not guaranteed to happen after twsk
> insertion below.

Oh no, spin_lock() dance sychronises the threads but I still
think this is rather harmful for normal cases; now sending
an unmatched packet can trigger lock dance, which is easily
abused for DDoS.


>
>
> > -
> > -       /* Step 3: Remove SK from hash chain */
> > +       /* Step 2: 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().
> > @@ -161,6 +157,9 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
> >          */
> >         refcount_set(&tw->tw_refcnt, 3);
> >
> > +       /* Step 3: Hash TW into tcp ehash chain */
> > +       inet_twsk_add_node_rcu(tw, &ehead->chain);
> > +
> >         inet_twsk_schedule(tw, timeo);
> >
> >         spin_unlock(lock);
> > --
> > 2.25.1
> >

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Eric Dumazet]

On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>
> Since the lookup of sk in ehash is lockless, when one CPU is performing a
> lookup while another CPU is executing delete and insert operations
> (deleting reqsk and inserting sk), the lookup CPU may miss either of
> them, if sk cannot be found, an RST may be sent.
>
> The call trace map is drawn as follows:
>    CPU 0                           CPU 1
>    -----                           -----
>                                 spin_lock()
>                                 sk_nulls_del_node_init_rcu(osk)
> __inet_lookup_established()
>                                 __sk_nulls_add_node_rcu(sk, list)
>                                 spin_unlock()
>
> We can try using spin_lock()/spin_unlock() to wait for ehash updates
> (ensuring all deletions and insertions are completed) after a failed
> lookup in ehash, then lookup sk again after the update. Since the sk
> expected to be found is unlikely to encounter the aforementioned scenario
> multiple times consecutively, we only need one update.

No need for a lock really...
- add the new node (with a temporary 'wrong' nulls value),
- delete the old node
- replace the nulls value by the expected one.

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Jason Xing]

On Wed, Sep 3, 2025 at 2:40 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
> >
> > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> >
> > Since the lookup of sk in ehash is lockless, when one CPU is performing a
> > lookup while another CPU is executing delete and insert operations
> > (deleting reqsk and inserting sk), the lookup CPU may miss either of
> > them, if sk cannot be found, an RST may be sent.
> >
> > The call trace map is drawn as follows:
> >    CPU 0                           CPU 1
> >    -----                           -----
> >                                 spin_lock()
> >                                 sk_nulls_del_node_init_rcu(osk)
> > __inet_lookup_established()
> >                                 __sk_nulls_add_node_rcu(sk, list)
> >                                 spin_unlock()
> >
> > We can try using spin_lock()/spin_unlock() to wait for ehash updates
> > (ensuring all deletions and insertions are completed) after a failed
> > lookup in ehash, then lookup sk again after the update. Since the sk
> > expected to be found is unlikely to encounter the aforementioned scenario
> > multiple times consecutively, we only need one update.
>
> No need for a lock really...
> - add the new node (with a temporary 'wrong' nulls value),
> - delete the old node
> - replace the nulls value by the expected one.

Yes. The plan is simple enough to fix this particular issue and I
verified in production long ago. Sadly the following patch got
reverted...
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3f4ca5fafc08881d7a57daa20449d171f2887043

Thanks,
Jason

Re: [PATCH net] inet: Avoid established lookup missing active sk

[luoxuanqiang]

在 2025/9/3 13:48, Kuniyuki Iwashima 写道:
> On Tue, Sep 2, 2025 at 10:16 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>> On Tue, Sep 2, 2025 at 7:45 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>>
>>> Since the lookup of sk in ehash is lockless, when one CPU is performing a
>>> lookup while another CPU is executing delete and insert operations
>>> (deleting reqsk and inserting sk), the lookup CPU may miss either of
>>> them, if sk cannot be found, an RST may be sent.
>>>
>>> The call trace map is drawn as follows:
>>>     CPU 0                           CPU 1
>>>     -----                           -----
>>>                                  spin_lock()
>>>                                  sk_nulls_del_node_init_rcu(osk)
>>> __inet_lookup_established()
>>>                                  __sk_nulls_add_node_rcu(sk, list)
>>>                                  spin_unlock()
>> This usually does not happen except for local communication, and
>> retrying on the client side is much better than penalising all lookups
>> for SYN.
>>
>>> We can try using spin_lock()/spin_unlock() to wait for ehash updates
>>> (ensuring all deletions and insertions are completed) after a failed
>>> lookup in ehash, then lookup sk again after the update. Since the sk
>>> expected to be found is unlikely to encounter the aforementioned scenario
>>> multiple times consecutively, we only need one update.
>>>
>>> Similarly, an issue occurs in tw hashdance. Try adjusting the order in
>>> which it operates on ehash: remove sk first, then add tw. If sk is missed
>>> during lookup, it will likewise wait for the update to find tw, without
>>> worrying about the skc_refcnt issue that would arise if tw were found
>>> first.
>>>
>>> Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
>>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>> ---
>>>   net/ipv4/inet_hashtables.c    | 12 ++++++++++++
>>>   net/ipv4/inet_timewait_sock.c |  9 ++++-----
>>>   2 files changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>>> index ceeeec9b7290..4eb3a55b855b 100644
>>> --- a/net/ipv4/inet_hashtables.c
>>> +++ b/net/ipv4/inet_hashtables.c
>>> @@ -505,6 +505,7 @@ struct sock *__inet_lookup_established(const struct net *net,
>>>          unsigned int hash = inet_ehashfn(net, daddr, hnum, saddr, sport);
>>>          unsigned int slot = hash & hashinfo->ehash_mask;
>>>          struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
>>> +       bool try_lock = true;
>>>
>>>   begin:
>>>          sk_nulls_for_each_rcu(sk, node, &head->chain) {
>>> @@ -528,6 +529,17 @@ struct sock *__inet_lookup_established(const struct net *net,
>>>           */
>>>          if (get_nulls_value(node) != slot)
>>>                  goto begin;
>>> +
>>> +       if (try_lock) {
>>> +               spinlock_t *lock = inet_ehash_lockp(hashinfo, hash);
>>> +
>>> +               try_lock = false;
>>> +               spin_lock(lock);
>>> +               /* Ensure ehash ops under spinlock complete. */
>>> +               spin_unlock(lock);
>>> +               goto begin;
>>> +       }
>>> +
>>>   out:
>>>          sk = NULL;
>>>   found:
>>> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
>>> index 875ff923a8ed..a91e02e19c53 100644
>>> --- a/net/ipv4/inet_timewait_sock.c
>>> +++ b/net/ipv4/inet_timewait_sock.c
>>> @@ -139,14 +139,10 @@ 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);
>> You are adding a new RST scenario where the corresponding
>> socket is not found and a listener or no socket is found.
>>
>> The try_lock part is not guaranteed to happen after twsk
>> insertion below.

I'm sorry, I didn't get it. If try_lock can search again, other places 
should have left the spinlock critical section. That means twsk should 
have been inserted already. Or maybe I missed some details. Could you 
please explain more clearly? Your guidance would be really helpful. :)

> Oh no, spin_lock() dance sychronises the threads but I still
> think this is rather harmful for normal cases; now sending
> an unmatched packet can trigger lock dance, which is easily
> abused for DDoS.
>
I agree this scenario is possible. It does make DDoS attacks more 
impactful. Thanks for pointing that out. However, this is the best 
approach I can think of right now. Thanks Xuanqiang

>>
>>> -
>>> -       /* Step 3: Remove SK from hash chain */
>>> +       /* Step 2: 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().
>>> @@ -161,6 +157,9 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>>>           */
>>>          refcount_set(&tw->tw_refcnt, 3);
>>>
>>> +       /* Step 3: Hash TW into tcp ehash chain */
>>> +       inet_twsk_add_node_rcu(tw, &ehead->chain);
>>> +
>>>          inet_twsk_schedule(tw, timeo);
>>>
>>>          spin_unlock(lock);
>>> --
>>> 2.25.1
>>>

Re: [PATCH net] inet: Avoid established lookup missing active sk

[luoxuanqiang]

在 2025/9/3 14:52, Jason Xing 写道:
> On Wed, Sep 3, 2025 at 2:40 PM Eric Dumazet <edumazet@google.com> wrote:
>> On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>>
>>> Since the lookup of sk in ehash is lockless, when one CPU is performing a
>>> lookup while another CPU is executing delete and insert operations
>>> (deleting reqsk and inserting sk), the lookup CPU may miss either of
>>> them, if sk cannot be found, an RST may be sent.
>>>
>>> The call trace map is drawn as follows:
>>>     CPU 0                           CPU 1
>>>     -----                           -----
>>>                                  spin_lock()
>>>                                  sk_nulls_del_node_init_rcu(osk)
>>> __inet_lookup_established()
>>>                                  __sk_nulls_add_node_rcu(sk, list)
>>>                                  spin_unlock()
>>>
>>> We can try using spin_lock()/spin_unlock() to wait for ehash updates
>>> (ensuring all deletions and insertions are completed) after a failed
>>> lookup in ehash, then lookup sk again after the update. Since the sk
>>> expected to be found is unlikely to encounter the aforementioned scenario
>>> multiple times consecutively, we only need one update.
>> No need for a lock really...
>> - add the new node (with a temporary 'wrong' nulls value),
>> - delete the old node
>> - replace the nulls value by the expected one.
> Yes. The plan is simple enough to fix this particular issue and I
> verified in production long ago. Sadly the following patch got
> reverted...
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3f4ca5fafc08881d7a57daa20449d171f2887043
>
> Thanks,
> Jason

Yes, I'm fully aware of this history. I was excited when this issue was 
once fixed, because we've already encountered this type of RST issue 
many times.


Also, I'm sharing the link to our previous discussion about this type of 
issue. If other people see this email, it might be easier for them to 
get the full details：

https://lore.kernel.org/netdev/20230615121345.83597-1-duanmuquan@baidu.com/ 
https://lore.kernel.org/lkml/20230112065336.41034-1-kerneljasonxing@gmail.com/ 

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Eric Dumazet]

On Tue, Sep 2, 2025 at 11:53 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 2:40 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
> > >
> > > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> > >
> > > Since the lookup of sk in ehash is lockless, when one CPU is performing a
> > > lookup while another CPU is executing delete and insert operations
> > > (deleting reqsk and inserting sk), the lookup CPU may miss either of
> > > them, if sk cannot be found, an RST may be sent.
> > >
> > > The call trace map is drawn as follows:
> > >    CPU 0                           CPU 1
> > >    -----                           -----
> > >                                 spin_lock()
> > >                                 sk_nulls_del_node_init_rcu(osk)
> > > __inet_lookup_established()
> > >                                 __sk_nulls_add_node_rcu(sk, list)
> > >                                 spin_unlock()
> > >
> > > We can try using spin_lock()/spin_unlock() to wait for ehash updates
> > > (ensuring all deletions and insertions are completed) after a failed
> > > lookup in ehash, then lookup sk again after the update. Since the sk
> > > expected to be found is unlikely to encounter the aforementioned scenario
> > > multiple times consecutively, we only need one update.
> >
> > No need for a lock really...
> > - add the new node (with a temporary 'wrong' nulls value),
> > - delete the old node
> > - replace the nulls value by the expected one.
>
> Yes. The plan is simple enough to fix this particular issue and I
> verified in production long ago. Sadly the following patch got
> reverted...
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3f4ca5fafc08881d7a57daa20449d171f2887043

Please read again what I wrote, and compare it to your old patch.

- add the new node (with a temporary 'wrong' nulls value),
- delete the old node
- replace the nulls value by the expected one.

Can you see a difference ?

Re: [PATCH net] inet: Avoid established lookup missing active sk

[Jason Xing]

On Wed, Sep 3, 2025 at 4:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 2, 2025 at 11:53 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > On Wed, Sep 3, 2025 at 2:40 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
> > > >
> > > > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> > > >
> > > > Since the lookup of sk in ehash is lockless, when one CPU is performing a
> > > > lookup while another CPU is executing delete and insert operations
> > > > (deleting reqsk and inserting sk), the lookup CPU may miss either of
> > > > them, if sk cannot be found, an RST may be sent.
> > > >
> > > > The call trace map is drawn as follows:
> > > >    CPU 0                           CPU 1
> > > >    -----                           -----
> > > >                                 spin_lock()
> > > >                                 sk_nulls_del_node_init_rcu(osk)
> > > > __inet_lookup_established()
> > > >                                 __sk_nulls_add_node_rcu(sk, list)
> > > >                                 spin_unlock()
> > > >
> > > > We can try using spin_lock()/spin_unlock() to wait for ehash updates
> > > > (ensuring all deletions and insertions are completed) after a failed
> > > > lookup in ehash, then lookup sk again after the update. Since the sk
> > > > expected to be found is unlikely to encounter the aforementioned scenario
> > > > multiple times consecutively, we only need one update.
> > >
> > > No need for a lock really...
> > > - add the new node (with a temporary 'wrong' nulls value),
> > > - delete the old node
> > > - replace the nulls value by the expected one.
> >
> > Yes. The plan is simple enough to fix this particular issue and I
> > verified in production long ago. Sadly the following patch got
> > reverted...
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3f4ca5fafc08881d7a57daa20449d171f2887043
>
> Please read again what I wrote, and compare it to your old patch.
>
> - add the new node (with a temporary 'wrong' nulls value),
> - delete the old node
> - replace the nulls value by the expected one.
>
> Can you see a difference ?

IIUC, you use a temporary value. It would avoid two sockets appearing
in the list at the same time.

Thanks,
Jason

Re: [PATCH net] inet: Avoid established lookup missing active sk

[luoxuanqiang]

在 2025/9/3 14:40, Eric Dumazet 写道:
> On Tue, Sep 2, 2025 at 7:46 PM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> Since the lookup of sk in ehash is lockless, when one CPU is performing a
>> lookup while another CPU is executing delete and insert operations
>> (deleting reqsk and inserting sk), the lookup CPU may miss either of
>> them, if sk cannot be found, an RST may be sent.
>>
>> The call trace map is drawn as follows:
>>     CPU 0                           CPU 1
>>     -----                           -----
>>                                  spin_lock()
>>                                  sk_nulls_del_node_init_rcu(osk)
>> __inet_lookup_established()
>>                                  __sk_nulls_add_node_rcu(sk, list)
>>                                  spin_unlock()
>>
>> We can try using spin_lock()/spin_unlock() to wait for ehash updates
>> (ensuring all deletions and insertions are completed) after a failed
>> lookup in ehash, then lookup sk again after the update. Since the sk
>> expected to be found is unlikely to encounter the aforementioned scenario
>> multiple times consecutively, we only need one update.
> No need for a lock really...
> - add the new node (with a temporary 'wrong' nulls value),
> - delete the old node
> - replace the nulls value by the expected one.
>
Hi Eric, May I ask if your main purpose is to add a temporary 'wrong' 
nulls to trigger a re-lookup in the lookup, until the real new sk is 
successfully replaced, like the following code (rough code)?

Thanks
Xuanqiang

--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -526,6 +526,8 @@ struct sock *__inet_lookup_established(const struct net *net,
       * not the expected one, we must restart lookup.
       * We probably met an item that was moved to another chain.
       */
+
+    /* Here, temporary NULL values cause a re-lookup. */
      if (get_nulls_value(node) != slot)
          goto begin;
  out:
@@ -684,7 +686,34 @@ bool inet_ehash_insert(struct sock *sk, struct sock *osk, bool *found_dup_sk)
  
      spin_lock(lock);
      if (osk) {
+        struct hlist_nulls_node *i, *last = NULL, *n = &sk->sk_nulls_node;
          WARN_ON_ONCE(sk->sk_hash != osk->sk_hash);
+
+        /* find the last node in the list */
+        for (i = list->first; !is_a_nulls(i); i = i->next)
+            last = i;
+
+        if (last) {
+            /* save the original nulls values  */
+            i = last->next;
+
+            /* 1. add the temporary 'wrong' nulls value */
+            rcu_assign_pointer(hlist_nulls_next_rcu(last),
+                       (struct hlist_nulls_node *)NULLS_MARKER(NULL));
+
+            /* 2. delete the old node */
+            ret = sk_nulls_del_node_init_rcu(osk);
+
+            /* 3. add the new node */
+            if (ret) {
+                WRITE_ONCE(n->next, i);
+                n->pprev = &last->next;
+                rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
+            } else {
+                rcu_assign_pointer(hlist_nulls_next_rcu(last), i);
+            }
+            goto unlock;
+        }
          ret = sk_nulls_del_node_init_rcu(osk);
      } else if (found_dup_sk) {
          *found_dup_sk = inet_ehash_lookup_by_sk(sk, list);
@@ -695,6 +724,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;

[syzbot ci] Re: inet: Avoid established lookup missing active sk

[syzbot ci]

syzbot ci has tested the following series

[v1] inet: Avoid established lookup missing active sk
https://lore.kernel.org/all/20250903024406.2418362-1-xuanqiang.luo@linux.dev
* [PATCH net] inet: Avoid established lookup missing active sk

and found the following issue:
inconsistent lock state in valid_state

Full report is available here:
https://ci.syzbot.org/series/e3eb0778-d6ff-4b0c-ae24-a5451a3472cb

***

inconsistent lock state in valid_state

tree:      net
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/netdev/net.git
base:      788bc43d8330511af433bf282021a8fecb6b9009
arch:      amd64
compiler:  Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
config:    https://ci.syzbot.org/builds/4fba502f-1812-45fd-881c-e5889996074b/config
C repro:   https://ci.syzbot.org/findings/1fde3273-fc6e-4ca1-9a99-a8f866c822cd/c_repro
syz repro: https://ci.syzbot.org/findings/1fde3273-fc6e-4ca1-9a99-a8f866c822cd/syz_repro

================================
WARNING: inconsistent lock state
syzkaller #0 Not tainted
--------------------------------
inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
syz.0.17/5984 [HC0[0]:SC0[0]:HE1:SE1] takes:
ffffc90000069958 (&ptr[i]){+.?.}-{3:3}, at: spin_lock include/linux/spinlock.h:351 [inline]
ffffc90000069958 (&ptr[i]){+.?.}-{3:3}, at: __inet_lookup_established+0x71d/0x8d0 net/ipv4/inet_hashtables.c:537
{IN-SOFTIRQ-W} state was registered at:
  lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
  __raw_spin_lock include/linux/spinlock_api_smp.h:133 [inline]
  _raw_spin_lock+0x2e/0x40 kernel/locking/spinlock.c:154
  spin_lock include/linux/spinlock.h:351 [inline]
  __inet_lookup_established+0x71d/0x8d0 net/ipv4/inet_hashtables.c:537
  tcp_v4_early_demux+0x4e1/0x9d0 net/ipv4/tcp_ipv4.c:1995
  ip_rcv_finish_core+0x108e/0x1c00 net/ipv4/ip_input.c:346
  ip_list_rcv_finish net/ipv4/ip_input.c:616 [inline]
  ip_sublist_rcv+0x397/0x9b0 net/ipv4/ip_input.c:642
  ip_list_rcv+0x3e2/0x430 net/ipv4/ip_input.c:676
  __netif_receive_skb_list_ptype net/core/dev.c:6034 [inline]
  __netif_receive_skb_list_core+0x7d2/0x800 net/core/dev.c:6081
  __netif_receive_skb_list net/core/dev.c:6133 [inline]
  netif_receive_skb_list_internal+0x975/0xcc0 net/core/dev.c:6224
  gro_normal_list include/net/gro.h:532 [inline]
  gro_flush_normal include/net/gro.h:540 [inline]
  napi_complete_done+0x2f2/0x7c0 net/core/dev.c:6593
  e1000_clean+0xd0b/0x2b00 drivers/net/ethernet/intel/e1000/e1000_main.c:3815
  __napi_poll+0xc7/0x360 net/core/dev.c:7506
  napi_poll net/core/dev.c:7569 [inline]
  net_rx_action+0x707/0xe30 net/core/dev.c:7696
  handle_softirqs+0x286/0x870 kernel/softirq.c:579
  __do_softirq kernel/softirq.c:613 [inline]
  invoke_softirq kernel/softirq.c:453 [inline]
  __irq_exit_rcu+0xca/0x1f0 kernel/softirq.c:680
  irq_exit_rcu+0x9/0x30 kernel/softirq.c:696
  common_interrupt+0xbb/0xe0 arch/x86/kernel/irq.c:318
  asm_common_interrupt+0x26/0x40 arch/x86/include/asm/idtentry.h:693
  native_safe_halt arch/x86/include/asm/irqflags.h:48 [inline]
  pv_native_safe_halt+0x13/0x20 arch/x86/kernel/paravirt.c:81
  arch_safe_halt arch/x86/include/asm/paravirt.h:107 [inline]
  default_idle+0x13/0x20 arch/x86/kernel/process.c:757
  default_idle_call+0x74/0xb0 kernel/sched/idle.c:122
  cpuidle_idle_call kernel/sched/idle.c:190 [inline]
  do_idle+0x1e8/0x510 kernel/sched/idle.c:330
  cpu_startup_entry+0x44/0x60 kernel/sched/idle.c:428
  start_secondary+0x101/0x110 arch/x86/kernel/smpboot.c:315
  common_startup_64+0x13e/0x147
irq event stamp: 807
hardirqs last  enabled at (807): [<ffffffff8184e7fd>] __local_bh_enable_ip+0x12d/0x1c0 kernel/softirq.c:412
hardirqs last disabled at (805): [<ffffffff8184e79e>] __local_bh_enable_ip+0xce/0x1c0 kernel/softirq.c:389
softirqs last  enabled at (806): [<ffffffff8962777b>] local_bh_disable include/linux/bottom_half.h:20 [inline]
softirqs last  enabled at (806): [<ffffffff8962777b>] rcu_read_lock_bh include/linux/rcupdate.h:892 [inline]
softirqs last  enabled at (806): [<ffffffff8962777b>] __dev_queue_xmit+0x27b/0x3b50 net/core/dev.c:4650
softirqs last disabled at (798): [<ffffffff8962777b>] local_bh_disable include/linux/bottom_half.h:20 [inline]
softirqs last disabled at (798): [<ffffffff8962777b>] rcu_read_lock_bh include/linux/rcupdate.h:892 [inline]
softirqs last disabled at (798): [<ffffffff8962777b>] __dev_queue_xmit+0x27b/0x3b50 net/core/dev.c:4650

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&ptr[i]);
  <Interrupt>
    lock(&ptr[i]);

 *** DEADLOCK ***

1 lock held by syz.0.17/5984:
 #0: ffffffff8e139ee0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:331 [inline]
 #0: ffffffff8e139ee0 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:841 [inline]
 #0: ffffffff8e139ee0 (rcu_read_lock){....}-{1:3}, at: inet_diag_find_one_icsk+0x2e/0x790 net/ipv4/inet_diag.c:527

stack backtrace:
CPU: 0 UID: 0 PID: 5984 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
 print_usage_bug+0x297/0x2e0 kernel/locking/lockdep.c:4042
 valid_state+0xc3/0xf0 kernel/locking/lockdep.c:4056
 mark_lock_irq+0x36/0x390 kernel/locking/lockdep.c:4267
 mark_lock+0x11b/0x190 kernel/locking/lockdep.c:4753
 mark_usage kernel/locking/lockdep.c:-1 [inline]
 __lock_acquire+0x9e2/0xd20 kernel/locking/lockdep.c:5191
 lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
 __raw_spin_lock include/linux/spinlock_api_smp.h:133 [inline]
 _raw_spin_lock+0x2e/0x40 kernel/locking/spinlock.c:154
 spin_lock include/linux/spinlock.h:351 [inline]
 __inet_lookup_established+0x71d/0x8d0 net/ipv4/inet_hashtables.c:537
 __inet_lookup include/net/inet_hashtables.h:408 [inline]
 inet_lookup+0xc4/0x290 include/net/inet_hashtables.h:428
 inet_diag_find_one_icsk+0x1c1/0x790 net/ipv4/inet_diag.c:529
 inet_diag_dump_one_icsk+0xa4/0x520 net/ipv4/inet_diag.c:576
 inet_diag_cmd_exact+0x3d5/0x4e0 net/ipv4/inet_diag.c:628
 inet_diag_get_exact_compat net/ipv4/inet_diag.c:1406 [inline]
 inet_diag_rcv_msg_compat+0x2b5/0x3b0 net/ipv4/inet_diag.c:1428
 sock_diag_rcv_msg+0x4cc/0x600 net/core/sock_diag.c:-1
 netlink_rcv_skb+0x208/0x470 net/netlink/af_netlink.c:2552
 netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
 netlink_unicast+0x82f/0x9e0 net/netlink/af_netlink.c:1346
 netlink_sendmsg+0x805/0xb30 net/netlink/af_netlink.c:1896
 sock_sendmsg_nosec net/socket.c:714 [inline]
 __sock_sendmsg+0x21c/0x270 net/socket.c:729
 ____sys_sendmsg+0x505/0x830 net/socket.c:2614
 ___sys_sendmsg+0x21f/0x2a0 net/socket.c:2668
 __sys_sendmsg net/socket.c:2700 [inline]
 __do_sys_sendmsg net/socket.c:2705 [inline]
 __se_sys_sendmsg net/socket.c:2703 [inline]
 __x64_sys_sendmsg+0x19b/0x260 net/socket.c:2703
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f0e1a18ebe9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd085aadc8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007f0e1a3c5fa0 RCX: 00007f0e1a18ebe9
RDX: 0000000000000000 RSI: 0000200000000200 RDI: 0000000000000003
RBP: 00007f0e1a211e19 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007f0e1a3c5fa0 R14: 00007f0e1a3c5fa0 R15: 0000000000000003
 </TASK>


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.