[PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[NeilBrown]

Mike reports ongoing problem with leakage of refcounts for the net in
nfsd when localio is used.  I believe the first patch fixes one possible
cause.  The second patch removes some related dead code.

Mike: thanks for your testing so far.  Hopefully you could find time to
test this one too?

Thanks,
NeilBrown

 [PATCH 1/2] nfsd: avoid ref leak in nfsd_open_local_fh()
 [PATCH 2/2] nfsd: discard nfsd_file_get_local()

[PATCH 1/2] nfsd: avoid ref leak in nfsd_open_local_fh()

[NeilBrown]

If two calls to nfsd_open_local_fh() race and both successfully call
nfsd_file_acquire_local(), they will both get an extra reference to the
net to accompany the file reference stored in *pnf.

One of them will fail to store (using xchg()) the file reference in
*pnf and will drop that reference but WONT drop the accompanying
reference to the net.  This leak means that when the nfs server is shut
down it will hang in nfsd_shutdown_net() waiting for
&nn->nfsd_net_free_done.

This patch adds the missing nfsd_net_put().

Reported-by: Mike Snitzer <snitzer@kernel.org>
Fixes: e6f7e1487ab5 ("nfs_localio: simplify interface to nfsd for getting nfsd_file")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/localio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
index 80d9ff6608a7..519bbdedcb11 100644
--- a/fs/nfsd/localio.c
+++ b/fs/nfsd/localio.c
@@ -103,10 +103,11 @@ nfsd_open_local_fh(struct net *net, struct auth_domain *dom,
 			if (nfsd_file_get(new) == NULL)
 				goto again;
 			/*
-			 * Drop the ref we were going to install and the
-			 * one we were going to return.
+			 * Drop the ref we were going to install (both file and
+			 * net) and the one we were going to return (only file).
 			 */
 			nfsd_file_put(localio);
+			nfsd_net_put(net);
 			nfsd_file_put(localio);
 			localio = new;
 		}
-- 
2.49.0

[PATCH 2/2] nfsd: discard nfsd_file_get_local()

[NeilBrown]

This interface was deprecated by

Commit e6f7e1487ab5 ("nfs_localio: simplify interface to nfsd for getting nfsd_file")

and is now unused. So let's remove it.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/filecache.c        | 21 ---------------------
 fs/nfsd/filecache.h        |  1 -
 fs/nfsd/localio.c          |  1 -
 include/linux/nfslocalio.h |  1 -
 4 files changed, 24 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e108b6c705b4..fed80d5d60e7 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -391,27 +391,6 @@ nfsd_file_put_local(struct nfsd_file __rcu **pnf)
 	return net;
 }
 
-/**
- * nfsd_file_get_local - get nfsd_file reference and reference to net
- * @nf: nfsd_file of which to put the reference
- *
- * Get reference to both the nfsd_file and nf->nf_net.
- */
-struct nfsd_file *
-nfsd_file_get_local(struct nfsd_file *nf)
-{
-	struct net *net = nf->nf_net;
-
-	if (nfsd_net_try_get(net)) {
-		nf = nfsd_file_get(nf);
-		if (!nf)
-			nfsd_net_put(net);
-	} else {
-		nf = NULL;
-	}
-	return nf;
-}
-
 /**
  * nfsd_file_file - get the backing file of an nfsd_file
  * @nf: nfsd_file of which to access the backing file.
diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index 722b26c71e45..24ddf60e8434 100644
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h
@@ -63,7 +63,6 @@ int nfsd_file_cache_start_net(struct net *net);
 void nfsd_file_cache_shutdown_net(struct net *net);
 void nfsd_file_put(struct nfsd_file *nf);
 struct net *nfsd_file_put_local(struct nfsd_file __rcu **nf);
-struct nfsd_file *nfsd_file_get_local(struct nfsd_file *nf);
 struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
 struct file *nfsd_file_file(struct nfsd_file *nf);
 void nfsd_file_close_inode_sync(struct inode *inode);
diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
index 519bbdedcb11..bcb2258f9309 100644
--- a/fs/nfsd/localio.c
+++ b/fs/nfsd/localio.c
@@ -122,7 +122,6 @@ static const struct nfsd_localio_operations nfsd_localio_ops = {
 	.nfsd_net_put  = nfsd_net_put,
 	.nfsd_open_local_fh = nfsd_open_local_fh,
 	.nfsd_file_put_local = nfsd_file_put_local,
-	.nfsd_file_get_local = nfsd_file_get_local,
 	.nfsd_file_file = nfsd_file_file,
 };
 
diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
index 5c7c92659e73..59ea90bd136b 100644
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -63,7 +63,6 @@ struct nfsd_localio_operations {
 						struct nfsd_file __rcu **pnf,
 						const fmode_t);
 	struct net *(*nfsd_file_put_local)(struct nfsd_file __rcu **);
-	struct nfsd_file *(*nfsd_file_get_local)(struct nfsd_file *);
 	struct file *(*nfsd_file_file)(struct nfsd_file *);
 } ____cacheline_aligned;
 
-- 
2.49.0

Re: [PATCH 1/2] nfsd: avoid ref leak in nfsd_open_local_fh()

[Mike Snitzer]

On Fri, Jul 18, 2025 at 11:26:14AM +1000, NeilBrown wrote:
> If two calls to nfsd_open_local_fh() race and both successfully call
> nfsd_file_acquire_local(), they will both get an extra reference to the
> net to accompany the file reference stored in *pnf.
> 
> One of them will fail to store (using xchg()) the file reference in
> *pnf and will drop that reference but WONT drop the accompanying
> reference to the net.  This leak means that when the nfs server is shut
> down it will hang in nfsd_shutdown_net() waiting for
> &nn->nfsd_net_free_done.
> 
> This patch adds the missing nfsd_net_put().
> 
> Reported-by: Mike Snitzer <snitzer@kernel.org>
> Fixes: e6f7e1487ab5 ("nfs_localio: simplify interface to nfsd for getting nfsd_file")
> Signed-off-by: NeilBrown <neil@brown.name>

Tested-by: Mike Snitzer <snitzer@kernel.org>
Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Looks really solid now, thanks!

Mike

Re: [PATCH 2/2] nfsd: discard nfsd_file_get_local()

[Mike Snitzer]

On Fri, Jul 18, 2025 at 11:26:15AM +1000, NeilBrown wrote:
> This interface was deprecated by
> 
> Commit e6f7e1487ab5 ("nfs_localio: simplify interface to nfsd for getting nfsd_file")
> 
> and is now unused. So let's remove it.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Re: [PATCH 1/2] nfsd: avoid ref leak in nfsd_open_local_fh()

[NeilBrown]

On Fri, 18 Jul 2025, Mike Snitzer wrote:
> On Fri, Jul 18, 2025 at 11:26:14AM +1000, NeilBrown wrote:
> > If two calls to nfsd_open_local_fh() race and both successfully call
> > nfsd_file_acquire_local(), they will both get an extra reference to the
> > net to accompany the file reference stored in *pnf.
> > 
> > One of them will fail to store (using xchg()) the file reference in
> > *pnf and will drop that reference but WONT drop the accompanying
> > reference to the net.  This leak means that when the nfs server is shut
> > down it will hang in nfsd_shutdown_net() waiting for
> > &nn->nfsd_net_free_done.
> > 
> > This patch adds the missing nfsd_net_put().
> > 
> > Reported-by: Mike Snitzer <snitzer@kernel.org>
> > Fixes: e6f7e1487ab5 ("nfs_localio: simplify interface to nfsd for getting nfsd_file")
> > Signed-off-by: NeilBrown <neil@brown.name>
> 
> Tested-by: Mike Snitzer <snitzer@kernel.org>
> Reviewed-by: Mike Snitzer <snitzer@kernel.org>
> 
> Looks really solid now, thanks!
> 
> Mike
> 

Thanks Mike - I appreciate the prompt testing!

NeilBrown

Re: [PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[Jeff Layton]

On Fri, 2025-07-18 at 11:26 +1000, NeilBrown wrote:
> Mike reports ongoing problem with leakage of refcounts for the net in
> nfsd when localio is used.  I believe the first patch fixes one possible
> cause.  The second patch removes some related dead code.
> 
> Mike: thanks for your testing so far.  Hopefully you could find time to
> test this one too?
> 
> Thanks,
> NeilBrown
> 
>  [PATCH 1/2] nfsd: avoid ref leak in nfsd_open_local_fh()
>  [PATCH 2/2] nfsd: discard nfsd_file_get_local()

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

Re: [PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Fri, 18 Jul 2025 11:26:13 +1000, NeilBrown wrote:
> Mike reports ongoing problem with leakage of refcounts for the net in
> nfsd when localio is used.  I believe the first patch fixes one possible
> cause.  The second patch removes some related dead code.
> 
> Mike: thanks for your testing so far.  Hopefully you could find time to
> test this one too?
> 
> [...]

Applied to nfsd-testing, thanks!

I assume 1/2 should be expedited? If so, I can add a Cc: stable and
get it into v6.17-rc.

[1/2] nfsd: avoid ref leak in nfsd_open_local_fh()
      commit: 3ae40032ec9e2a829fb5ab1ebaa92d5d2b1fae89
[2/2] nfsd: discard nfsd_file_get_local()
      commit: 470c6ae5e920028abc2ef0044a05023977c4058c

--
Chuck Lever

Re: [PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[Mike Snitzer]

On Fri, Jul 18, 2025 at 10:25:26AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> On Fri, 18 Jul 2025 11:26:13 +1000, NeilBrown wrote:
> > Mike reports ongoing problem with leakage of refcounts for the net in
> > nfsd when localio is used.  I believe the first patch fixes one possible
> > cause.  The second patch removes some related dead code.
> > 
> > Mike: thanks for your testing so far.  Hopefully you could find time to
> > test this one too?
> > 
> > [...]
> 
> Applied to nfsd-testing, thanks!
> 
> I assume 1/2 should be expedited? If so, I can add a Cc: stable and
> get it into v6.17-rc.

Yes, it should be expedited.

Thanks,
Mike

> 
> [1/2] nfsd: avoid ref leak in nfsd_open_local_fh()
>       commit: 3ae40032ec9e2a829fb5ab1ebaa92d5d2b1fae89
> [2/2] nfsd: discard nfsd_file_get_local()
>       commit: 470c6ae5e920028abc2ef0044a05023977c4058c
> 
> --
> Chuck Lever
> 

Re: [PATCH 0/2 RFT] nfsd: fix another problem with recent localio changes

[Mike Snitzer]

On Fri, Jul 18, 2025 at 10:27:03AM -0400, Mike Snitzer wrote:
> On Fri, Jul 18, 2025 at 10:25:26AM -0400, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > On Fri, 18 Jul 2025 11:26:13 +1000, NeilBrown wrote:
> > > Mike reports ongoing problem with leakage of refcounts for the net in
> > > nfsd when localio is used.  I believe the first patch fixes one possible
> > > cause.  The second patch removes some related dead code.
> > > 
> > > Mike: thanks for your testing so far.  Hopefully you could find time to
> > > test this one too?
> > > 
> > > [...]
> > 
> > Applied to nfsd-testing, thanks!
> > 
> > I assume 1/2 should be expedited? If so, I can add a Cc: stable and
> > get it into v6.17-rc.
> 
> Yes, it should be expedited.

BTW, Neil's LOCALIO changes that were merged during 6.16 merge window
have dependencies on his generic wait_on_var advances.  So all these
LOCALIO 6.16 changes won't have an easy time of getting back to 6.12
stable@ for example.

But given the Fixes: in the LOCALIO changes that were merged to 6.16
I'd imagine 6.14 stable@ _could_ pick them up... so explicitly tagging
this fix with "Cc: stable" likely makes sense.  But had they picked up
the 6.16 patches for 6.14 stable@ then they should know to pick up
this fix anyway (as side-effect of Neil's Fixes: tag).

Chuck, pretty certain you know all this.. I'm just sharing what I know
for others' benefit.