[PATCH v1] NFSD: Take a file unhashed during construction back off the LRU

[PATCH v1] NFSD: Take a file unhashed during construction back off the LRU

[Chuck Lever]

nfsd_file_do_acquire() inserts a new nfsd_file into the hash table,
opens the file, and only then adds the entry to the LRU. In between,
nfsd_file_cond_queue() can unhash the entry from any caller that
tests GC but not PENDING, such as nfsd_file_queue_for_close(), with
nothing on the LRU to remove. The constructor then calls
nfsd_file_lru_add(), so the entry sits on the LRU with HASHED clear.

The first laundrette pass after the caller's nfsd_file_put() reaps
the entry. nfsd_file_cache_shutdown_net() purges through the hash
table and misses the entry. With one net namespace, if
nfsd_file_cache_shutdown() runs before that laundrette pass,
list_lru_destroy() runs with the entry still linked, the struct file
stays open, and the export cannot be unmounted. With more than one
net namespace, the entry can outlive its namespace, because nf_net
does not hold a reference. A later laundrette pass then follows a
dangling nf_net pointer in nfsd_file_dispose_list_delayed().

A local F_SETLEASE, or nfsd_close_cached_files() on an NFS re-export,
reaches a file under construction when it races an open of the same
inode on another nfsd thread. A process that repeatedly takes and
releases a read lease on an exported file, while an NFSv3 client
reads that file, hits the window on three reads out of four. Before
commit 56221b42d717 ("nfsd: filecache: don't repeatedly add/remove
files on the lru list"), only nfsd_file_put() added an entry to the
LRU, after testing HASHED.

Test HASHED again after the LRU add. When the bit is clear, take the
entry off the LRU and drop the LRU's reference, so the caller's
nfsd_file_put() frees it.

Fixes: 56221b42d717 ("nfsd: filecache: don't repeatedly add/remove files on the lru list")
Assisted-by: https://github.com/brauner/linux-formal
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/filecache.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 42bd8c7859cf..ddca3d580019 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1311,8 +1311,17 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
 	 */
 	if (status != nfs_ok || inode->i_nlink == 0)
 		nfsd_file_unhash(nf);
-	else if (want_gc)
+	else if (want_gc) {
 		nfsd_file_lru_add(nf);
+		/*
+		 * nfsd_file_cond_queue() can unhash nf before the LRU add.
+		 * Only one LRU removal succeeds, and that one drops the LRU
+		 * reference.
+		 */
+		if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags) &&
+		    nfsd_file_lru_remove(nf))
+			refcount_dec(&nf->nf_ref);
+	}
 
 	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
 	if (status == nfs_ok)
-- 
2.54.0

Re: [PATCH v1] NFSD: Take a file unhashed during construction back off the LRU

[Jeff Layton]

On Sun, 2026-10-11 at 16:54 -0400, Chuck Lever wrote:
> nfsd_file_do_acquire() inserts a new nfsd_file into the hash table,
> opens the file, and only then adds the entry to the LRU. In between,
> nfsd_file_cond_queue() can unhash the entry from any caller that
> tests GC but not PENDING, such as nfsd_file_queue_for_close(), with
> nothing on the LRU to remove. The constructor then calls
> nfsd_file_lru_add(), so the entry sits on the LRU with HASHED clear.
> 
> The first laundrette pass after the caller's nfsd_file_put() reaps
> the entry. nfsd_file_cache_shutdown_net() purges through the hash
> table and misses the entry. With one net namespace, if
> nfsd_file_cache_shutdown() runs before that laundrette pass,
> list_lru_destroy() runs with the entry still linked, the struct file
> stays open, and the export cannot be unmounted. With more than one
> net namespace, the entry can outlive its namespace, because nf_net
> does not hold a reference. A later laundrette pass then follows a
> dangling nf_net pointer in nfsd_file_dispose_list_delayed().
> 
> A local F_SETLEASE, or nfsd_close_cached_files() on an NFS re-export,
> reaches a file under construction when it races an open of the same
> inode on another nfsd thread. A process that repeatedly takes and
> releases a read lease on an exported file, while an NFSv3 client
> reads that file, hits the window on three reads out of four. Before
> commit 56221b42d717 ("nfsd: filecache: don't repeatedly add/remove
> files on the lru list"), only nfsd_file_put() added an entry to the
> LRU, after testing HASHED.
> 
> Test HASHED again after the LRU add. When the bit is clear, take the
> entry off the LRU and drop the LRU's reference, so the caller's
> nfsd_file_put() frees it.
> 
> Fixes: 56221b42d717 ("nfsd: filecache: don't repeatedly add/remove files on the lru list")
> Assisted-by: https://github.com/brauner/linux-formal
> Signed-off-by: Chuck Lever <cel@kernel.org>
> ---
>  fs/nfsd/filecache.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 42bd8c7859cf..ddca3d580019 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1311,8 +1311,17 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
>  	 */
>  	if (status != nfs_ok || inode->i_nlink == 0)
>  		nfsd_file_unhash(nf);
> -	else if (want_gc)
> +	else if (want_gc) {
>  		nfsd_file_lru_add(nf);
> +		/*
> +		 * nfsd_file_cond_queue() can unhash nf before the LRU add.
> +		 * Only one LRU removal succeeds, and that one drops the LRU
> +		 * reference.
> +		 */
> +		if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags) &&
> +		    nfsd_file_lru_remove(nf))
> +			refcount_dec(&nf->nf_ref);
> +	}
>  
>  	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
>  	if (status == nfs_ok)

The problem and solution seem sane. IMO, the comment is not
particularly helpful and will be subject to bitrot (what if we change
the name of nfsd_file_cond_queue()?). For the code itself though:

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