[PATCH] nfsd: don't repurpose nf_lru while there are other references outstanding

[PATCH] nfsd: don't repurpose nf_lru while there are other references outstanding

[Jeff Layton]

When testing with a backport of the latest nfsd_file overhaul patches
this week, I hit some list corruption with the nf->nf_lru list_head.
Analysis of the vmcore suggested that a nf_lru had been reinitialized
while it was sitting on the LRU.

It's not safe to use the nf_lru list_head for anything but the LRU
unless you can be sure that no other references can be taken, ensuring
that you have exclusive access to it.

In practical terms, this means that we can't queue objects to a dispose
list until their refcounts have gone to zero. nfsd_unhash_and_queue
currently violates this rule, as it queues nfsd_files to the dispose
list with active references held.

This function is called both during server shutdown and when there is
conflicting access to an inode; two situations that have little to do
with one another. Remove nfsd_file_unhash_and_queue altogether, as its
callers need different things, and just open code what its callers need.

Rename __nfsd_file_close_inode and rework the loop in it to put the
reference(s) earlier and only queue the nf to the dispose list if its
refcount went to zero.

__nfsd_file_cache_purge is called during server shutdown. At that point,
we don't care about the refcount since we know we have exclusive access.
Just unhash the objects, dequeue them from the LRU and then free them.

Finally, update the comments over nfsd_file_close_inode_sync,
nfsd_file_close_inode and __nfsd_file_cache_purge to better describe
their intended purpose.

Fixes: d2cdf429693a ("nfsd: rework refcounting in filecache")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 111 +++++++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 53 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 1e76b0d3b83a..8bf2fcb0f580 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -461,35 +461,6 @@ nfsd_file_get(struct nfsd_file *nf)
 	return NULL;
 }
 
-/**
- * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list
- * @nf: nfsd_file to be unhashed and queued
- * @dispose: list to which it should be queued
- *
- * Attempt to unhash a nfsd_file and queue it to the given list. Each file
- * will have a reference held on behalf of the list. That reference may come
- * from the LRU, or we may need to take one. If we can't get a reference,
- * ignore it altogether.
- */
-static bool
-nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)
-{
-	trace_nfsd_file_unhash_and_queue(nf);
-	if (nfsd_file_unhash(nf)) {
-		/*
-		 * If we remove it from the LRU, then just use that
-		 * reference for the dispose list. Otherwise, we need
-		 * to take a reference. If that fails, just ignore
-		 * the file altogether.
-		 */
-		if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
-			return false;
-		list_add(&nf->nf_lru, dispose);
-		return true;
-	}
-	return false;
-}
-
 /**
  * nfsd_file_put - put the reference to a nfsd_file
  * @nf: nfsd_file of which to put the reference
@@ -700,15 +671,24 @@ static struct shrinker	nfsd_file_shrinker = {
 	.seeks = 1,
 };
 
-/*
- * Find all cache items across all net namespaces that match @inode, unhash
- * them, take references and then put them on @dispose if that was successful.
+/**
+ * nfsd_file_queue_for_close: try to close out any open nfsd_files for an inode
+ * @inode:   inode on which to close out nfsd_files
+ * @dispose: list on which to gather nfsd_files to close out
+ *
+ * An nfsd_file represents a struct file being held open on behalf of nfsd. An
+ * open file however can block other activity (such as leases), or cause
+ * undesirable behavior (e.g. spurious silly-renames when reexporting NFS).
+ *
+ * This function is intended to find open nfsd_files when this sort of
+ * conflicting access occurs and then attempt to close those files out.
  *
- * The nfsd_file objects on the list will be unhashed, and each will have a
- * reference taken.
+ * Populates the dispose list with entries that have already had their
+ * refcounts go to zero. The actual free of an nfsd_file can be expensive,
+ * so we leave it up to the caller whether it wants to wait or not.
  */
 static void
-__nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
+nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
 {
 	struct nfsd_file_lookup_key key = {
 		.type	= NFSD_FILE_KEY_INODE,
@@ -718,12 +698,30 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
 
 	rcu_read_lock();
 	do {
+		int decrement = 1;
+
 		nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
 				       nfsd_file_rhash_params);
 		if (!nf)
 			break;
 
-		nfsd_file_unhash_and_queue(nf, dispose);
+		/* If we raced with someone else unhashing, ignore it */
+		if (!nfsd_file_unhash(nf))
+			continue;
+
+		/* If we can't get a reference, ignore it */
+		if (!nfsd_file_get(nf))
+			continue;
+
+		/* Extra decrement if we remove from the LRU */
+		if (nfsd_file_lru_remove(nf))
+			++decrement;
+
+		/* If refcount goes to 0, then put on the dispose list */
+		if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
+			list_add(&nf->nf_lru, dispose);
+			trace_nfsd_file_closing(nf);
+		}
 	} while (1);
 	rcu_read_unlock();
 }
@@ -732,22 +730,17 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
  * nfsd_file_close_inode - attempt a delayed close of a nfsd_file
  * @inode: inode of the file to attempt to remove
  *
- * Unhash and put all cache item associated with @inode. Queue any that have
- * refcounts that go to zero to the dispose_list_delayed infrastructure for
- * eventual cleanup.
+ * Close out any open nfsd_files that can be reaped for @inode. The
+ * actual freeing is deferred to the dispose_list_delayed infrastructure.
+ *
+ * This is used by the fsnotify callbacks and setlease notifier.
  */
 static void
 nfsd_file_close_inode(struct inode *inode)
 {
-	struct nfsd_file *nf, *tmp;
 	LIST_HEAD(dispose);
 
-	__nfsd_file_close_inode(inode, &dispose);
-	list_for_each_entry_safe(nf, tmp, &dispose, nf_lru) {
-		trace_nfsd_file_closing(nf);
-		if (!refcount_dec_and_test(&nf->nf_ref))
-			list_del_init(&nf->nf_lru);
-	}
+	nfsd_file_queue_for_close(inode, &dispose);
 	nfsd_file_dispose_list_delayed(&dispose);
 }
 
@@ -755,7 +748,11 @@ nfsd_file_close_inode(struct inode *inode)
  * nfsd_file_close_inode_sync - attempt to forcibly close a nfsd_file
  * @inode: inode of the file to attempt to remove
  *
- * Unhash and put, then flush and fput all cache items associated with @inode.
+ * Close out any open nfsd_files that can be reaped for @inode. The
+ * nfsd_files are closed out synchronously.
+ *
+ * This is called from nfsd_rename and nfsd_unlink to avoid silly-renames
+ * when reexporting NFS.
  */
 void
 nfsd_file_close_inode_sync(struct inode *inode)
@@ -765,13 +762,11 @@ nfsd_file_close_inode_sync(struct inode *inode)
 
 	trace_nfsd_file_close(inode);
 
-	__nfsd_file_close_inode(inode, &dispose);
+	nfsd_file_queue_for_close(inode, &dispose);
 	while (!list_empty(&dispose)) {
 		nf = list_first_entry(&dispose, struct nfsd_file, nf_lru);
 		list_del_init(&nf->nf_lru);
-		trace_nfsd_file_closing(nf);
-		if (refcount_dec_and_test(&nf->nf_ref))
-			nfsd_file_free(nf);
+		nfsd_file_free(nf);
 	}
 	flush_delayed_fput();
 }
@@ -923,6 +918,13 @@ nfsd_file_cache_init(void)
 	goto out;
 }
 
+/**
+ * __nfsd_file_cache_purge: clean out the cache for shutdown
+ * @net: net-namespace to shut down the cache (may be NULL)
+ *
+ * Walk the nfsd_file cache and close out any that match @net. If @net is NULL,
+ * then close out everything. Called when an nfsd instance is being shut down.
+ */
 static void
 __nfsd_file_cache_purge(struct net *net)
 {
@@ -936,8 +938,11 @@ __nfsd_file_cache_purge(struct net *net)
 
 		nf = rhashtable_walk_next(&iter);
 		while (!IS_ERR_OR_NULL(nf)) {
-			if (!net || nf->nf_net == net)
-				nfsd_file_unhash_and_queue(nf, &dispose);
+			if (!net || nf->nf_net == net) {
+				nfsd_file_unhash(nf);
+				nfsd_file_lru_remove(nf);
+				list_add(&nf->nf_lru, &dispose);
+			}
 			nf = rhashtable_walk_next(&iter);
 		}
 
-- 
2.38.1

Re: [PATCH] nfsd: don't repurpose nf_lru while there are other references outstanding

[Chuck Lever III]

> On Dec 7, 2022, at 4:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> When testing with a backport of the latest nfsd_file overhaul patches
> this week, I hit some list corruption with the nf->nf_lru list_head.
> Analysis of the vmcore suggested that a nf_lru had been reinitialized
> while it was sitting on the LRU.
> 
> It's not safe to use the nf_lru list_head for anything but the LRU
> unless you can be sure that no other references can be taken, ensuring
> that you have exclusive access to it.
> 
> In practical terms, this means that we can't queue objects to a dispose
> list until their refcounts have gone to zero. nfsd_unhash_and_queue
> currently violates this rule, as it queues nfsd_files to the dispose
> list with active references held.
> 
> This function is called both during server shutdown and when there is
> conflicting access to an inode; two situations that have little to do
> with one another. Remove nfsd_file_unhash_and_queue altogether, as its
> callers need different things, and just open code what its callers need.
> 
> Rename __nfsd_file_close_inode and rework the loop in it to put the
> reference(s) earlier and only queue the nf to the dispose list if its
> refcount went to zero.
> 
> __nfsd_file_cache_purge is called during server shutdown. At that point,
> we don't care about the refcount since we know we have exclusive access.
> Just unhash the objects, dequeue them from the LRU and then free them.
> 
> Finally, update the comments over nfsd_file_close_inode_sync,
> nfsd_file_close_inode and __nfsd_file_cache_purge to better describe
> their intended purpose.
> 
> Fixes: d2cdf429693a ("nfsd: rework refcounting in filecache")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Applied internally for testing. I'll plan to include this fix in
next week's v6.2 PR for nfsd.


> ---
> fs/nfsd/filecache.c | 111 +++++++++++++++++++++++---------------------
> 1 file changed, 58 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 1e76b0d3b83a..8bf2fcb0f580 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -461,35 +461,6 @@ nfsd_file_get(struct nfsd_file *nf)
> 	return NULL;
> }
> 
> -/**
> - * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list
> - * @nf: nfsd_file to be unhashed and queued
> - * @dispose: list to which it should be queued
> - *
> - * Attempt to unhash a nfsd_file and queue it to the given list. Each file
> - * will have a reference held on behalf of the list. That reference may come
> - * from the LRU, or we may need to take one. If we can't get a reference,
> - * ignore it altogether.
> - */
> -static bool
> -nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)
> -{
> -	trace_nfsd_file_unhash_and_queue(nf);
> -	if (nfsd_file_unhash(nf)) {
> -		/*
> -		 * If we remove it from the LRU, then just use that
> -		 * reference for the dispose list. Otherwise, we need
> -		 * to take a reference. If that fails, just ignore
> -		 * the file altogether.
> -		 */
> -		if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
> -			return false;
> -		list_add(&nf->nf_lru, dispose);
> -		return true;
> -	}
> -	return false;
> -}
> -
> /**
>  * nfsd_file_put - put the reference to a nfsd_file
>  * @nf: nfsd_file of which to put the reference
> @@ -700,15 +671,24 @@ static struct shrinker	nfsd_file_shrinker = {
> 	.seeks = 1,
> };
> 
> -/*
> - * Find all cache items across all net namespaces that match @inode, unhash
> - * them, take references and then put them on @dispose if that was successful.
> +/**
> + * nfsd_file_queue_for_close: try to close out any open nfsd_files for an inode
> + * @inode:   inode on which to close out nfsd_files
> + * @dispose: list on which to gather nfsd_files to close out
> + *
> + * An nfsd_file represents a struct file being held open on behalf of nfsd. An
> + * open file however can block other activity (such as leases), or cause
> + * undesirable behavior (e.g. spurious silly-renames when reexporting NFS).
> + *
> + * This function is intended to find open nfsd_files when this sort of
> + * conflicting access occurs and then attempt to close those files out.
>  *
> - * The nfsd_file objects on the list will be unhashed, and each will have a
> - * reference taken.
> + * Populates the dispose list with entries that have already had their
> + * refcounts go to zero. The actual free of an nfsd_file can be expensive,
> + * so we leave it up to the caller whether it wants to wait or not.
>  */
> static void
> -__nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
> +nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> {
> 	struct nfsd_file_lookup_key key = {
> 		.type	= NFSD_FILE_KEY_INODE,
> @@ -718,12 +698,30 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
> 
> 	rcu_read_lock();
> 	do {
> +		int decrement = 1;
> +
> 		nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> 				       nfsd_file_rhash_params);
> 		if (!nf)
> 			break;
> 
> -		nfsd_file_unhash_and_queue(nf, dispose);
> +		/* If we raced with someone else unhashing, ignore it */
> +		if (!nfsd_file_unhash(nf))
> +			continue;
> +
> +		/* If we can't get a reference, ignore it */
> +		if (!nfsd_file_get(nf))
> +			continue;
> +
> +		/* Extra decrement if we remove from the LRU */
> +		if (nfsd_file_lru_remove(nf))
> +			++decrement;
> +
> +		/* If refcount goes to 0, then put on the dispose list */
> +		if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> +			list_add(&nf->nf_lru, dispose);
> +			trace_nfsd_file_closing(nf);
> +		}
> 	} while (1);
> 	rcu_read_unlock();
> }
> @@ -732,22 +730,17 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
>  * nfsd_file_close_inode - attempt a delayed close of a nfsd_file
>  * @inode: inode of the file to attempt to remove
>  *
> - * Unhash and put all cache item associated with @inode. Queue any that have
> - * refcounts that go to zero to the dispose_list_delayed infrastructure for
> - * eventual cleanup.
> + * Close out any open nfsd_files that can be reaped for @inode. The
> + * actual freeing is deferred to the dispose_list_delayed infrastructure.
> + *
> + * This is used by the fsnotify callbacks and setlease notifier.
>  */
> static void
> nfsd_file_close_inode(struct inode *inode)
> {
> -	struct nfsd_file *nf, *tmp;
> 	LIST_HEAD(dispose);
> 
> -	__nfsd_file_close_inode(inode, &dispose);
> -	list_for_each_entry_safe(nf, tmp, &dispose, nf_lru) {
> -		trace_nfsd_file_closing(nf);
> -		if (!refcount_dec_and_test(&nf->nf_ref))
> -			list_del_init(&nf->nf_lru);
> -	}
> +	nfsd_file_queue_for_close(inode, &dispose);
> 	nfsd_file_dispose_list_delayed(&dispose);
> }
> 
> @@ -755,7 +748,11 @@ nfsd_file_close_inode(struct inode *inode)
>  * nfsd_file_close_inode_sync - attempt to forcibly close a nfsd_file
>  * @inode: inode of the file to attempt to remove
>  *
> - * Unhash and put, then flush and fput all cache items associated with @inode.
> + * Close out any open nfsd_files that can be reaped for @inode. The
> + * nfsd_files are closed out synchronously.
> + *
> + * This is called from nfsd_rename and nfsd_unlink to avoid silly-renames
> + * when reexporting NFS.
>  */
> void
> nfsd_file_close_inode_sync(struct inode *inode)
> @@ -765,13 +762,11 @@ nfsd_file_close_inode_sync(struct inode *inode)
> 
> 	trace_nfsd_file_close(inode);
> 
> -	__nfsd_file_close_inode(inode, &dispose);
> +	nfsd_file_queue_for_close(inode, &dispose);
> 	while (!list_empty(&dispose)) {
> 		nf = list_first_entry(&dispose, struct nfsd_file, nf_lru);
> 		list_del_init(&nf->nf_lru);
> -		trace_nfsd_file_closing(nf);
> -		if (refcount_dec_and_test(&nf->nf_ref))
> -			nfsd_file_free(nf);
> +		nfsd_file_free(nf);
> 	}
> 	flush_delayed_fput();
> }
> @@ -923,6 +918,13 @@ nfsd_file_cache_init(void)
> 	goto out;
> }
> 
> +/**
> + * __nfsd_file_cache_purge: clean out the cache for shutdown
> + * @net: net-namespace to shut down the cache (may be NULL)
> + *
> + * Walk the nfsd_file cache and close out any that match @net. If @net is NULL,
> + * then close out everything. Called when an nfsd instance is being shut down.
> + */
> static void
> __nfsd_file_cache_purge(struct net *net)
> {
> @@ -936,8 +938,11 @@ __nfsd_file_cache_purge(struct net *net)
> 
> 		nf = rhashtable_walk_next(&iter);
> 		while (!IS_ERR_OR_NULL(nf)) {
> -			if (!net || nf->nf_net == net)
> -				nfsd_file_unhash_and_queue(nf, &dispose);
> +			if (!net || nf->nf_net == net) {
> +				nfsd_file_unhash(nf);
> +				nfsd_file_lru_remove(nf);
> +				list_add(&nf->nf_lru, &dispose);
> +			}
> 			nf = rhashtable_walk_next(&iter);
> 		}
> 
> -- 
> 2.38.1
> 

--
Chuck Lever