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: <20210407111318.39c2374d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 7 Apr 2021 11:13:18 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Paolo Abeni <pabeni@...hat.com>
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, 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.

> > the idea that every NAPI consumer needs to be aware
> > of all the other consumers to make things work. That's n^2 mental
> > complexity.  
> 
> IMHO the overall complexity is not that bad: both napi_disable() and
> NAPI poll already set their own specific NAPI bit when acquiring the
> NAPI instance, they don't need to be aware of any other NAPI consumer
> internal.
> 
> The only NAPI user that needs to be aware of others is napi threaded,
> and I guess/hope we are not going to add more different kind of NAPI
> users.

I thought we agreed that we should leave the door open for other
pollers as a condition of merging this simplistic thread thing.

> 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.

> > > Here I do not follow?!? Modulo the tiny race (which i was unable to
> > > trigger so far) above napi_disable()/napi_enable() loops work correctly
> > > here.
> > > 
> > > Could you please re-phrase?  
> > 
> > After napi_disable() the thread will exit right? (napi_thread_wait()
> > returns -1, the loop in napi_threaded_poll() breaks, and the function
> > returns).
> > 
> > napi_enable() will not re-start the thread.
> > 
> > What driver are you testing with? You driver must always call
> > netif_napi_del() and netif_napi_add().  
> 
> veth + some XDP dummy prog - used just to enable NAPI.
> 
> Yep, it does a full netif_napi_del()/netif_napi_add().
> 
> Looks like plain napi_disable()/napi_enable() is not going to work in
> threaded mode.

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();