[PATCH 3/3] nfs: take rmdir_sem for a silly-delete only when its REMOVE is sent

[Jonathan Flynn]

nfs_call_unlink() takes the directory's rmdir_sem for read before it
looks the silly name up, and keeps it until the REMOVE it sends is done
or, when it hands the silly-delete to another cached entry for the name
instead, until it has dropped that alias.  Several things inside that
hold can drop the last reference to a dentry marked DCACHE_NFSFS_RENAMED:
the dput() of an alias that has just been handed the silly-delete; the
dput()s in d_alloc_parallel() of an entry it found but will not return,
after a failed ->d_seq recheck or after waiting for an in-lookup entry
that changed; and ->unlink_setup(), where NFSv4 returns the file's
delegation, putting the open contexts it reclaims and waiting for
writeback whose completion can put others.  Such a dentry is killed at
once: nfs_dentry_delete() asks for that, and nfs_dentry_iput() passes
it to nfs_complete_unlink(), which calls nfs_call_unlink() again, in
this thread or in the one completing the writeback.  That call takes
rmdir_sem for read while the first hold is still taken.  If an rmdir()
of the directory is waiting for rmdir_sem as a writer by then, the
second reader waits behind it, the writer waits for the first reader,
and none of them returns.

Take rmdir_sem only when the REMOVE is about to be submitted, after
->unlink_setup(), and hold it until the REMOVE is done, as before.  The
hand-off sends no REMOVE and needs no hold; the alias sends it later,
through a call of its own.  An rmdir() that runs before the hold is
taken finds the silly-renamed file still there, as it would have a
moment earlier.

The ->unlink_setup() route does not depend on NeilBrown's "nfs: fix
nfs_call_unlink()": it needs a second silly-renamed name of the same
inode in the same directory, such as a hard link removed while open,
and a delegation still held when the silly-delete is sent.  That patch
makes the other two reachable, along with the hand-off itself.  With it
applied, the re-entry through the alias has been reproduced: an unused
second entry for a silly name received the delete when the file was
closed, and nfs_call_unlink() ran again with rmdir_sem held for read.
The deadlock needs a waiting rmdir() and has not been reproduced.
Sashiko's review of that patch reported the alias case; an AI coding
assistant (Claude Code) found the other two while reviewing this fix.

Fixes: 884be175351e ("nfs: per-name sillyunlink exclusion")
Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
---
 fs/nfs/unlink.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 491ebd8582..679ef22637 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -113,6 +113,19 @@ static void nfs_do_call_unlink(struct inode *inode, struct nfs_unlinkdata *data)
 
 	NFS_PROTO(dir)->unlink_setup(&msg, data->dentry, inode);
 
+	/*
+	 * Hold off an rmdir() of the directory from here until the REMOVE
+	 * is done (nfs_async_unlink_release()), and not before.  Until the
+	 * REMOVE is submitted, the lookup in nfs_call_unlink(), its dput()
+	 * of an alias, and ->unlink_setup() (where NFSv4 returns the
+	 * file's delegation, putting open contexts and waiting for
+	 * writeback) can each drop the last reference to a dentry carrying
+	 * a silly-delete.  Its kill calls nfs_call_unlink() again, in this
+	 * thread or in one this thread waits for, and if rmdir_sem were
+	 * held here by then, that call's read hold would wait behind an
+	 * rmdir() waiting for this one.
+	 */
+	down_read_non_owner(&NFS_I(dir)->rmdir_sem);
 	task_setup_data.rpc_client = NFS_CLIENT(dir);
 	task = rpc_run_task(&task_setup_data);
 	if (!IS_ERR(task))
@@ -121,18 +134,14 @@ static void nfs_do_call_unlink(struct inode *inode, struct nfs_unlinkdata *data)
 
 static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nfs_unlinkdata *data)
 {
-	struct inode *dir = d_inode(dentry->d_parent);
 	struct dentry *alias;
 
-	down_read_non_owner(&NFS_I(dir)->rmdir_sem);
 	data->args.name.hash = full_name_hash(dentry->d_parent,
 					      data->args.name.name,
 					      data->args.name.len);
 	alias = d_alloc_parallel(dentry->d_parent, &data->args.name);
-	if (IS_ERR(alias)) {
-		up_read_non_owner(&NFS_I(dir)->rmdir_sem);
+	if (IS_ERR(alias))
 		return 0;
-	}
 	if (!d_in_lookup(alias)) {
 		int ret;
 		void *devname_garbage = NULL;
@@ -170,7 +179,6 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf
 			ret = 0;
 		spin_unlock(&alias->d_lock);
 		dput(alias);
-		up_read_non_owner(&NFS_I(dir)->rmdir_sem);
 		/*
 		 * If we'd displaced old cached devname, free it.  At that
 		 * point dentry is definitely not a root, so we won't need
-- 
2.43.0