2023-03-03 21:09:28

by Benjamin Coddington

Subject: [PATCH 1/1] SUNRPC: Fix a server shutdown leak

Fix a race where kthread_stop() may prevent the threadfn from ever getting
called. If that happens the svc_rqst will not be cleaned up.

Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown")
Signed-off-by: Benjamin Coddington <[email protected]>
---
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 svc_pool *pool, int nrservs)
static int
svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
{
+ struct svc_rqst *rqstp;
struct task_struct *task;
unsigned int state = serv->sv_nrthreads-1;

@@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
task = choose_victim(serv, pool, &state);
if (task == NULL)
break;
- kthread_stop(task);
+ rqstp = kthread_data(task);
+ /* Did we lose a race to svo_function threadfn? */
+ if (kthread_stop(task) == -EINTR)
+ svc_exit_thread(rqstp);
nrservs++;
} while (nrservs < 0);
return 0;
--
2.31.1

2023-03-08 03:14:31

by Chuck Lever

Subject: Re: [PATCH 1/1] SUNRPC: Fix a server shutdown leak

> On Mar 3, 2023, at 4:08 PM, Benjamin Coddington <[email protected]> wrote:
>
> Fix a race where kthread_stop() may prevent the threadfn from ever getting
> called. If that happens the svc_rqst will not be cleaned up.
>
> Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> 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 svc_pool *pool, int nrservs)
> static int
> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> {
> + struct svc_rqst *rqstp;
> struct task_struct *task;
> unsigned int state = serv->sv_nrthreads-1;
>
> @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> task = choose_victim(serv, pool, &state);
> if (task == NULL)
> break;
> - kthread_stop(task);
> + rqstp = kthread_data(task);
> + /* Did we lose a race to svo_function threadfn? */
> + if (kthread_stop(task) == -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 included?


--
Chuck Lever

2023-03-08 10:38:51

by Benjamin Coddington

Subject: Re: [PATCH 1/1] SUNRPC: Fix a server shutdown leak

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

>> On Mar 3, 2023, at 4:08 PM, Benjamin Coddington <[email protected]> wrote:
>>
>> Fix a race where kthread_stop() may prevent the threadfn from ever getting
>> called. If that happens the svc_rqst will not be cleaned up.
>>
>> Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown")
>> Signed-off-by: Benjamin Coddington <[email protected]>
>> ---
>> 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 svc_pool *pool, int nrservs)
>> static int
>> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
>> {
>> + struct svc_rqst *rqstp;
>> struct task_struct *task;
>> unsigned int state = serv->sv_nrthreads-1;
>>
>> @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
>> task = choose_victim(serv, pool, &state);
>> if (task == NULL)
>> break;
>> - kthread_stop(task);
>> + rqstp = kthread_data(task);
>> + /* Did we lose a race to svo_function threadfn? */
>> + if (kthread_stop(task) == -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 included?

No, the issue was found in a private environment.

Ben

2023-03-08 10:48:29

by Jeffrey Layton

Subject: Re: [PATCH 1/1] SUNRPC: Fix a server shutdown leak

On Fri, 2023-03-03 at 16:08 -0500, Benjamin Coddington wrote:
> Fix a race where kthread_stop() may prevent the threadfn from ever getting
> called. If that happens the svc_rqst will not be cleaned up.
>
> Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> 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 svc_pool *pool, int nrservs)
> static int
> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> {
> + struct svc_rqst *rqstp;
> struct task_struct *task;
> unsigned int state = serv->sv_nrthreads-1;
>
> @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> task = choose_victim(serv, pool, &state);
> if (task == NULL)
> break;
> - kthread_stop(task);
> + rqstp = kthread_data(task);
> + /* Did we lose a race to svo_function threadfn? */
> + if (kthread_stop(task) == -EINTR)
> + svc_exit_thread(rqstp);
> nrservs++;
> } while (nrservs < 0);
> return 0;

Reviewed-by: Jeff Layton <[email protected]>