[PATCHES][RFC][CFT] simple_recursive_removal() work

[PATCHES][RFC][CFT] simple_recursive_removal() work

[Al Viro]

[another part of tree-in-dcache pile pulled into a separate branch]

	Removing subtrees of kernel filesystems is done in quite a few
places; unfortunately, it's easy to get wrong.	A number of open-coded
attempts are out there, with varying amount of bogosities.

	simple_recursive_removal() had been introduced for doing that with
all precautions needed; it does an equivalent of rm -rf, with sufficient
locking, eviction of anything mounted on top of the subtree, etc.

	The series below converts a bunch of open-coded instances
to using that.  It's v6.16-rc1-based, lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.simple_recursive_removal
Individual patches in followups.

The first commit is shared with work.rpc_pipe (and several other branches);
the rest has not been posted yet.  If there's no objections, into -next
it goes...

Shortlog:
Al Viro(8)
      simple_recursive_removal(): saner interaction with fsnotify
      add locked_recursive_removal()
      spufs: switch to locked_recursive_removal()
      binfmt_misc: switch to locked_recursive_removal()
      pstore: switch to locked_recursive_removal()
      fuse_ctl: use simple_recursive_removal()
      kill binderfs_remove_file()
      functionfs, gadgetfs: use simple_recursive_removal()

Diffstat:
 arch/powerpc/platforms/cell/spufs/inode.c | 49 ++++++-------------------------
 drivers/android/binder.c                  |  2 +-
 drivers/android/binder_internal.h         |  2 --
 drivers/android/binderfs.c                | 15 ----------
 drivers/usb/gadget/function/f_fs.c        |  3 +-
 drivers/usb/gadget/legacy/inode.c         |  7 +----
 fs/binfmt_misc.c                          | 40 +------------------------
 fs/fuse/control.c                         | 30 ++++++++-----------
 fs/fuse/fuse_i.h                          |  6 ----
 fs/libfs.c                                | 29 +++++++++++++-----
 fs/pstore/inode.c                         |  3 +-
 include/linux/fs.h                        |  2 ++
 12 files changed, 50 insertions(+), 138 deletions(-)

[PATCH 1/8] simple_recursive_removal(): saner interaction with fsnotify

[Al Viro]

Make it match the real unlink(2)/rmdir(2) - notify *after* the
operation.  And use fsnotify_delete() instead of messing with
fsnotify_unlink()/fsnotify_rmdir().

Currently the only caller that cares is the one in debugfs, and
there the order matching the normal syscalls makes more sense;
it'll get more serious for users introduced later in the series.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/libfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 9ea0ecc325a8..42e226af6095 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -628,12 +628,9 @@ void simple_recursive_removal(struct dentry *dentry,
 			inode_lock(inode);
 			if (simple_positive(victim)) {
 				d_invalidate(victim);	// avoid lost mounts
-				if (d_is_dir(victim))
-					fsnotify_rmdir(inode, victim);
-				else
-					fsnotify_unlink(inode, victim);
 				if (callback)
 					callback(victim);
+				fsnotify_delete(inode, d_inode(victim), victim);
 				dput(victim);		// unpin it
 			}
 			if (victim == dentry) {
-- 
2.39.5

[PATCH 2/8] add locked_recursive_removal()

[Al Viro]

simple_recursive_removal() assumes that parent is not locked and
locks it when it finally gets to removing the victim itself.
Usually that's what we want, but there are places where the
parent is *already* locked and we need it to stay that way.
In those cases simple_recursive_removal() would, of course,
deadlock, so we have to play racy games with unlocking/relocking
the parent around the call or open-code the entire thing.

A better solution is to provide a variant that expects to
be called with the parent already locked by the caller.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/libfs.c         | 24 ++++++++++++++++++++----
 include/linux/fs.h |  2 ++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 42e226af6095..d0f934f0e541 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -605,8 +605,9 @@ struct dentry *find_next_child(struct dentry *parent, struct dentry *prev)
 }
 EXPORT_SYMBOL(find_next_child);
 
-void simple_recursive_removal(struct dentry *dentry,
-                              void (*callback)(struct dentry *))
+static void __simple_recursive_removal(struct dentry *dentry,
+                              void (*callback)(struct dentry *),
+			      bool locked)
 {
 	struct dentry *this = dget(dentry);
 	while (true) {
@@ -625,7 +626,8 @@ void simple_recursive_removal(struct dentry *dentry,
 			victim = this;
 			this = this->d_parent;
 			inode = this->d_inode;
-			inode_lock(inode);
+			if (!locked || victim != dentry)
+				inode_lock(inode);
 			if (simple_positive(victim)) {
 				d_invalidate(victim);	// avoid lost mounts
 				if (callback)
@@ -638,7 +640,8 @@ void simple_recursive_removal(struct dentry *dentry,
 						      inode_set_ctime_current(inode));
 				if (d_is_dir(dentry))
 					drop_nlink(inode);
-				inode_unlock(inode);
+				if (!locked)
+					inode_unlock(inode);
 				dput(dentry);
 				return;
 			}
@@ -647,8 +650,21 @@ void simple_recursive_removal(struct dentry *dentry,
 		this = child;
 	}
 }
+
+void simple_recursive_removal(struct dentry *dentry,
+                              void (*callback)(struct dentry *))
+{
+	return __simple_recursive_removal(dentry, callback, false);
+}
 EXPORT_SYMBOL(simple_recursive_removal);
 
+void locked_recursive_removal(struct dentry *dentry,
+                              void (*callback)(struct dentry *))
+{
+	return __simple_recursive_removal(dentry, callback, true);
+}
+EXPORT_SYMBOL(locked_recursive_removal);
+
 static const struct super_operations simple_super_operations = {
 	.statfs		= simple_statfs,
 };
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 96c7925a6551..4f0c6bf8d652 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3595,6 +3595,8 @@ extern int simple_rename(struct mnt_idmap *, struct inode *,
 			 unsigned int);
 extern void simple_recursive_removal(struct dentry *,
                               void (*callback)(struct dentry *));
+extern void locked_recursive_removal(struct dentry *,
+                              void (*callback)(struct dentry *));
 extern int noop_fsync(struct file *, loff_t, loff_t, int);
 extern ssize_t noop_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int simple_empty(struct dentry *);
-- 
2.39.5

[PATCH 3/8] spufs: switch to locked_recursive_removal()

[Al Viro]

... and fix an old deadlock on spufs_mkdir() failures to populate
subdirectory - spufs_rmdir() had always been taking lock on the
victim, so doing it while the victim is locked is a bad idea.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/powerpc/platforms/cell/spufs/inode.c | 49 +++++------------------
 1 file changed, 9 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index 9f9e4b871627..7ec60290abe6 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -143,42 +143,13 @@ spufs_evict_inode(struct inode *inode)
 		put_spu_gang(ei->i_gang);
 }
 
-static void spufs_prune_dir(struct dentry *dir)
-{
-	struct dentry *dentry;
-	struct hlist_node *n;
-
-	inode_lock(d_inode(dir));
-	hlist_for_each_entry_safe(dentry, n, &dir->d_children, d_sib) {
-		spin_lock(&dentry->d_lock);
-		if (simple_positive(dentry)) {
-			dget_dlock(dentry);
-			__d_drop(dentry);
-			spin_unlock(&dentry->d_lock);
-			simple_unlink(d_inode(dir), dentry);
-			/* XXX: what was dcache_lock protecting here? Other
-			 * filesystems (IB, configfs) release dcache_lock
-			 * before unlink */
-			dput(dentry);
-		} else {
-			spin_unlock(&dentry->d_lock);
-		}
-	}
-	shrink_dcache_parent(dir);
-	inode_unlock(d_inode(dir));
-}
-
 /* Caller must hold parent->i_mutex */
-static int spufs_rmdir(struct inode *parent, struct dentry *dir)
+static void spufs_rmdir(struct inode *parent, struct dentry *dir)
 {
-	/* remove all entries */
-	int res;
-	spufs_prune_dir(dir);
-	d_drop(dir);
-	res = simple_rmdir(parent, dir);
-	/* We have to give up the mm_struct */
-	spu_forget(SPUFS_I(d_inode(dir))->i_ctx);
-	return res;
+	struct spu_context *ctx = SPUFS_I(d_inode(dir))->i_ctx;
+
+	locked_recursive_removal(dir, NULL);
+	spu_forget(ctx);
 }
 
 static int spufs_fill_dir(struct dentry *dir,
@@ -222,15 +193,13 @@ static int spufs_dir_close(struct inode *inode, struct file *file)
 {
 	struct inode *parent;
 	struct dentry *dir;
-	int ret;
 
 	dir = file->f_path.dentry;
 	parent = d_inode(dir->d_parent);
 
 	inode_lock_nested(parent, I_MUTEX_PARENT);
-	ret = spufs_rmdir(parent, dir);
+	spufs_rmdir(parent, dir);
 	inode_unlock(parent);
-	WARN_ON(ret);
 
 	unuse_gang(dir->d_parent);
 	return dcache_dir_close(inode, file);
@@ -288,11 +257,11 @@ spufs_mkdir(struct inode *dir, struct dentry *dentry, unsigned int flags,
 		ret = spufs_fill_dir(dentry, spufs_dir_debug_contents,
 				mode, ctx);
 
+	inode_unlock(inode);
+
 	if (ret)
 		spufs_rmdir(dir, dentry);
 
-	inode_unlock(inode);
-
 	return ret;
 }
 
@@ -475,7 +444,7 @@ spufs_create_context(struct inode *inode, struct dentry *dentry,
 
 	ret = spufs_context_open(&path);
 	if (ret < 0)
-		WARN_ON(spufs_rmdir(inode, dentry));
+		spufs_rmdir(inode, dentry);
 
 out_aff_unlock:
 	if (affinity)
-- 
2.39.5

[PATCH 4/8] binfmt_misc: switch to locked_recursive_removal()

[Al Viro]

... fixing a mount leak, strictly speaking.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/binfmt_misc.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 432fbf4fc334..760437a91648 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -674,44 +674,6 @@ static void bm_evict_inode(struct inode *inode)
 	}
 }
 
-/**
- * unlink_binfmt_dentry - remove the dentry for the binary type handler
- * @dentry: dentry associated with the binary type handler
- *
- * Do the actual filesystem work to remove a dentry for a registered binary
- * type handler. Since binfmt_misc only allows simple files to be created
- * directly under the root dentry of the filesystem we ensure that we are
- * indeed passed a dentry directly beneath the root dentry, that the inode
- * associated with the root dentry is locked, and that it is a regular file we
- * are asked to remove.
- */
-static void unlink_binfmt_dentry(struct dentry *dentry)
-{
-	struct dentry *parent = dentry->d_parent;
-	struct inode *inode, *parent_inode;
-
-	/* All entries are immediate descendants of the root dentry. */
-	if (WARN_ON_ONCE(dentry->d_sb->s_root != parent))
-		return;
-
-	/* We only expect to be called on regular files. */
-	inode = d_inode(dentry);
-	if (WARN_ON_ONCE(!S_ISREG(inode->i_mode)))
-		return;
-
-	/* The parent inode must be locked. */
-	parent_inode = d_inode(parent);
-	if (WARN_ON_ONCE(!inode_is_locked(parent_inode)))
-		return;
-
-	if (simple_positive(dentry)) {
-		dget(dentry);
-		simple_unlink(parent_inode, dentry);
-		d_delete(dentry);
-		dput(dentry);
-	}
-}
-
 /**
  * remove_binfmt_handler - remove a binary type handler
  * @misc: handle to binfmt_misc instance
@@ -729,7 +691,7 @@ static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e)
 	write_lock(&misc->entries_lock);
 	list_del_init(&e->list);
 	write_unlock(&misc->entries_lock);
-	unlink_binfmt_dentry(e->dentry);
+	locked_recursive_removal(e->dentry, NULL);
 }
 
 /* /<entry> */
-- 
2.39.5

[PATCH 5/8] pstore: switch to locked_recursive_removal()

[Al Viro]

rather than playing with manual d_invalidate()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/pstore/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index bb3b769edc71..93307e677f91 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -318,8 +318,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
 		list_for_each_entry_safe(pos, tmp, &records_list, list) {
 			if (pos->record->psi == psi) {
 				list_del_init(&pos->list);
-				d_invalidate(pos->dentry);
-				simple_unlink(d_inode(root), pos->dentry);
+				locked_recursive_removal(pos->dentry, NULL);
 				pos->dentry = NULL;
 			}
 		}
-- 
2.39.5

[PATCH 6/8] fuse_ctl: use simple_recursive_removal()

[Al Viro]

easier that way - no need to keep that array of dentry references, etc.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/fuse/control.c | 30 +++++++++++++-----------------
 fs/fuse/fuse_i.h  |  6 ------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 2a730d88cc3b..bb407705603c 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/fs_context.h>
+#include <linux/namei.h>
 
 #define FUSE_CTL_SUPER_MAGIC 0x65735543
 
@@ -212,7 +213,6 @@ static struct dentry *fuse_ctl_add_dentry(struct dentry *parent,
 	struct dentry *dentry;
 	struct inode *inode;
 
-	BUG_ON(fc->ctl_ndents >= FUSE_CTL_NUM_DENTRIES);
 	dentry = d_alloc_name(parent, name);
 	if (!dentry)
 		return NULL;
@@ -236,8 +236,6 @@ static struct dentry *fuse_ctl_add_dentry(struct dentry *parent,
 	inode->i_private = fc;
 	d_add(dentry, inode);
 
-	fc->ctl_dentry[fc->ctl_ndents++] = dentry;
-
 	return dentry;
 }
 
@@ -280,27 +278,29 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
 	return -ENOMEM;
 }
 
+static void remove_one(struct dentry *dentry)
+{
+	d_inode(dentry)->i_private = NULL;
+}
+
 /*
  * Remove a connection from the control filesystem (if it exists).
  * Caller must hold fuse_mutex
  */
 void fuse_ctl_remove_conn(struct fuse_conn *fc)
 {
-	int i;
+	struct dentry *dentry;
+	char name[32];
 
 	if (!fuse_control_sb || fc->no_control)
 		return;
 
-	for (i = fc->ctl_ndents - 1; i >= 0; i--) {
-		struct dentry *dentry = fc->ctl_dentry[i];
-		d_inode(dentry)->i_private = NULL;
-		if (!i) {
-			/* Get rid of submounts: */
-			d_invalidate(dentry);
-		}
-		dput(dentry);
+	sprintf(name, "%u", fc->dev);
+	dentry = lookup_noperm_positive_unlocked(&QSTR(name), fuse_control_sb->s_root);
+	if (!IS_ERR(dentry)) {
+		simple_recursive_removal(dentry, remove_one);
+		dput(dentry);	// paired with lookup_noperm_positive_unlocked()
 	}
-	drop_nlink(d_inode(fuse_control_sb->s_root));
 }
 
 static int fuse_ctl_fill_super(struct super_block *sb, struct fs_context *fsc)
@@ -346,12 +346,8 @@ static int fuse_ctl_init_fs_context(struct fs_context *fsc)
 
 static void fuse_ctl_kill_sb(struct super_block *sb)
 {
-	struct fuse_conn *fc;
-
 	mutex_lock(&fuse_mutex);
 	fuse_control_sb = NULL;
-	list_for_each_entry(fc, &fuse_conn_list, entry)
-		fc->ctl_ndents = 0;
 	mutex_unlock(&fuse_mutex);
 
 	kill_litter_super(sb);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index b54f4f57789f..30206605e114 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -913,12 +913,6 @@ struct fuse_conn {
 	/** Device ID from the root super block */
 	dev_t dev;
 
-	/** Dentries in the control filesystem */
-	struct dentry *ctl_dentry[FUSE_CTL_NUM_DENTRIES];
-
-	/** number of dentries used in the above array */
-	int ctl_ndents;
-
 	/** Key for lock owner ID scrambling */
 	u32 scramble_key[4];
 
-- 
2.39.5

[PATCH 7/8] kill binderfs_remove_file()

[Al Viro]

don't try to open-code simple_recursive_removal(), especially when
you miss things like d_invalidate()...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/android/binder.c          |  2 +-
 drivers/android/binder_internal.h |  2 --
 drivers/android/binderfs.c        | 15 ---------------
 3 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c463ca4a8fff..ffecf212c2d0 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6128,7 +6128,7 @@ static int binder_release(struct inode *nodp, struct file *filp)
 	debugfs_remove(proc->debugfs_entry);
 
 	if (proc->binderfs_entry) {
-		binderfs_remove_file(proc->binderfs_entry);
+		simple_recursive_removal(proc->binderfs_entry, NULL);
 		proc->binderfs_entry = NULL;
 	}
 
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 1ba5caf1d88d..a5fd23dcafcd 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -81,7 +81,6 @@ extern bool is_binderfs_device(const struct inode *inode);
 extern struct dentry *binderfs_create_file(struct dentry *dir, const char *name,
 					   const struct file_operations *fops,
 					   void *data);
-extern void binderfs_remove_file(struct dentry *dentry);
 #else
 static inline bool is_binderfs_device(const struct inode *inode)
 {
@@ -94,7 +93,6 @@ static inline struct dentry *binderfs_create_file(struct dentry *dir,
 {
 	return NULL;
 }
-static inline void binderfs_remove_file(struct dentry *dentry) {}
 #endif
 
 #ifdef CONFIG_ANDROID_BINDERFS
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 024275dbfdd8..acc946bb1457 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -500,21 +500,6 @@ static struct dentry *binderfs_create_dentry(struct dentry *parent,
 	return dentry;
 }
 
-void binderfs_remove_file(struct dentry *dentry)
-{
-	struct inode *parent_inode;
-
-	parent_inode = d_inode(dentry->d_parent);
-	inode_lock(parent_inode);
-	if (simple_positive(dentry)) {
-		dget(dentry);
-		simple_unlink(parent_inode, dentry);
-		d_delete(dentry);
-		dput(dentry);
-	}
-	inode_unlock(parent_inode);
-}
-
 struct dentry *binderfs_create_file(struct dentry *parent, const char *name,
 				    const struct file_operations *fops,
 				    void *data)
-- 
2.39.5

[PATCH 8/8] functionfs, gadgetfs: use simple_recursive_removal()

[Al Viro]

usual mount leaks if something had been bound on top of disappearing
files there.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 drivers/usb/gadget/legacy/inode.c  | 7 +------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2dea9e42a0f8..ea5f0af1e8d2 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2369,8 +2369,7 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 	for (; count; --count, ++epfile) {
 		BUG_ON(mutex_is_locked(&epfile->mutex));
 		if (epfile->dentry) {
-			d_delete(epfile->dentry);
-			dput(epfile->dentry);
+			simple_recursive_removal(epfile->dentry, NULL);
 			epfile->dentry = NULL;
 		}
 	}
diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index fcce84a726f2..b51e132b0cd2 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -1561,7 +1561,6 @@ static void destroy_ep_files (struct dev_data *dev)
 	spin_lock_irq (&dev->lock);
 	while (!list_empty(&dev->epfiles)) {
 		struct ep_data	*ep;
-		struct inode	*parent;
 		struct dentry	*dentry;
 
 		/* break link to FS */
@@ -1571,7 +1570,6 @@ static void destroy_ep_files (struct dev_data *dev)
 
 		dentry = ep->dentry;
 		ep->dentry = NULL;
-		parent = d_inode(dentry->d_parent);
 
 		/* break link to controller */
 		mutex_lock(&ep->lock);
@@ -1586,10 +1584,7 @@ static void destroy_ep_files (struct dev_data *dev)
 		put_ep (ep);
 
 		/* break link to dcache */
-		inode_lock(parent);
-		d_delete (dentry);
-		dput (dentry);
-		inode_unlock(parent);
+		simple_recursive_removal(dentry, NULL);
 
 		spin_lock_irq (&dev->lock);
 	}
-- 
2.39.5

Re: [PATCH 1/8] simple_recursive_removal(): saner interaction with fsnotify

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:23AM +0100, Al Viro wrote:
> Make it match the real unlink(2)/rmdir(2) - notify *after* the
> operation.  And use fsnotify_delete() instead of messing with
> fsnotify_unlink()/fsnotify_rmdir().
> 
> Currently the only caller that cares is the one in debugfs, and
> there the order matching the normal syscalls makes more sense;
> it'll get more serious for users introduced later in the series.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 2/8] add locked_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:24AM +0100, Al Viro wrote:
> simple_recursive_removal() assumes that parent is not locked and
> locks it when it finally gets to removing the victim itself.
> Usually that's what we want, but there are places where the
> parent is *already* locked and we need it to stay that way.
> In those cases simple_recursive_removal() would, of course,
> deadlock, so we have to play racy games with unlocking/relocking
> the parent around the call or open-code the entire thing.
> 
> A better solution is to provide a variant that expects to
> be called with the parent already locked by the caller.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 3/8] spufs: switch to locked_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:25AM +0100, Al Viro wrote:
> ... and fix an old deadlock on spufs_mkdir() failures to populate
> subdirectory - spufs_rmdir() had always been taking lock on the
> victim, so doing it while the victim is locked is a bad idea.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Fwiw, I think simple_recursive_removal_locked() might be better.
It's longer and arguably uglier but it clearer communicates that its the
same helper as simple_recursive_removal() just with the assumption that
the caller already holds the lock.

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 4/8] binfmt_misc: switch to locked_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:26AM +0100, Al Viro wrote:
> ... fixing a mount leak, strictly speaking.

Imho all those entries in binfmt_misc should probably just all be
switched to call dont_mount(). Zero reason to support this.

> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 5/8] pstore: switch to locked_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:27AM +0100, Al Viro wrote:
> rather than playing with manual d_invalidate()
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 6/8] fuse_ctl: use simple_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:28AM +0100, Al Viro wrote:
> easier that way - no need to keep that array of dentry references, etc.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 7/8] kill binderfs_remove_file()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:29AM +0100, Al Viro wrote:
> don't try to open-code simple_recursive_removal(), especially when
> you miss things like d_invalidate()...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 8/8] functionfs, gadgetfs: use simple_recursive_removal()

[Christian Brauner]

On Sat, Jun 14, 2025 at 07:02:30AM +0100, Al Viro wrote:
> usual mount leaks if something had been bound on top of disappearing
> files there.
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 3/8] spufs: switch to locked_recursive_removal()

[Al Viro]

On Mon, Jun 16, 2025 at 04:40:14PM +0200, Christian Brauner wrote:
> On Sat, Jun 14, 2025 at 07:02:25AM +0100, Al Viro wrote:
> > ... and fix an old deadlock on spufs_mkdir() failures to populate
> > subdirectory - spufs_rmdir() had always been taking lock on the
> > victim, so doing it while the victim is locked is a bad idea.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> 
> Fwiw, I think simple_recursive_removal_locked() might be better.
> It's longer and arguably uglier but it clearer communicates that its the
> same helper as simple_recursive_removal() just with the assumption that
> the caller already holds the lock.

Not sure...  TBH, I'm somewhat tempted to rename simple_recursive_removal()
to simple_remove()...

Re: [PATCH 4/8] binfmt_misc: switch to locked_recursive_removal()

[Al Viro]

On Mon, Jun 16, 2025 at 04:43:06PM +0200, Christian Brauner wrote:
> On Sat, Jun 14, 2025 at 07:02:26AM +0100, Al Viro wrote:
> > ... fixing a mount leak, strictly speaking.
> 
> Imho all those entries in binfmt_misc should probably just all be
> switched to call dont_mount(). Zero reason to support this.

Might as well, but that's a separate story.  IMO this commit
makes sense on its own...

Re: [PATCH 3/8] spufs: switch to locked_recursive_removal()

[Christian Brauner]

On Mon, Jun 16, 2025 at 08:14:58PM +0100, Al Viro wrote:
> On Mon, Jun 16, 2025 at 04:40:14PM +0200, Christian Brauner wrote:
> > On Sat, Jun 14, 2025 at 07:02:25AM +0100, Al Viro wrote:
> > > ... and fix an old deadlock on spufs_mkdir() failures to populate
> > > subdirectory - spufs_rmdir() had always been taking lock on the
> > > victim, so doing it while the victim is locked is a bad idea.
> > > 
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > ---
> > 
> > Fwiw, I think simple_recursive_removal_locked() might be better.
> > It's longer and arguably uglier but it clearer communicates that its the
> > same helper as simple_recursive_removal() just with the assumption that
> > the caller already holds the lock.
> 
> Not sure...  TBH, I'm somewhat tempted to rename simple_recursive_removal()
> to simple_remove()...

Sounds good.