netdev - Re: [PATCH net] net: fix hangup on napi

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: <7ff0f0e6027c3b84b0d0e1d58096392bfc0fe806.camel@redhat.com>
Date:   Fri, 09 Apr 2021 11:24:07 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        Wei Wang <weiwan@...gle.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] net: fix hangup on napi_disable for threaded napi

On Wed, 2021-04-07 at 11:13 -0700, Jakub Kicinski wrote:
> On Wed, 07 Apr 2021 16:54:29 +0200 Paolo Abeni wrote:
> > > > I think in the above example even the normal processing will be
> > > > fooled?!? e.g. even without the napi_disable(), napi_thread_wait() will
> > > >  will miss the event/will not understand to it really own the napi and
> > > > will call schedule().
> > > > 
> > > > It looks a different problem to me ?!?
> > > > 
> > > > I *think* that replacing inside the napi_thread_wait() loop:
> > > > 
> > > > 	if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) 
> > > > 
> > > > with:
> > > > 
> > > > 	unsigned long state = READ_ONCE(napi->state);
> > > > 
> > > > 	if (state & NAPIF_STATE_SCHED &&
> > > > 	    !(state & (NAPIF_STATE_IN_BUSY_POLL | NAPIF_STATE_DISABLE)) 
> > > > 
> > > > should solve it and should also allow removing the
> > > > NAPI_STATE_SCHED_THREADED bit. I feel like I'm missing some relevant
> > > > point here.  
> > > 
> > > Heh, that's closer to the proposal Eric put forward.
> > > 
> > > I strongly dislike   
> > 
> > I guess that can't be addressed ;)
> 
> I'm not _that_ unreasonable, I hope :) if there is multiple people
> disagreeing with me then so be it.

I'm sorry, I mean no offence! Just joking about the fact that is
usually hard changing preferences ;)

> > If you have strong opinion against the above, the only other option I
> > can think of is patching napi_schedule_prep() to set
> > both NAPI_STATE_SCHED and NAPI_STATE_SCHED_THREADED if threaded mode is
> > enabled for the running NAPI. That looks more complex and error prone,
> > so I really would avoid that.
> > 
> > Any other better option?
> > 
> > Side note: regardless of the above, I think we still need something
> > similar to the code in this patch, can we address the different issues
> > separately?
> 
> Not sure what issues you're referring to.

The patch that started this thread was ment to address a slightly
different race: napi_disable() hanging because napi_threaded_poll()
don't clear the NAPI_STATE_SCHED even when owning the napi instance.

> Right, I think the problem is disable_pending check is out of place.
> 
> How about this:
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9d1a8fac793f..e53f8bfed6a1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7041,7 +7041,7 @@ static int napi_thread_wait(struct napi_struct *napi)
>  
>         set_current_state(TASK_INTERRUPTIBLE);
>  
> -       while (!kthread_should_stop() && !napi_disable_pending(napi)) {
> +       while (!kthread_should_stop()) {
>                 /* Testing SCHED_THREADED bit here to make sure the current
>                  * kthread owns this napi and could poll on this napi.
>                  * Testing SCHED bit is not enough because SCHED bit might be
> @@ -7049,8 +7049,14 @@ static int napi_thread_wait(struct napi_struct *napi)
>                  */
>                 if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) {
>                         WARN_ON(!list_empty(&napi->poll_list));
> -                       __set_current_state(TASK_RUNNING);
> -                       return 0;
> +                       if (unlikely(napi_disable_pending(napi))) {
> +                               clear_bit(NAPI_STATE_SCHED, &napi->state);
> +                               clear_bit(NAPI_STATE_SCHED_THREADED,
> +                                         &napi->state);
> +                       } else {
> +                               __set_current_state(TASK_RUNNING);
> +                               return 0;
> +                       }
>                 }
>  
>                 schedule();

It looks like the above works, and also fixes the problem I originally
reported. 

I think it can be simplified as the following - if NAPIF_STATE_DISABLE
is set, napi_threaded_poll()/__napi_poll() will return clearing the
SCHEDS bits after trying to poll the device:

---
diff --git a/net/core/dev.c b/net/core/dev.c
index d9db02d4e044..5cb6f411043d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6985,7 +6985,7 @@ static int napi_thread_wait(struct napi_struct *napi)
 
        set_current_state(TASK_INTERRUPTIBLE);
 
-       while (!kthread_should_stop() && !napi_disable_pending(napi)) {
+       while (!kthread_should_stop()) {
                /* Testing SCHED_THREADED bit here to make sure the current
                 * kthread owns this napi and could poll on this napi.
                 * Testing SCHED bit is not enough because SCHED bit might be

---

And works as intended here. I'll submit that formally, unless there are
objection.

Thanks!

Paolo
>