netdev - Re: [PATCH net v2] net: fix race between napi kthread mode and busy poll

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: <CAEA6p_CJx7K1Fab1C0Qkw=1VNnDaV9qwB_UUtikPMoqNUUWJuA@mail.gmail.com>
Date:   Fri, 26 Feb 2021 17:02:17 -0800
From:   Wei Wang <weiwan@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Martin Zaharinov <micron10@...il.com>,
        Alexander Duyck <alexanderduyck@...com>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>
Subject: Re: [PATCH net v2] net: fix race between napi kthread mode and busy poll

On Fri, Feb 26, 2021 at 4:48 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 26 Feb 2021 16:30:47 -0800 Wei Wang wrote:
> >               thread = READ_ONCE(napi->thread);
> >               if (thread) {
> > +                     set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
> >                       wake_up_process(thread);
>
> What about the version which checks RUNNING? As long as
> wake_up_process() implies a barrier I _think_ it should
> work as well. Am I missing some case, or did you decide
> to go with the simpler/safer approach?


I assume you are referring to the following proposed patch in your
previous email right?
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4294,6 +4294,8 @@ static inline void ____napi_schedule(struct
softnet_data *sd,
                 */
                thread = READ_ONCE(napi->thread);
                if (thread) {
+                       if (thread->state == TASK_RUNNING)
+                               set_bit(NAPIF_STATE_SCHED_THREAD, &napi->state);
                        wake_up_process(thread);
                        return;
                }
@@ -6486,7 +6488,8 @@ bool napi_complete_done(struct napi_struct *n,
int work_done)
                WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));

                new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
-                             NAPIF_STATE_PREFER_BUSY_POLL);
+                             NAPIF_STATE_PREFER_BUSY_POLL |
+                             NAPIF_STATE_SCHED_THREAD);

                /* If STATE_MISSED was set, leave STATE_SCHED set,
                 * because we will call napi->poll() one more time.
@@ -6968,16 +6971,24 @@ static int napi_poll(struct napi_struct *n,
struct list_head *repoll)

 static int napi_thread_wait(struct napi_struct *napi)
 {
+       bool woken = false;
+
        set_current_state(TASK_INTERRUPTIBLE);

        while (!kthread_should_stop() && !napi_disable_pending(napi)) {
-               if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
+               unsigned long state = READ_ONCE(napi->state);
+
+               if ((state & NAPIF_STATE_SCHED) &&
+                   ((state & NAPIF_STATE_SCHED_THREAD) || woken)) {
                        WARN_ON(!list_empty(&napi->poll_list));
                        __set_current_state(TASK_RUNNING);
                        return 0;
+               } else {
+                       WARN_ON(woken);
                }

                schedule();
+               woken = true;
                set_current_state(TASK_INTERRUPTIBLE);
        }
        __set_current_state(TASK_RUNNING);

I don't think it is sufficient to only set SCHED_THREADED bit when the
thread is in RUNNING state.
In fact, the thread is most likely NOT in RUNNING mode before we call
wake_up_process() in ____napi_schedule(), because it has finished the
previous round of napi->poll() and SCHED bit was cleared, so
napi_thread_wait() sets the state to INTERRUPTIBLE and schedule() call
should already put it in sleep.