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

[Jeff Layton <jlayton@kernel.org>]

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>