[PATCH] lockd: hold a reference to nlmsvc_serv while stopping thread.

We are required to hold a reference to the svc_serv struct while
stopping the last thread, as doing that could otherwise drop the last
reference itself and the svc_serv would be freed while still in use.

lockd doesn't do this.  After startup, the only reference is held by the
running thread.

So change locked to hold a reference on nlmsvc_serv while-ever the
service is active, and only drop it after the last thread has been
stopped.

Note: it doesn't really make sense for threads to hold references to the
svc_serv any more.  The fact threads are included in serv->sv_nrthreads
is sufficient.  Maybe a future patch could address this.

Reported-by: Jeff Layton <jlayton@kernel.org>
Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
Signed-off-by: NeilBrown <redacted>
---

Thanks for the report Jeff !!!

 fs/lockd/svc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index b441c706c2b8..7a5c90a00522 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c

@@ -345,10 +345,10 @@ static int lockd_get(void)
 
 	serv->sv_maxconn = nlm_max_connections;
 	error = svc_set_num_threads(serv, NULL, 1);
-	/* The thread now holds the only reference */
-	svc_put(serv);
-	if (error < 0)
+	if (error < 0) {
+		svc_put(serv);
 		return error;
+	}
 
 	nlmsvc_serv = serv;
 	register_inetaddr_notifier(&lockd_inetaddr_notifier);

@@ -374,6 +374,7 @@ static void lockd_put(void)
 
 	svc_set_num_threads(nlmsvc_serv, NULL, 0);
 	timer_delete_sync(&nlmsvc_retry);
+	svc_put(nlmsvc_serv);
 	nlmsvc_serv = NULL;
 	dprintk("lockd_down: service destroyed\n");
 }

-- 
2.42.0

On Tue, 2023-10-10 at 10:35 +1100, NeilBrown wrote:

Thanks Neil! You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Tested-by: Jeff Layton <jlayton@kernel.org>

On Oct 9, 2023, at 7:35 PM, NeilBrown [off-list ref] wrote:
We are required to hold a reference to the svc_serv struct while
stopping the last thread, as doing that could otherwise drop the last
reference itself and the svc_serv would be freed while still in use.

lockd doesn't do this.  After startup, the only reference is held by the
running thread.

So change locked to hold a reference on nlmsvc_serv while-ever the
service is active, and only drop it after the last thread has been
stopped.

Note: it doesn't really make sense for threads to hold references to the
svc_serv any more.  The fact threads are included in serv->sv_nrthreads
is sufficient.  Maybe a future patch could address this.

Reported-by: Jeff Layton <jlayton@kernel.org>
Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
Signed-off-by: NeilBrown <redacted>

Thanks for the fast response!

Should I squash this into 68cc, or apply it before? I would
like to ensure that bisect works nicely over this series of
commits.


--
Chuck Lever

On Tue, 10 Oct 2023, Chuck Lever III wrote:

quoted

On Oct 9, 2023, at 7:35 PM, NeilBrown [off-list ref] wrote:
We are required to hold a reference to the svc_serv struct while
stopping the last thread, as doing that could otherwise drop the last
reference itself and the svc_serv would be freed while still in use.

lockd doesn't do this.  After startup, the only reference is held by the
running thread.

So change locked to hold a reference on nlmsvc_serv while-ever the
service is active, and only drop it after the last thread has been
stopped.

Note: it doesn't really make sense for threads to hold references to the
svc_serv any more.  The fact threads are included in serv->sv_nrthreads
is sufficient.  Maybe a future patch could address this.

Reported-by: Jeff Layton <jlayton@kernel.org>
Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
Signed-off-by: NeilBrown <redacted>

Thanks for the fast response!

Should I squash this into 68cc, or apply it before? I would
like to ensure that bisect works nicely over this series of
commits.

Probably makes sense to put it before.  In that case the patch
description needs re-wording.

And on reflection I think the code should be changed a little so that it
matches similar code in nfsd and nfs4-callback.
So I'll repost.
I'll take the liberty of preserving Jeff's review/test even though I've
changed the code .... I hope that's OK.

NeilBrown

On Oct 10, 2023, at 6:30 PM, NeilBrown [off-list ref] wrote:

On Tue, 10 Oct 2023, Chuck Lever III wrote:

quoted

quoted

On Oct 9, 2023, at 7:35 PM, NeilBrown [off-list ref] wrote:
We are required to hold a reference to the svc_serv struct while
stopping the last thread, as doing that could otherwise drop the last
reference itself and the svc_serv would be freed while still in use.

lockd doesn't do this.  After startup, the only reference is held by the
running thread.

So change locked to hold a reference on nlmsvc_serv while-ever the
service is active, and only drop it after the last thread has been
stopped.

Note: it doesn't really make sense for threads to hold references to the
svc_serv any more.  The fact threads are included in serv->sv_nrthreads
is sufficient.  Maybe a future patch could address this.

Reported-by: Jeff Layton <jlayton@kernel.org>
Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
Signed-off-by: NeilBrown <redacted>

Thanks for the fast response!

Should I squash this into 68cc, or apply it before? I would
like to ensure that bisect works nicely over this series of
commits.

Probably makes sense to put it before.  In that case the patch
description needs re-wording.

And on reflection I think the code should be changed a little so that it
matches similar code in nfsd and nfs4-callback.
So I'll repost.
I'll take the liberty of preserving Jeff's review/test even though I've
changed the code .... I hope that's OK.

Sounds like a plan. I've picked up v2 and applied it to nfsd-next.


--
Chuck Lever