MIME-Version: 1.0
References: <20211110191126.1214-1-alexandr.lobakin@intel.com>
In-Reply-To: <20211110191126.1214-1-alexandr.lobakin@intel.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Wed, 10 Nov 2021 11:24:39 -0800
Message-ID: <CANn89i+ZH83K9V7-7D6egC5AF=hxBv8FL+rroEqOskB-+TLZCA@mail.gmail.com>
Subject: Re: [PATCH net] net: fix premature exit from NAPI state polling in napi_disable()
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Michal Swiatkowski <michal.swiatkowski@intel.com>,
        Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
        Antoine Tenart <atenart@kernel.org>,
        Wei Wang <weiwan@google.com>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 10, 2021 at 11:11 AM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
>
> Commit 719c57197010 ("net: make napi_disable() symmetric with
> enable") accidentally introduced a bug sometimes leading to a kernel
> BUG when bringing an iface up/down under heavy traffic load.
>
> Prior to this commit, napi_disable() was polling n->state until
> none of (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC) is set and then
> always flip them. Now there's a possibility to get away with the
> NAPIF_STATE_SCHE unset as 'continue' drops us to the cmpxchg()
> call with an unitialized variable, rather than straight to
> another round of the state check.
>
> Error path looks like:
>
> napi_disable():
> unsigned long val, new; /* new is uninitialized */
>
> do {
>         val = READ_ONCE(n->state); /* NAPIF_STATE_NPSVC and/or
>                                       NAPIF_STATE_SCHED is set */
>         if (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) { /* true */
>                 usleep_range(20, 200);
>                 continue; /* go straight to the condition check */
>         }
>         new = val | <...>
> } while (cmpxchg(&n->state, val, new) != val); /* state == val, cmpxchg()
>                                                   writes garbage */
>
> napi_enable():
> do {
>         val = READ_ONCE(n->state);
>         BUG_ON(!test_bit(NAPI_STATE_SCHED, &val)); /* 50/50 boom */
> <...>
>
> while the typical BUG splat is like:
>
> [
> Fix this by replacing this 'continue' with a goto to the beginning
> of the loop body to restore the original behaviour.
> This could be written without a goto, but would look uglier and
> require one more indent level.
>
> Fixes: 719c57197010 ("net: make napi_disable() symmetric with enable")
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>  net/core/dev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index edeb811c454e..5e101c53b9de 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6929,10 +6929,11 @@ void napi_disable(struct napi_struct *n)
>         set_bit(NAPI_STATE_DISABLE, &n->state);
>
>         do {
> +retry:
>                 val = READ_ONCE(n->state);
>                 if (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
>                         usleep_range(20, 200);
> -                       continue;
> +                       goto retry;
>                 }
>
>                 new = val | NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC;
> --
> 2.33.1
>

Good catch !

What about replacing the error prone do {...} while (cmpxchg(..)) by
something less confusing ?

This way, no need for a label.

diff --git a/net/core/dev.c b/net/core/dev.c
index 5e37d6809317fb3c54686188a908bfcb0bfccdab..9327141892cdaaf0282e082e0c6746abae0f12a7
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6264,7 +6264,7 @@ void napi_disable(struct napi_struct *n)
        might_sleep();
        set_bit(NAPI_STATE_DISABLE, &n->state);

-       do {
+       for (;;) {
                val = READ_ONCE(n->state);
                if (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
                        usleep_range(20, 200);
@@ -6273,7 +6273,9 @@ void napi_disable(struct napi_struct *n)

                new = val | NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC;
                new &= ~(NAPIF_STATE_THREADED | NAPIF_STATE_PREFER_BUSY_POLL);
-       } while (cmpxchg(&n->state, val, new) != val);
+               if (cmpxchg(&n->state, val, new) == val)
+                       break;
+       }

        hrtimer_cancel(&n->timer);