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] [day] [month] [year] [list]

Date:   Mon, 1 Mar 2021 10:16:45 -0800
From:   Wei Wang <weiwan@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Alexander Duyck <alexanderduyck@...com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Martin Zaharinov <micron10@...il.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 Sun, Feb 28, 2021 at 11:17 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 27 Feb 2021 15:23:56 -0800 Wei Wang wrote:
> > > > Indeed, looks like the task will be in WAKING state until it runs?
> > > > We can switch the check in ____napi_schedule() from
> > > >
> > > >         if (thread->state == TASK_RUNNING)
> > > >
> > > > to
> > > >
> > > >         if (!(thread->state & TASK_INTERRUPTIBLE))
> > > >
> > > > ?
> > >
> > > Hmm... I am not very sure what state the thread will be put in after
> > > kthread_create(). Could it be in TASK_INTERRUPTIBLE?
> >
> > I did a printk and confirmed that the thread->state is
> > TASK_UNINTERRUPTIBLE after kthread_create() is called.
> > So I think if we change the above state to:
> >           if (thread->state != TASK_INTERRUPTIBLE)
> >                   set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
> > It should work.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 6c5967e80132..43607523ee99 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1501,17 +1501,18 @@ static int napi_kthread_create(struct napi_struct *n)
> >  {
> >         int err = 0;
> >
> > -       /* Create and wake up the kthread once to put it in
> > -        * TASK_INTERRUPTIBLE mode to avoid the blocked task
> > -        * warning and work with loadavg.
> > +       /* Avoid waking up the kthread during creation to prevent
> > +        * potential race.
> >          */
> > -       n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
> > -                               n->dev->name, n->napi_id);
> > +       n->thread = kthread_create(napi_threaded_poll, n, "napi/%s-%d",
> > +                                  n->dev->name, n->napi_id);
>
> Does kthread_run() make the thread go into TASK_INTERRUPTIBLE ?
> It just calls wake_up_process(), which according to a comment in the
> kdoc..
>
>  * Conceptually does:
>  *
>  *   If (@state & @p->state) @p->state = TASK_RUNNING.
>
> So I think we could safely stick to kthread_run() if the condition in
> at the NAPI wake point checks for INTERRUPTIBLE?

I think so. kthread_run() wakes up the kthread and kthread_wait_poll()
should put it to INTERRUPTIBLE mode and schedule() will make it go to
sleep, and wait for the next napi_schedule().
I've also tested on my setup and saw no issues.