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_B6baYFZnEOMS=Nmvg0kA_qB=7ip4S96ys9ZoJWfOiOCA@mail.gmail.com>
Date:   Fri, 26 Feb 2021 17:35:21 -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 5:22 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 26 Feb 2021 17:02:17 -0800 Wei Wang wrote:
> >  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.
>
> That's why the check says "|| woken":
>
>         ((state & NAPIF_STATE_SCHED_THREAD) ||  woken))
>
> thread knows it owns the NAPI if:
>
>   (a) the NAPI has the explicit flag set
> or
>   (b) it was just worken up and !kthread_should_stop(), since only
>       someone who just claimed the normal SCHED on thread's behalf
>       will wake it up

The 'woken' is set after schedule(). If it is the first time
napi_threaded_wait() is called, and SCHED_THREADED is not set, and
woken is not set either, this thread will be put to sleep when it
reaches schedule(), even though there is work waiting to be done on
that napi. And I think this kthread will not be woken up again
afterwards, since the SCHED bit is already grabbed.