[PATCH 0/1] NFSv4 callback service ate my homework

[PATCH 0/1] NFSv4 callback service ate my homework

[Benjamin Coddington]

A system with very low autofs timeouts for many mounts of v4.0 exports,
while attempting to mount with the non-specific vers=4 option would run out
of memory every few days.  Each mount was doing the v4.2, v4.1, v4.0
negotiation and constantly setting up and tearing down the backchannel
service for positive minorversions.  We found a probable cause:
wake_up_process() isn't guarunteed to execute the threadfn; kthread_stop()
can race in and prevent it.

Benjamin Coddington (1):
  SUNRPC: Fix a server shutdown leak

 net/sunrpc/svc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.31.1

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

[Benjamin Coddington]

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 <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 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

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

[Chuck Lever III]

> 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 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 <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 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

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

[Benjamin Coddington]

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 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 <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 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

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

[Jeff Layton]

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 <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 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 <jlayton@kernel.org>