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: <20210228111710.4e82a88e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sun, 28 Feb 2021 11:17:10 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Wei Wang <weiwan@...gle.com>
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 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?