From:   Benjamin Coddington <bcodding@redhat.com>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] SUNRPC: Fix a server shutdown leak
Date:   Wed, 08 Mar 2023 05:37:29 -0500
Message-ID: <F4367E72-25E7-432F-A19C-4A480DF63BD4@redhat.com>
In-Reply-To: <FA64D05E-9902-4C37-9F14-40E3EEA3B656@oracle.com>
References: <cover.1677877233.git.bcodding@redhat.com>
 <65d0248533fbdea2f6190faa1ee150d2d615344b.1677877233.git.bcodding@redhat.com>
 <FA64D05E-9902-4C37-9F14-40E3EEA3B656@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 7 Mar 2023, at 22:14, Chuck Lever III wrote:

>> On Mar 3, 2023, at 4:08 PM, Benjamin Coddington <bcodding@redhat.com> =
wrote:
>>
>> Fix a race where kthread_stop() may prevent the threadfn from ever get=
ting
>> called.  If that happens the svc_rqst will not be cleaned up.
>>
>> Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown")
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>> net/sunrpc/svc.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 1fd3f5e57285..fea7ce8fba14 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -798,6 +798,7 @@ svc_start_kthreads(struct svc_serv *serv, struct s=
vc_pool *pool, int nrservs)
>> static int
>> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nr=
servs)
>> {
>> + struct svc_rqst *rqstp;
>> struct task_struct *task;
>> unsigned int state =3D serv->sv_nrthreads-1;
>>
>> @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct s=
vc_pool *pool, int nrservs)
>> task =3D choose_victim(serv, pool, &state);
>> if (task =3D=3D NULL)
>> break;
>> - kthread_stop(task);
>> + rqstp =3D kthread_data(task);
>> + /* Did we lose a race to svo_function threadfn? */
>> + if (kthread_stop(task) =3D=3D -EINTR)
>> + svc_exit_thread(rqstp);
>> nrservs++;
>> } while (nrservs < 0);
>> return 0;
>> -- =

>> 2.31.1
>>
>
> Seems sensible, applied. Is there a bugzilla link that should be includ=
ed?

No, the issue was found in a private environment.

Ben