netdev - Re: [PATCH net v2] napi: fix race inside 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1632293267.9421082-1-xuanzhuo@linux.alibaba.com>
Date:   Wed, 22 Sep 2021 14:47:47 +0800
From:   Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org,@vger.kernel.org,
         linyunsheng@...wei.com,@vger.kernel.org,
         David S. Miller <davem@...emloft.net>,@vger.kernel.org,
         Eric Dumazet <edumazet@...gle.com>,@vger.kernel.org,
         Daniel Borkmann <daniel@...earbox.net>,@vger.kernel.org,
         Antoine Tenart <atenart@...nel.org>,@vger.kernel.org,
         Alexander Lobakin <alobakin@...me>,@vger.kernel.org,
         Wei Wang <weiwan@...gle.com>,@vger.kernel.org,
         Taehee Yoo <ap420073@...il.com>,@vger.kernel.org,
         Björn Töpel <bjorn@...nel.org>,@vger.kernel.org,
         Arnd Bergmann <arnd@...db.de>,@vger.kernel.org,
         Kumar Kartikeya Dwivedi <memxor@...il.com>,@vger.kernel.org,
         Neil Horman <nhorman@...hat.com>,@vger.kernel.org,
         Dust Li <dust.li@...ux.alibaba.com>@vger.kernel.org
Subject: Re: [PATCH net v2] napi: fix race inside napi_enable

On Mon, 20 Sep 2021 12:20:24 -0700, Jakub Kicinski <kuba@...nel.org> wrote:
> On Sat, 18 Sep 2021 16:52:32 +0800 Xuan Zhuo wrote:
> > The process will cause napi.state to contain NAPI_STATE_SCHED and
> > not in the poll_list, which will cause napi_disable() to get stuck.
> >
> > The prefix "NAPI_STATE_" is removed in the figure below, and
> > NAPI_STATE_HASHED is ignored in napi.state.
> >
> >                       CPU0       |                   CPU1       | napi.state
> > ===============================================================================
> > napi_disable()                   |                              | SCHED | NPSVC
> > napi_enable()                    |                              |
> > {                                |                              |
> >     smp_mb__before_atomic();     |                              |
> >     clear_bit(SCHED, &n->state); |                              | NPSVC
> >                                  | napi_schedule_prep()         | SCHED | NPSVC
> >                                  | napi_poll()                  |
> >                                  |   napi_complete_done()       |
> >                                  |   {                          |
> >                                  |      if (n->state & (NPSVC | | (1)
> >                                  |               _BUSY_POLL)))  |
> >                                  |           return false;      |
> >                                  |     ................         |
> >                                  |   }                          | SCHED | NPSVC
> >                                  |                              |
> >     clear_bit(NPSVC, &n->state); |                              | SCHED
> > }                                |                              |
> >                                  |                              |
> > napi_schedule_prep()             |                              | SCHED | MISSED (2)
> >
> > (1) Here return direct. Because of NAPI_STATE_NPSVC exists.
> > (2) NAPI_STATE_SCHED exists. So not add napi.poll_list to sd->poll_list
> >
> > Since NAPI_STATE_SCHED already exists and napi is not in the
> > sd->poll_list queue, NAPI_STATE_SCHED cannot be cleared and will always
> > exist.
> >
> > 1. This will cause this queue to no longer receive packets.
> > 2. If you encounter napi_disable under the protection of rtnl_lock, it
> >    will cause the entire rtnl_lock to be locked, affecting the overall
> >    system.
> >
> > This patch uses cmpxchg to implement napi_enable(), which ensures that
> > there will be no race due to the separation of clear two bits.
> >
> > Fixes: 2d8bff12699abc ("netpoll: Close race condition between poll_one_napi and napi_disable")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > Reviewed-by: Dust Li <dust.li@...ux.alibaba.com>
>
> Why don't you just invert the order of clearing the bits:

I think it should be an atomic operation. The original two-step clear itself is
problematic. So from this perspective, it is not just a solution to this
problem.

Thanks.

>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a796754f75cc..706eca8112c1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6953,8 +6953,8 @@ void napi_enable(struct napi_struct *n)
>  {
>         BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
>         smp_mb__before_atomic();
> -       clear_bit(NAPI_STATE_SCHED, &n->state);
>         clear_bit(NAPI_STATE_NPSVC, &n->state);
> +       clear_bit(NAPI_STATE_SCHED, &n->state);
>         if (n->dev->threaded && n->thread)
>                 set_bit(NAPI_STATE_THREADED, &n->state);
>  }
>
> That's simpler and symmetric with the disable path.