[PATCH 00/13] Further centralising of directory locking for name ops.

[PATCH 00/13] Further centralising of directory locking for name ops.

[NeilBrown]

I am working towards changing the locking rules for name-operations: locking
the name rather than the whole directory.

The current part of this process is centralising all the locking so that
it can be changed in one place.

Recently "start_creating", "start_removing", "start_renaming" and related
interaces were added which combine the locking and the lookup.  At that time
many callers were changed to use the new interfaces.  However there are still
an assortment of places out side of fs/namei.c where the directory is locked
explictly, whether with inode_lock() or lock_rename() or similar.  These were
missed in the first pass for an assortment of uninteresting reasons.

This series addresses the remaining places where explicit locking is
used, and changes them to use the new interfaces, or otherwise removes
the explicit locking.

The biggest changes are in overlayfs.  The other changes are quite
simple, though maybe the cachefiles changes is the least simple of those.

I'm running the --overlay tests in xfstests and nothing has popped yet.
I'll continue with this and run some NFS tests too.

Thanks for your review of these patches!

NeilBrown


 [PATCH 01/13] fs/proc: Don't lock root inode when creating "self" and
 [PATCH 02/13] VFS: move the start_dirop() kerndoc comment to before
 [PATCH 03/13] libfs: change simple_done_creating() to use
 [PATCH 04/13] Apparmor: Use simple_start_creating() /
 [PATCH 05/13] selinux: Use simple_start_creating() /
 [PATCH 06/13] nfsd: switch purge_old() to use start_removing_noperm()
 [PATCH 07/13] VFS: make lookup_one_qstr_excl() static.
 [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
 [PATCH 09/13] cachefiles: change cachefiles_bury_object to use
 [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when
 [PATCH 11/13] ovl: use is_subdir() for testing if one thing is a
 [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
 [PATCH 13/13] VFS: unexport lock_rename(), lock_rename_child(),

[PATCH 01/13] fs/proc: Don't lock root inode when creating "self" and "thread-self"

[NeilBrown]

From: NeilBrown <neil@brown.name>

proc_setup_self() and proc_setup_thread_self() are only called from
proc_fill_super() which is before the filesystem is "live".  So there is
no need to lock the root directory when adding "self" and "thread-self".
This is clear from simple_fill_super() which provides similar
functionality for other filesystems and does not lock anything.

The locking is not harmful, except that it may be confusing to a reader.
As part of a an effort to centralise all locking for directories for
name-based operations (prior to changing some locking rules), it is
simplest to remove the locking here.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/proc/self.c        | 3 ---
 fs/proc/thread_self.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/fs/proc/self.c b/fs/proc/self.c
index 62d2c0cfe35c..56adf1c68f7a 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -35,11 +35,9 @@ unsigned self_inum __ro_after_init;
 
 int proc_setup_self(struct super_block *s)
 {
-	struct inode *root_inode = d_inode(s->s_root);
 	struct dentry *self;
 	int ret = -ENOMEM;
 
-	inode_lock(root_inode);
 	self = d_alloc_name(s->s_root, "self");
 	if (self) {
 		struct inode *inode = new_inode(s);
@@ -55,7 +53,6 @@ int proc_setup_self(struct super_block *s)
 		}
 		dput(self);
 	}
-	inode_unlock(root_inode);
 
 	if (ret)
 		pr_err("proc_fill_super: can't allocate /proc/self\n");
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index d6113dbe58e0..61ac62c3fd9f 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -35,11 +35,9 @@ unsigned thread_self_inum __ro_after_init;
 
 int proc_setup_thread_self(struct super_block *s)
 {
-	struct inode *root_inode = d_inode(s->s_root);
 	struct dentry *thread_self;
 	int ret = -ENOMEM;
 
-	inode_lock(root_inode);
 	thread_self = d_alloc_name(s->s_root, "thread-self");
 	if (thread_self) {
 		struct inode *inode = new_inode(s);
@@ -55,7 +53,6 @@ int proc_setup_thread_self(struct super_block *s)
 		}
 		dput(thread_self);
 	}
-	inode_unlock(root_inode);
 
 	if (ret)
 		pr_err("proc_fill_super: can't allocate /proc/thread-self\n");
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 02/13] VFS: move the start_dirop() kerndoc comment to before start_dirop()

[NeilBrown]

From: NeilBrown <neil@brown.name>

This kerneldoc comment was always meant for start_dirop(), not for
__start_dirop() which is a static function and doesn't need
documentation.

It was in the wrong place and was then incorrectly renamed (instead of
moved) and useless "documentation" was added for "@state" was provided.

This patch reverts the name, removes the mention of @state, and moves
the comment to where it belongs.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index b28ecb699f32..40af78ddfb1b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2841,20 +2841,6 @@ static int filename_parentat(int dfd, struct filename *name,
 	return __filename_parentat(dfd, name, flags, parent, last, type, NULL);
 }
 
-/**
- * __start_dirop - begin a create or remove dirop, performing locking and lookup
- * @parent:       the dentry of the parent in which the operation will occur
- * @name:         a qstr holding the name within that parent
- * @lookup_flags: intent and other lookup flags.
- * @state:        task state bitmask
- *
- * The lookup is performed and necessary locks are taken so that, on success,
- * the returned dentry can be operated on safely.
- * The qstr must already have the hash value calculated.
- *
- * Returns: a locked dentry, or an error.
- *
- */
 static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
 				    unsigned int lookup_flags,
 				    unsigned int state)
@@ -2876,6 +2862,19 @@ static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
 	return dentry;
 }
 
+/**
+ * start_dirop - begin a create or remove dirop, performing locking and lookup
+ * @parent:       the dentry of the parent in which the operation will occur
+ * @name:         a qstr holding the name within that parent
+ * @lookup_flags: intent and other lookup flags.
+ *
+ * The lookup is performed and necessary locks are taken so that, on success,
+ * the returned dentry can be operated on safely.
+ * The qstr must already have the hash value calculated.
+ *
+ * Returns: a locked dentry, or an error.
+ *
+ */
 struct dentry *start_dirop(struct dentry *parent, struct qstr *name,
 			   unsigned int lookup_flags)
 {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 03/13] libfs: change simple_done_creating() to use end_creating()

[NeilBrown]

From: NeilBrown <neil@brown.name>

simple_done_creating() and end_creating() are identical.
So change the former to use the latter.  This further centralises
unlocking of directories.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/libfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index f1860dff86f2..db18b53fc189 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
 /* parent must have been held exclusive since simple_start_creating() */
 void simple_done_creating(struct dentry *child)
 {
-	inode_unlock(child->d_parent->d_inode);
-	dput(child);
+	end_creating(child);
 }
 EXPORT_SYMBOL(simple_done_creating);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Instead of explicitly locking the parent and performing a look up in
apparmor, use simple_start_creating(), and then simple_done_creating()
to unlock and drop the dentry.

This removes the need to check for an existing entry (as
simple_start_creating() acts like an exclusive create and can return
-EEXIST), simplifies error paths, and keeps dir locking code
centralised.

Signed-off-by: NeilBrown <neil@brown.name>
---
 security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 907bd2667e28..7f78c36e6e50 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
 
 	dir = d_inode(parent);
 
-	inode_lock(dir);
-	dentry = lookup_noperm(&QSTR(name), parent);
+	dentry = simple_start_creating(parent, name);
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
-		goto fail_lock;
-	}
-
-	if (d_really_is_positive(dentry)) {
-		error = -EEXIST;
-		goto fail_dentry;
+		goto fail;
 	}
 
 	error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
+	simple_done_creating(dentry);
 	if (error)
-		goto fail_dentry;
-	inode_unlock(dir);
-
-	return dentry;
-
-fail_dentry:
-	dput(dentry);
-
-fail_lock:
-	inode_unlock(dir);
+		goto fail;
+	return 0;
+fail:
 	simple_release_fs(&aafs_mnt, &aafs_count);
-
 	return ERR_PTR(error);
 }
 
@@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
 	if (error)
 		return error;
 
-	inode_lock(d_inode(parent));
-	dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
+	dentry = simple_start_creating(parent, NULL_FILE_NAME);
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
 		goto out;
@@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
 	inode = new_inode(parent->d_inode->i_sb);
 	if (!inode) {
 		error = -ENOMEM;
-		goto out1;
+		goto out;
 	}
 
 	inode->i_ino = get_next_ino();
@@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
 	aa_null.dentry = dget(dentry);
 	aa_null.mnt = mntget(mount);
 
-	error = 0;
-
-out1:
-	dput(dentry);
 out:
-	inode_unlock(d_inode(parent));
+	simple_done_creating(dentry);
 	simple_release_fs(&mount, &count);
 	return error;
 }
 
-
-
 static const char *policy_get_link(struct dentry *dentry,
 				   struct inode *inode,
 				   struct delayed_call *done)
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Instead of explicitly locking the parent and performing a lookup in
selinux, use simple_start_creating(), and then use
simple_done_creating() to unlock.

This extends the region that the directory is locked for, and also
performs a lookup.
The lock extension is of no real consequence.
The lookup uses simple_lookup() and so always succeeds.  Thus when
d_make_persistent() is called the dentry will already be hashed.
d_make_persistent() handles this case.

Signed-off-by: NeilBrown <neil@brown.name>
---
 security/selinux/selinuxfs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 896acad1f5f7..97e02cd5a9dc 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1930,15 +1930,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
 static struct dentry *sel_make_swapover_dir(struct super_block *sb,
 						unsigned long *ino)
 {
-	struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
+	struct dentry *dentry;
 	struct inode *inode;
 
-	if (!dentry)
+	inode = sel_make_inode(sb, S_IFDIR);
+	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
-	inode = sel_make_inode(sb, S_IFDIR);
-	if (!inode) {
-		dput(dentry);
+	dentry = simple_start_creating(sb->s_root, ".swapover");
+	if (!dentry) {
+		iput(inode);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -1946,11 +1947,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
 	inode->i_ino = ++(*ino);
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
-	inode_lock(sb->s_root->d_inode);
 	d_make_persistent(dentry, inode);
 	inc_nlink(sb->s_root->d_inode);
-	inode_unlock(sb->s_root->d_inode);
-	dput(dentry);
+	simple_done_creating(dentry);
 	return dentry;	// borrowed
 }
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 06/13] nfsd: switch purge_old() to use start_removing_noperm()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Rather than explicit locking, use the start_removing_noperm() and
end_removing() wrappers.
This was not done with other start_removing changes due to conflicting
in-flight patches.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4recover.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 441dfbfe2d2b..52fbe723a3c8 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -351,16 +351,14 @@ purge_old(struct dentry *parent, char *cname, struct nfsd_net *nn)
 	if (nfs4_has_reclaimed_state(name, nn))
 		goto out_free;
 
-	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
-	child = lookup_one(&nop_mnt_idmap, &QSTR(cname), parent);
+	child = start_removing_noperm(parent, &QSTR(cname));
 	if (!IS_ERR(child)) {
 		status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child, NULL);
 		if (status)
 			printk("failed to remove client recovery directory %pd\n",
 			       child);
-		dput(child);
 	}
-	inode_unlock(d_inode(parent));
+	end_removing(child);
 
 out_free:
 	kfree(name.data);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 07/13] VFS: make lookup_one_qstr_excl() static.

[NeilBrown]

From: NeilBrown <neil@brown.name>

lookup_one_qstr_excl() is no longer used outside of namei.c, so
make it static.

Signed-off-by: NeilBrown <neil@brown.name>
---
 Documentation/filesystems/porting.rst | 7 +++++++
 fs/namei.c                            | 5 ++---
 include/linux/namei.h                 | 3 ---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index ed3ac56e3c76..ed86c95d9d01 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1340,3 +1340,10 @@ The ->setlease() file_operation must now be explicitly set in order to provide
 support for leases. When set to NULL, the kernel will now return -EINVAL to
 attempts to set a lease. Filesystems that wish to use the kernel-internal lease
 implementation should set it to generic_setlease().
+
+---
+
+**mandatory**
+
+lookup_one_qstr_excl() is no longer exported - use start_creating() or
+similar.
diff --git a/fs/namei.c b/fs/namei.c
index 40af78ddfb1b..307b4d0866b8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1730,8 +1730,8 @@ static struct dentry *lookup_dcache(const struct qstr *name,
  * Will return -ENOENT if name isn't found and LOOKUP_CREATE wasn't passed.
  * Will return -EEXIST if name is found and LOOKUP_EXCL was passed.
  */
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
-				    struct dentry *base, unsigned int flags)
+static struct dentry *lookup_one_qstr_excl(const struct qstr *name,
+					   struct dentry *base, unsigned int flags)
 {
 	struct dentry *dentry;
 	struct dentry *old;
@@ -1768,7 +1768,6 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
 	}
 	return dentry;
 }
-EXPORT_SYMBOL(lookup_one_qstr_excl);
 
 /**
  * lookup_fast - do fast lockless (but racy) lookup of a dentry
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 58600cf234bc..c7a7288cdd25 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -54,9 +54,6 @@ extern int path_pts(struct path *path);
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
-				    struct dentry *base,
-				    unsigned int flags);
 extern int kern_path(const char *, unsigned, struct path *);
 struct dentry *kern_path_parent(const char *name, struct path *parent);
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 08/13] ovl: Simplify ovl_lookup_real_one()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The primary purpose of this patch is to remove the locking from
ovl_lookup_real_one() as part of centralising all locking of directories
for name operations.

The locking here isn't needed.  By performing consistency tests after
the lookup we can be sure that the result of the lookup was valid at
least for a moment, which is all the original code promised.

lookup_noperm_unlocked() is used for the lookup and it will take the
lock if needed only where it is needed.

Also:
 - don't take a reference to real->d_parent.  The parent is
   only use for a pointer comparison, and no reference is needed for
   that.
 - Several "if" statements have a "goto" followed by "else" - the
   else isn't needed: the following statement can directly follow
   the "if" as a new statement
 - Use a consistent pattern of setting "err" before performing a test
   and possibly going to "fail".
 - remove the "out" label (now that we don't need to dput(parent) or
   unlock) and simply return from fail:.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 83f80fdb1567..dcd28ffc4705 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
 					  struct dentry *real,
 					  const struct ovl_layer *layer)
 {
-	struct inode *dir = d_inode(connected);
-	struct dentry *this, *parent = NULL;
+	struct dentry *this;
 	struct name_snapshot name;
 	int err;
 
 	/*
-	 * Lookup child overlay dentry by real name. The dir mutex protects us
-	 * from racing with overlay rename. If the overlay dentry that is above
-	 * real has already been moved to a parent that is not under the
-	 * connected overlay dir, we return -ECHILD and restart the lookup of
-	 * connected real path from the top.
+	 * @connected is a directory in the overlay and @real is an object
+	 * on @layer which is expected to be a child of @connected.
+	 * The goal is to return a dentry from the overlay which corresponds
+	 * to @real.  This is done by looking up the name from @real in
+	 * @connected and checking that the result meets expectations.
+	 *
+	 * Return %-ECHILD if the parent of @real no-longer corresponds to
+	 * @connected, and %-ESTALE if the dentry found by lookup doesn't
+	 * correspond to @real.
 	 */
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	err = -ECHILD;
-	parent = dget_parent(real);
-	if (ovl_dentry_real_at(connected, layer->idx) != parent)
-		goto fail;
 
-	/*
-	 * We also need to take a snapshot of real dentry name to protect us
-	 * from racing with underlying layer rename. In this case, we don't
-	 * care about returning ESTALE, only from dereferencing a free name
-	 * pointer because we hold no lock on the real dentry.
-	 */
 	take_dentry_name_snapshot(&name, real);
-	/*
-	 * No idmap handling here: it's an internal lookup.
-	 */
-	this = lookup_noperm(&name.name, connected);
+	this = lookup_noperm_unlocked(&name.name, connected);
 	release_dentry_name_snapshot(&name);
+
+	err = -ECHILD;
+	if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
+		goto fail;
+
 	err = PTR_ERR(this);
-	if (IS_ERR(this)) {
+	if (IS_ERR(this))
 		goto fail;
-	} else if (!this || !this->d_inode) {
-		dput(this);
-		err = -ENOENT;
+
+	err = -ENOENT;
+	if (!this || !this->d_inode)
 		goto fail;
-	} else if (ovl_dentry_real_at(this, layer->idx) != real) {
-		dput(this);
-		err = -ESTALE;
+
+	err = -ESTALE;
+	if (ovl_dentry_real_at(this, layer->idx) != real)
 		goto fail;
-	}
 
-out:
-	dput(parent);
-	inode_unlock(dir);
 	return this;
 
 fail:
 	pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
 			    real, layer->idx, connected, err);
-	this = ERR_PTR(err);
-	goto out;
+	if (!IS_ERR(this))
+		dput(this);
+	return ERR_PTR(err);
 }
 
 static struct dentry *ovl_lookup_real(struct super_block *sb,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 09/13] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Rather then using lock_rename() and lookup_one() etc we can use
the new start_renaming_dentry().  This is part of centralising dir
locking and lookup so that locking rules can be changed.

Some error check are removed as not necessary.  Checks for rep being a
non-dir or IS_DEADDIR and the check that ->graveyard is still a
directory only provide slightly more informative errors and have been
dropped.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
 1 file changed, 14 insertions(+), 62 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index e5ec90dccc27..3af42ec78411 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 			   struct dentry *rep,
 			   enum fscache_why_object_killed why)
 {
-	struct dentry *grave, *trap;
+	struct dentry *grave;
+	struct renamedata rd = {};
 	struct path path, path_to_graveyard;
 	char nbuffer[8 + 8 + 1];
 	int ret;
@@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 		(uint32_t) ktime_get_real_seconds(),
 		(uint32_t) atomic_inc_return(&cache->gravecounter));
 
-	/* do the multiway lock magic */
-	trap = lock_rename(cache->graveyard, dir);
-	if (IS_ERR(trap))
-		return PTR_ERR(trap);
-
-	/* do some checks before getting the grave dentry */
-	if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
-		/* the entry was probably culled when we dropped the parent dir
-		 * lock */
-		unlock_rename(cache->graveyard, dir);
-		_leave(" = 0 [culled?]");
-		return 0;
-	}
-
-	if (!d_can_lookup(cache->graveyard)) {
-		unlock_rename(cache->graveyard, dir);
-		cachefiles_io_error(cache, "Graveyard no longer a directory");
-		return -EIO;
-	}
-
-	if (trap == rep) {
-		unlock_rename(cache->graveyard, dir);
-		cachefiles_io_error(cache, "May not make directory loop");
+	rd.mnt_idmap = &nop_mnt_idmap;
+	rd.old_parent = dir;
+	rd.new_parent = cache->graveyard;
+	rd.flags = 0;
+	ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
+	if (ret) {
+		cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
 		return -EIO;
 	}
 
 	if (d_mountpoint(rep)) {
-		unlock_rename(cache->graveyard, dir);
+		end_renaming(&rd);
 		cachefiles_io_error(cache, "Mountpoint in cache");
 		return -EIO;
 	}
 
-	grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
-	if (IS_ERR(grave)) {
-		unlock_rename(cache->graveyard, dir);
-		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
-					   PTR_ERR(grave),
-					   cachefiles_trace_lookup_error);
-
-		if (PTR_ERR(grave) == -ENOMEM) {
-			_leave(" = -ENOMEM");
-			return -ENOMEM;
-		}
-
-		cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
-		return -EIO;
-	}
-
+	grave = rd.new_dentry;
 	if (d_is_positive(grave)) {
-		unlock_rename(cache->graveyard, dir);
-		dput(grave);
+		end_renaming(&rd);
 		grave = NULL;
 		cond_resched();
 		goto try_again;
 	}
 
 	if (d_mountpoint(grave)) {
-		unlock_rename(cache->graveyard, dir);
-		dput(grave);
+		end_renaming(&rd);
 		cachefiles_io_error(cache, "Mountpoint in graveyard");
 		return -EIO;
 	}
 
-	/* target should not be an ancestor of source */
-	if (trap == grave) {
-		unlock_rename(cache->graveyard, dir);
-		dput(grave);
-		cachefiles_io_error(cache, "May not make directory loop");
-		return -EIO;
-	}
-
 	/* attempt the rename */
 	path.mnt = cache->mnt;
 	path.dentry = dir;
@@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 	if (ret < 0) {
 		cachefiles_io_error(cache, "Rename security error %d", ret);
 	} else {
-		struct renamedata rd = {
-			.mnt_idmap	= &nop_mnt_idmap,
-			.old_parent	= dir,
-			.old_dentry	= rep,
-			.new_parent	= cache->graveyard,
-			.new_dentry	= grave,
-		};
 		trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
 		ret = cachefiles_inject_read_error();
 		if (ret == 0)
@@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 	}
 
 	__cachefiles_unmark_inode_in_use(object, d_inode(rep));
-	unlock_rename(cache->graveyard, dir);
-	dput(grave);
+	end_renaming(&rd);
 	_leave(" = 0");
 	return 0;
 }
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When ovl_create_real() is used to create a file on the upper filesystem
it needs to return the resulting dentry - positive and hashed.
It is usually the case the that dentry passed to the create function
(e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
filesystem may unhash that dentry forcing a repeat lookup next time the
name is wanted.

So ovl_create_real() must be (and is) aware of this and prepared to
perform that lookup to get a hash positive dentry.

This is currently done under that same directory lock that provided
exclusion for the create.  Proposed changes to locking will make this
not possible - as the name, rather than the directory, will be locked.
The new APIs provided for lookup and locking do not and cannot support
this pattern.

The lock isn't needed.  ovl_create_real() can drop the lock and then get
a new lock for the lookup - then check that the lookup returned the
correct inode.  In a well-behaved configuration where the upper
filesystem is not being modified by a third party, this will always work
reliably, and if there are separate modification it will fail cleanly.

So change ovl_create_real() to drop the lock and call
ovl_start_creating_upper() to find the correct dentry.  Note that
start_creating doesn't fail if the name already exists.

This removes the only remaining use of ovl_lookup_upper, so it is
removed.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
 fs/overlayfs/overlayfs.h |  7 -------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ff3dbd1ca61f..ec08904d084d 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
 		err = -EIO;
 	} else if (d_unhashed(newdentry)) {
 		struct dentry *d;
+		struct name_snapshot name;
 		/*
 		 * Some filesystems (i.e. casefolded) may return an unhashed
-		 * negative dentry from the ovl_lookup_upper() call before
+		 * negative dentry from the ovl_start_creating_upper() call before
 		 * ovl_create_real().
 		 * In that case, lookup again after making the newdentry
 		 * positive, so ovl_create_upper() always returns a hashed
 		 * positive dentry.
+		 * As we have to drop the lock before the lookup a race
+		 * could result in a lookup failure.  In that case we return
+		 * an error.
 		 */
-		d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
-				     newdentry->d_name.len);
-		dput(newdentry);
-		if (IS_ERR_OR_NULL(d))
+		take_dentry_name_snapshot(&name, newdentry);
+		end_creating_keep(newdentry);
+		d = ovl_start_creating_upper(ofs, parent, &name.name);
+		release_dentry_name_snapshot(&name);
+
+		if (IS_ERR_OR_NULL(d)) {
 			err = d ? PTR_ERR(d) : -ENOENT;
-		else
+		} else if (d->d_inode != newdentry->d_inode) {
+			err = -EIO;
+			dput(newdentry);
+		} else {
+			dput(newdentry);
 			return d;
+		}
+		return ERR_PTR(err);
 	}
 out:
 	if (err) {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 315882a360ce..4fb4750a83e4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -406,13 +406,6 @@ static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
 	return file;
 }
 
-static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
-					      const char *name,
-					      struct dentry *base, int len)
-{
-	return lookup_one(ovl_upper_mnt_idmap(ofs), &QSTR_LEN(name, len), base);
-}
-
 static inline struct dentry *ovl_lookup_upper_unlocked(struct ovl_fs *ofs,
 						       const char *name,
 						       struct dentry *base,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 11/13] ovl: use is_subdir() for testing if one thing is a subdir of another

[NeilBrown]

From: NeilBrown <neil@brown.name>

Rather than using lock_rename(), use the more obvious is_subdir() for
ensuring that neither upper nor workdir contain the other.
Also be explicit in the comment that the two directories cannot be the
same.

As this is a point-it-time sanity check and does not provide any
on-going guarantees, the removal of locking does not introduce any
interesting races.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/super.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ba9146f22a2c..2fd3e0aee50e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
 	return 0;
 }
 
-/* Workdir should not be subdir of upperdir and vice versa */
+/*
+ * Workdir should not be subdir of upperdir and vice versa, and
+ * they should not be the same.
+ */
 static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
 {
-	bool ok = false;
-
-	if (workdir != upperdir) {
-		struct dentry *trap = lock_rename(workdir, upperdir);
-		if (!IS_ERR(trap))
-			unlock_rename(workdir, upperdir);
-		ok = (trap == NULL);
-	}
-	return ok;
+	return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
 }
 
 static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 12/13] ovl: remove ovl_lock_rename_workdir()

[NeilBrown]

From: NeilBrown <neil@brown.name>

This function is unused.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/overlayfs.h |  2 --
 fs/overlayfs/util.c      | 25 -------------------------
 2 files changed, 27 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4fb4750a83e4..3eedc2684c23 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
 bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry);
 void ovl_nlink_end(struct dentry *dentry);
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
-			    struct dentry *upperdir, struct dentry *upper);
 int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
 			     struct ovl_metacopy *data);
 int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 94986d11a166..810c8752b4f7 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
 	ovl_inode_unlock(inode);
 }
 
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
-			    struct dentry *upperdir, struct dentry *upper)
-{
-	struct dentry *trap;
-
-	/* Workdir should not be subdir of upperdir and vice versa */
-	trap = lock_rename(workdir, upperdir);
-	if (IS_ERR(trap))
-		goto err;
-	if (trap)
-		goto err_unlock;
-	if (work && (work->d_parent != workdir || d_unhashed(work)))
-		goto err_unlock;
-	if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
-		goto err_unlock;
-
-	return 0;
-
-err_unlock:
-	unlock_rename(workdir, upperdir);
-err:
-	pr_err("failed to lock workdir+upperdir\n");
-	return -EIO;
-}
-
 /*
  * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
  * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 13/13] VFS: unexport lock_rename(), lock_rename_child(), unlock_rename()

[NeilBrown]

From: NeilBrown <neil@brown.name>

These three function are now only used in namei.c, so they don't need to
be exported.

Signed-off-by: NeilBrown <neil@brown.name>
---
 Documentation/filesystems/porting.rst | 7 +++++++
 fs/namei.c                            | 9 +++------
 include/linux/namei.h                 | 3 ---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index ed86c95d9d01..5f7008172f14 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1347,3 +1347,10 @@ implementation should set it to generic_setlease().
 
 lookup_one_qstr_excl() is no longer exported - use start_creating() or
 similar.
+---
+
+** mandatory**
+
+lock_rename(), lock_rename_child(), unlock_rename() are no
+longer available.  Use start_renaming() or similar.
+
diff --git a/fs/namei.c b/fs/namei.c
index 307b4d0866b8..0bc82bf90adc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3713,7 +3713,7 @@ static struct dentry *lock_two_directories(struct dentry *p1, struct dentry *p2)
 /*
  * p1 and p2 should be directories on the same fs.
  */
-struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
+static struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
 {
 	if (p1 == p2) {
 		inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
@@ -3723,12 +3723,11 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
 	mutex_lock(&p1->d_sb->s_vfs_rename_mutex);
 	return lock_two_directories(p1, p2);
 }
-EXPORT_SYMBOL(lock_rename);
 
 /*
  * c1 and p2 should be on the same fs.
  */
-struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
+static struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
 {
 	if (READ_ONCE(c1->d_parent) == p2) {
 		/*
@@ -3765,9 +3764,8 @@ struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
 	mutex_unlock(&c1->d_sb->s_vfs_rename_mutex);
 	return NULL;
 }
-EXPORT_SYMBOL(lock_rename_child);
 
-void unlock_rename(struct dentry *p1, struct dentry *p2)
+static void unlock_rename(struct dentry *p1, struct dentry *p2)
 {
 	inode_unlock(p1->d_inode);
 	if (p1 != p2) {
@@ -3775,7 +3773,6 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
 		mutex_unlock(&p1->d_sb->s_vfs_rename_mutex);
 	}
 }
-EXPORT_SYMBOL(unlock_rename);
 
 /**
  * __start_renaming - lookup and lock names for rename
diff --git a/include/linux/namei.h b/include/linux/namei.h
index c7a7288cdd25..2ad6dd9987b9 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -165,9 +165,6 @@ extern int follow_down_one(struct path *);
 extern int follow_down(struct path *path, unsigned int flags);
 extern int follow_up(struct path *);
 
-extern struct dentry *lock_rename(struct dentry *, struct dentry *);
-extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
-extern void unlock_rename(struct dentry *, struct dentry *);
 int start_renaming(struct renamedata *rd, int lookup_flags,
 		   struct qstr *old_last, struct qstr *new_last);
 int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()

[kernel test robot]

Hi NeilBrown,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brauner-vfs/vfs.all]
[also build test WARNING on viro-vfs/for-next linus/master v6.19-rc8 next-20260203]
[cannot apply to pcmoore-selinux/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/NeilBrown/fs-proc-Don-t-lock-root-inode-when-creating-self-and-thread-self/20260204-131659
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20260204050726.177283-5-neilb%40ownmail.net
patch subject: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()
config: arm-randconfig-r133-20260204 (https://download.01.org/0day-ci/archive/20260204/202602041851.x2RfFgKO-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260204/202602041851.x2RfFgKO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602041851.x2RfFgKO-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> security/apparmor/apparmorfs.c:295:16: sparse: sparse: Using plain integer as NULL pointer

vim +295 security/apparmor/apparmorfs.c

   247	
   248	/**
   249	 * aafs_create - create a dentry in the apparmorfs filesystem
   250	 *
   251	 * @name: name of dentry to create
   252	 * @mode: permissions the file should have
   253	 * @parent: parent directory for this dentry
   254	 * @data: data to store on inode.i_private, available in open()
   255	 * @link: if symlink, symlink target string
   256	 * @fops: struct file_operations that should be used for
   257	 * @iops: struct of inode_operations that should be used
   258	 *
   259	 * This is the basic "create a xxx" function for apparmorfs.
   260	 *
   261	 * Returns a pointer to a dentry if it succeeds, that must be free with
   262	 * aafs_remove(). Will return ERR_PTR on failure.
   263	 */
   264	static struct dentry *aafs_create(const char *name, umode_t mode,
   265					  struct dentry *parent, void *data, void *link,
   266					  const struct file_operations *fops,
   267					  const struct inode_operations *iops)
   268	{
   269		struct dentry *dentry;
   270		struct inode *dir;
   271		int error;
   272	
   273		AA_BUG(!name);
   274		AA_BUG(!parent);
   275	
   276		if (!(mode & S_IFMT))
   277			mode = (mode & S_IALLUGO) | S_IFREG;
   278	
   279		error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count);
   280		if (error)
   281			return ERR_PTR(error);
   282	
   283		dir = d_inode(parent);
   284	
   285		dentry = simple_start_creating(parent, name);
   286		if (IS_ERR(dentry)) {
   287			error = PTR_ERR(dentry);
   288			goto fail;
   289		}
   290	
   291		error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
   292		simple_done_creating(dentry);
   293		if (error)
   294			goto fail;
 > 295		return 0;
   296	fail:
   297		simple_release_fs(&aafs_mnt, &aafs_count);
   298		return ERR_PTR(error);
   299	}
   300	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 11/13] ovl: use is_subdir() for testing if one thing is a subdir of another

[Amir Goldstein]

On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> Rather than using lock_rename(), use the more obvious is_subdir() for
> ensuring that neither upper nor workdir contain the other.
> Also be explicit in the comment that the two directories cannot be the
> same.
>
> As this is a point-it-time sanity check and does not provide any
> on-going guarantees, the removal of locking does not introduce any
> interesting races.
>
> Signed-off-by: NeilBrown <neil@brown.name>

Looks reasonable

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/super.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ba9146f22a2c..2fd3e0aee50e 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
>         return 0;
>  }
>
> -/* Workdir should not be subdir of upperdir and vice versa */
> +/*
> + * Workdir should not be subdir of upperdir and vice versa, and
> + * they should not be the same.
> + */
>  static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
>  {
> -       bool ok = false;
> -
> -       if (workdir != upperdir) {
> -               struct dentry *trap = lock_rename(workdir, upperdir);
> -               if (!IS_ERR(trap))
> -                       unlock_rename(workdir, upperdir);
> -               ok = (trap == NULL);
> -       }
> -       return ok;
> +       return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
>  }
>
>  static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
> --
> 2.50.0.107.gf914562f5916.dirty
>

Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()

[Amir Goldstein]

On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> This function is unused.
>

I am confused.
What was this "fix" fixing an unused function:

e9c70084a64e5 ovl: fail ovl_lock_rename_workdir() if either target is unhashed

What am I missing?

Otherwise, feel free to add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

Re: [PATCH 03/13] libfs: change simple_done_creating() to use end_creating()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> simple_done_creating() and end_creating() are identical.
> So change the former to use the latter.  This further centralises
> unlocking of directories.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/libfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index f1860dff86f2..db18b53fc189 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
>  /* parent must have been held exclusive since simple_start_creating() */
>  void simple_done_creating(struct dentry *child)
>  {
> -	inode_unlock(child->d_parent->d_inode);
> -	dput(child);
> +	end_creating(child);
>  }
>  EXPORT_SYMBOL(simple_done_creating);

nit: seems like it would be better to turn this into a static inline
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[Amir Goldstein]

On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
> So ovl_create_real() must be (and is) aware of this and prepared to
> perform that lookup to get a hash positive dentry.
>
> This is currently done under that same directory lock that provided
> exclusion for the create.  Proposed changes to locking will make this
> not possible - as the name, rather than the directory, will be locked.
> The new APIs provided for lookup and locking do not and cannot support
> this pattern.
>
> The lock isn't needed.  ovl_create_real() can drop the lock and then get
> a new lock for the lookup - then check that the lookup returned the
> correct inode.  In a well-behaved configuration where the upper
> filesystem is not being modified by a third party, this will always work
> reliably, and if there are separate modification it will fail cleanly.
>
> So change ovl_create_real() to drop the lock and call
> ovl_start_creating_upper() to find the correct dentry.  Note that
> start_creating doesn't fail if the name already exists.
>
> This removes the only remaining use of ovl_lookup_upper, so it is
> removed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
>  fs/overlayfs/overlayfs.h |  7 -------
>  2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ff3dbd1ca61f..ec08904d084d 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
>                 err = -EIO;
>         } else if (d_unhashed(newdentry)) {
>                 struct dentry *d;
> +               struct name_snapshot name;
>                 /*
>                  * Some filesystems (i.e. casefolded) may return an unhashed
> -                * negative dentry from the ovl_lookup_upper() call before
> +                * negative dentry from the ovl_start_creating_upper() call before
>                  * ovl_create_real().


According to the new locking rules, if the hashed dentry itself is
the synchronization object, is it going to be allowed to
filesystem to unhash the dentry while the dentry still in the
"creating" scope? It is hard for me to wrap my head around this.

Or do we need this here because some filesystems (casefold in
particular) are not going to support parallel creations?

>                  * In that case, lookup again after making the newdentry
>                  * positive, so ovl_create_upper() always returns a hashed
>                  * positive dentry.
> +                * As we have to drop the lock before the lookup a race
> +                * could result in a lookup failure.  In that case we return
> +                * an error.
>                  */
> -               d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> -                                    newdentry->d_name.len);
> -               dput(newdentry);
> -               if (IS_ERR_OR_NULL(d))
> +               take_dentry_name_snapshot(&name, newdentry);
> +               end_creating_keep(newdentry);
> +               d = ovl_start_creating_upper(ofs, parent, &name.name);
> +               release_dentry_name_snapshot(&name);

OK. not saying no to this (yet) but I have to admit that it is pretty
ugly that the callers of ovl_create_real() want to create a specific
stable name, which is could be passed in as const char *name
and yet we end up doing this weird dance here just to keep the name
from newdentry.

Thanks,
Amir.

Re: [PATCH 01/13] fs/proc: Don't lock root inode when creating "self" and "thread-self"

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> proc_setup_self() and proc_setup_thread_self() are only called from
> proc_fill_super() which is before the filesystem is "live".  So there is
> no need to lock the root directory when adding "self" and "thread-self".
> This is clear from simple_fill_super() which provides similar
> functionality for other filesystems and does not lock anything.
> 
> The locking is not harmful, except that it may be confusing to a reader.
> As part of a an effort to centralise all locking for directories for
> name-based operations (prior to changing some locking rules), it is
> simplest to remove the locking here.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/proc/self.c        | 3 ---
>  fs/proc/thread_self.c | 3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/fs/proc/self.c b/fs/proc/self.c
> index 62d2c0cfe35c..56adf1c68f7a 100644
> --- a/fs/proc/self.c
> +++ b/fs/proc/self.c
> @@ -35,11 +35,9 @@ unsigned self_inum __ro_after_init;
>  
>  int proc_setup_self(struct super_block *s)
>  {
> -	struct inode *root_inode = d_inode(s->s_root);
>  	struct dentry *self;
>  	int ret = -ENOMEM;
>  
> -	inode_lock(root_inode);
>  	self = d_alloc_name(s->s_root, "self");
>  	if (self) {
>  		struct inode *inode = new_inode(s);
> @@ -55,7 +53,6 @@ int proc_setup_self(struct super_block *s)
>  		}
>  		dput(self);
>  	}
> -	inode_unlock(root_inode);
>  
>  	if (ret)
>  		pr_err("proc_fill_super: can't allocate /proc/self\n");
> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> index d6113dbe58e0..61ac62c3fd9f 100644
> --- a/fs/proc/thread_self.c
> +++ b/fs/proc/thread_self.c
> @@ -35,11 +35,9 @@ unsigned thread_self_inum __ro_after_init;
>  
>  int proc_setup_thread_self(struct super_block *s)
>  {
> -	struct inode *root_inode = d_inode(s->s_root);
>  	struct dentry *thread_self;
>  	int ret = -ENOMEM;
>  
> -	inode_lock(root_inode);
>  	thread_self = d_alloc_name(s->s_root, "thread-self");
>  	if (thread_self) {
>  		struct inode *inode = new_inode(s);
> @@ -55,7 +53,6 @@ int proc_setup_thread_self(struct super_block *s)
>  		}
>  		dput(thread_self);
>  	}
> -	inode_unlock(root_inode);
>  
>  	if (ret)
>  		pr_err("proc_fill_super: can't allocate /proc/thread-self\n");

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

Re: [PATCH 02/13] VFS: move the start_dirop() kerndoc comment to before start_dirop()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> This kerneldoc comment was always meant for start_dirop(), not for
> __start_dirop() which is a static function and doesn't need
> documentation.
> 
> It was in the wrong place and was then incorrectly renamed (instead of
> moved) and useless "documentation" was added for "@state" was provided.
> 
> This patch reverts the name, removes the mention of @state, and moves
> the comment to where it belongs.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index b28ecb699f32..40af78ddfb1b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2841,20 +2841,6 @@ static int filename_parentat(int dfd, struct filename *name,
>  	return __filename_parentat(dfd, name, flags, parent, last, type, NULL);
>  }
>  
> -/**
> - * __start_dirop - begin a create or remove dirop, performing locking and lookup
> - * @parent:       the dentry of the parent in which the operation will occur
> - * @name:         a qstr holding the name within that parent
> - * @lookup_flags: intent and other lookup flags.
> - * @state:        task state bitmask
> - *
> - * The lookup is performed and necessary locks are taken so that, on success,
> - * the returned dentry can be operated on safely.
> - * The qstr must already have the hash value calculated.
> - *
> - * Returns: a locked dentry, or an error.
> - *
> - */
>  static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
>  				    unsigned int lookup_flags,
>  				    unsigned int state)
> @@ -2876,6 +2862,19 @@ static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
>  	return dentry;
>  }
>  
> +/**
> + * start_dirop - begin a create or remove dirop, performing locking and lookup
> + * @parent:       the dentry of the parent in which the operation will occur
> + * @name:         a qstr holding the name within that parent
> + * @lookup_flags: intent and other lookup flags.
> + *
> + * The lookup is performed and necessary locks are taken so that, on success,
> + * the returned dentry can be operated on safely.
> + * The qstr must already have the hash value calculated.
> + *
> + * Returns: a locked dentry, or an error.
> + *
> + */
>  struct dentry *start_dirop(struct dentry *parent, struct qstr *name,
>  			   unsigned int lookup_flags)
>  {

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

Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
> 
> This extends the region that the directory is locked for, and also
> performs a lookup.
> The lock extension is of no real consequence.
> The lookup uses simple_lookup() and so always succeeds.  Thus when
> d_make_persistent() is called the dentry will already be hashed.
> d_make_persistent() handles this case.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  security/selinux/selinuxfs.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 896acad1f5f7..97e02cd5a9dc 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1930,15 +1930,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
>  static struct dentry *sel_make_swapover_dir(struct super_block *sb,
>  						unsigned long *ino)
>  {
> -	struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> +	struct dentry *dentry;
>  	struct inode *inode;
>  
> -	if (!dentry)
> +	inode = sel_make_inode(sb, S_IFDIR);
> +	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  
> -	inode = sel_make_inode(sb, S_IFDIR);
> -	if (!inode) {
> -		dput(dentry);
> +	dentry = simple_start_creating(sb->s_root, ".swapover");
> +	if (!dentry) {
> +		iput(inode);
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> @@ -1946,11 +1947,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
>  	inode->i_ino = ++(*ino);
>  	/* directory inodes start off with i_nlink == 2 (for "." entry) */
>  	inc_nlink(inode);
> -	inode_lock(sb->s_root->d_inode);
>  	d_make_persistent(dentry, inode);
>  	inc_nlink(sb->s_root->d_inode);
> -	inode_unlock(sb->s_root->d_inode);
> -	dput(dentry);
> +	simple_done_creating(dentry);
>  	return dentry;	// borrowed
>  }
>  

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

Re: [PATCH 06/13] nfsd: switch purge_old() to use start_removing_noperm()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather than explicit locking, use the start_removing_noperm() and
> end_removing() wrappers.
> This was not done with other start_removing changes due to conflicting
> in-flight patches.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4recover.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 441dfbfe2d2b..52fbe723a3c8 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -351,16 +351,14 @@ purge_old(struct dentry *parent, char *cname, struct nfsd_net *nn)
>  	if (nfs4_has_reclaimed_state(name, nn))
>  		goto out_free;
>  
> -	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> -	child = lookup_one(&nop_mnt_idmap, &QSTR(cname), parent);
> +	child = start_removing_noperm(parent, &QSTR(cname));
>  	if (!IS_ERR(child)) {
>  		status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child, NULL);
>  		if (status)
>  			printk("failed to remove client recovery directory %pd\n",
>  			       child);
> -		dput(child);
>  	}
> -	inode_unlock(d_inode(parent));
> +	end_removing(child);
>  
>  out_free:
>  	kfree(name.data);

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

Re: [PATCH 07/13] VFS: make lookup_one_qstr_excl() static.

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> lookup_one_qstr_excl() is no longer used outside of namei.c, so
> make it static.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst | 7 +++++++
>  fs/namei.c                            | 5 ++---
>  include/linux/namei.h                 | 3 ---
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed3ac56e3c76..ed86c95d9d01 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1340,3 +1340,10 @@ The ->setlease() file_operation must now be explicitly set in order to provide
>  support for leases. When set to NULL, the kernel will now return -EINVAL to
>  attempts to set a lease. Filesystems that wish to use the kernel-internal lease
>  implementation should set it to generic_setlease().
> +
> +---
> +
> +**mandatory**
> +
> +lookup_one_qstr_excl() is no longer exported - use start_creating() or
> +similar.
> diff --git a/fs/namei.c b/fs/namei.c
> index 40af78ddfb1b..307b4d0866b8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1730,8 +1730,8 @@ static struct dentry *lookup_dcache(const struct qstr *name,
>   * Will return -ENOENT if name isn't found and LOOKUP_CREATE wasn't passed.
>   * Will return -EEXIST if name is found and LOOKUP_EXCL was passed.
>   */
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> -				    struct dentry *base, unsigned int flags)
> +static struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> +					   struct dentry *base, unsigned int flags)
>  {
>  	struct dentry *dentry;
>  	struct dentry *old;
> @@ -1768,7 +1768,6 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
>  	}
>  	return dentry;
>  }
> -EXPORT_SYMBOL(lookup_one_qstr_excl);
>  
>  /**
>   * lookup_fast - do fast lockless (but racy) lookup of a dentry
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 58600cf234bc..c7a7288cdd25 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -54,9 +54,6 @@ extern int path_pts(struct path *path);
>  
>  extern int user_path_at(int, const char __user *, unsigned, struct path *);
>  
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> -				    struct dentry *base,
> -				    unsigned int flags);
>  extern int kern_path(const char *, unsigned, struct path *);
>  struct dentry *kern_path_parent(const char *name, struct path *parent);
>  

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

Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
> 
> The locking here isn't needed.  By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
> 
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
> 
> Also:
>  - don't take a reference to real->d_parent.  The parent is
>    only use for a pointer comparison, and no reference is needed for
>    that.
>  - Several "if" statements have a "goto" followed by "else" - the
>    else isn't needed: the following statement can directly follow
>    the "if" as a new statement
>  - Use a consistent pattern of setting "err" before performing a test
>    and possibly going to "fail".
>  - remove the "out" label (now that we don't need to dput(parent) or
>    unlock) and simply return from fail:.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..dcd28ffc4705 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
>  					  struct dentry *real,
>  					  const struct ovl_layer *layer)
>  {
> -	struct inode *dir = d_inode(connected);
> -	struct dentry *this, *parent = NULL;
> +	struct dentry *this;
>  	struct name_snapshot name;
>  	int err;
>  
>  	/*
> -	 * Lookup child overlay dentry by real name. The dir mutex protects us
> -	 * from racing with overlay rename. If the overlay dentry that is above
> -	 * real has already been moved to a parent that is not under the
> -	 * connected overlay dir, we return -ECHILD and restart the lookup of
> -	 * connected real path from the top.
> +	 * @connected is a directory in the overlay and @real is an object
> +	 * on @layer which is expected to be a child of @connected.
> +	 * The goal is to return a dentry from the overlay which corresponds
> +	 * to @real.  This is done by looking up the name from @real in
> +	 * @connected and checking that the result meets expectations.
> +	 *
> +	 * Return %-ECHILD if the parent of @real no-longer corresponds to
> +	 * @connected, and %-ESTALE if the dentry found by lookup doesn't
> +	 * correspond to @real.
>  	 */
> -	inode_lock_nested(dir, I_MUTEX_PARENT);
> -	err = -ECHILD;
> -	parent = dget_parent(real);
> -	if (ovl_dentry_real_at(connected, layer->idx) != parent)
> -		goto fail;
>  
> -	/*
> -	 * We also need to take a snapshot of real dentry name to protect us
> -	 * from racing with underlying layer rename. In this case, we don't
> -	 * care about returning ESTALE, only from dereferencing a free name
> -	 * pointer because we hold no lock on the real dentry.
> -	 */
>  	take_dentry_name_snapshot(&name, real);
> -	/*
> -	 * No idmap handling here: it's an internal lookup.
> -	 */
> -	this = lookup_noperm(&name.name, connected);
> +	this = lookup_noperm_unlocked(&name.name, connected);
>  	release_dentry_name_snapshot(&name);
> +
> +	err = -ECHILD;
> +	if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> +		goto fail;
> +
>  	err = PTR_ERR(this);
> -	if (IS_ERR(this)) {
> +	if (IS_ERR(this))
>  		goto fail;
> -	} else if (!this || !this->d_inode) {
> -		dput(this);
> -		err = -ENOENT;
> +
> +	err = -ENOENT;
> +	if (!this || !this->d_inode)
>  		goto fail;
> -	} else if (ovl_dentry_real_at(this, layer->idx) != real) {
> -		dput(this);
> -		err = -ESTALE;
> +
> +	err = -ESTALE;
> +	if (ovl_dentry_real_at(this, layer->idx) != real)
>  		goto fail;
> -	}
>  
> -out:
> -	dput(parent);
> -	inode_unlock(dir);
>  	return this;
>  
>  fail:
>  	pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
>  			    real, layer->idx, connected, err);
> -	this = ERR_PTR(err);
> -	goto out;
> +	if (!IS_ERR(this))
> +		dput(this);
> +	return ERR_PTR(err);
>  }
>  
>  static struct dentry *ovl_lookup_real(struct super_block *sb,

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

Re: [PATCH 09/13] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather then using lock_rename() and lookup_one() etc we can use
> the new start_renaming_dentry().  This is part of centralising dir
> locking and lookup so that locking rules can be changed.
> 
> Some error check are removed as not necessary.  Checks for rep being a
> non-dir or IS_DEADDIR and the check that ->graveyard is still a
> directory only provide slightly more informative errors and have been
> dropped.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
>  1 file changed, 14 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index e5ec90dccc27..3af42ec78411 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  			   struct dentry *rep,
>  			   enum fscache_why_object_killed why)
>  {
> -	struct dentry *grave, *trap;
> +	struct dentry *grave;
> +	struct renamedata rd = {};
>  	struct path path, path_to_graveyard;
>  	char nbuffer[8 + 8 + 1];
>  	int ret;
> @@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  		(uint32_t) ktime_get_real_seconds(),
>  		(uint32_t) atomic_inc_return(&cache->gravecounter));
>  
> -	/* do the multiway lock magic */
> -	trap = lock_rename(cache->graveyard, dir);
> -	if (IS_ERR(trap))
> -		return PTR_ERR(trap);
> -
> -	/* do some checks before getting the grave dentry */
> -	if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
> -		/* the entry was probably culled when we dropped the parent dir
> -		 * lock */
> -		unlock_rename(cache->graveyard, dir);
> -		_leave(" = 0 [culled?]");
> -		return 0;
> -	}
> -
> -	if (!d_can_lookup(cache->graveyard)) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "Graveyard no longer a directory");
> -		return -EIO;
> -	}
> -
> -	if (trap == rep) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "May not make directory loop");
> +	rd.mnt_idmap = &nop_mnt_idmap;
> +	rd.old_parent = dir;
> +	rd.new_parent = cache->graveyard;
> +	rd.flags = 0;
> +	ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
> +	if (ret) {
> +		cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
>  		return -EIO;
>  	}
>  
>  	if (d_mountpoint(rep)) {
> -		unlock_rename(cache->graveyard, dir);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in cache");
>  		return -EIO;
>  	}
>  
> -	grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
> -	if (IS_ERR(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> -					   PTR_ERR(grave),
> -					   cachefiles_trace_lookup_error);
> -
> -		if (PTR_ERR(grave) == -ENOMEM) {
> -			_leave(" = -ENOMEM");
> -			return -ENOMEM;
> -		}
> -
> -		cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
> -		return -EIO;
> -	}
> -
> +	grave = rd.new_dentry;
>  	if (d_is_positive(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		grave = NULL;
>  		cond_resched();
>  		goto try_again;
>  	}
>  
>  	if (d_mountpoint(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in graveyard");
>  		return -EIO;
>  	}
>  
> -	/* target should not be an ancestor of source */
> -	if (trap == grave) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> -		cachefiles_io_error(cache, "May not make directory loop");
> -		return -EIO;
> -	}
> -
>  	/* attempt the rename */
>  	path.mnt = cache->mnt;
>  	path.dentry = dir;
> @@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	if (ret < 0) {
>  		cachefiles_io_error(cache, "Rename security error %d", ret);
>  	} else {
> -		struct renamedata rd = {
> -			.mnt_idmap	= &nop_mnt_idmap,
> -			.old_parent	= dir,
> -			.old_dentry	= rep,
> -			.new_parent	= cache->graveyard,
> -			.new_dentry	= grave,
> -		};
>  		trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
>  		ret = cachefiles_inject_read_error();
>  		if (ret == 0)
> @@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	}
>  
>  	__cachefiles_unmark_inode_in_use(object, d_inode(rep));
> -	unlock_rename(cache->graveyard, dir);
> -	dput(grave);
> +	end_renaming(&rd);
>  	_leave(" = 0");
>  	return 0;
>  }

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

Re: [PATCH 11/13] ovl: use is_subdir() for testing if one thing is a subdir of another

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather than using lock_rename(), use the more obvious is_subdir() for
> ensuring that neither upper nor workdir contain the other.
> Also be explicit in the comment that the two directories cannot be the
> same.
> 
> As this is a point-it-time sanity check and does not provide any
> on-going guarantees, the removal of locking does not introduce any
> interesting races.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/super.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ba9146f22a2c..2fd3e0aee50e 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
>  	return 0;
>  }
>  
> -/* Workdir should not be subdir of upperdir and vice versa */
> +/*
> + * Workdir should not be subdir of upperdir and vice versa, and
> + * they should not be the same.
> + */
>  static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
>  {
> -	bool ok = false;
> -
> -	if (workdir != upperdir) {
> -		struct dentry *trap = lock_rename(workdir, upperdir);
> -		if (!IS_ERR(trap))
> -			unlock_rename(workdir, upperdir);
> -		ok = (trap == NULL);
> -	}
> -	return ok;
> +	return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
>  }
>  
>  static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,

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

Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> This function is unused.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/overlayfs.h |  2 --
>  fs/overlayfs/util.c      | 25 -------------------------
>  2 files changed, 27 deletions(-)
> 
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
>  			     struct ovl_metacopy *data);
>  int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
>  	ovl_inode_unlock(inode);
>  }
>  
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper)
> -{
> -	struct dentry *trap;
> -
> -	/* Workdir should not be subdir of upperdir and vice versa */
> -	trap = lock_rename(workdir, upperdir);
> -	if (IS_ERR(trap))
> -		goto err;
> -	if (trap)
> -		goto err_unlock;
> -	if (work && (work->d_parent != workdir || d_unhashed(work)))
> -		goto err_unlock;
> -	if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> -		goto err_unlock;
> -
> -	return 0;
> -
> -err_unlock:
> -	unlock_rename(workdir, upperdir);
> -err:
> -	pr_err("failed to lock workdir+upperdir\n");
> -	return -EIO;
> -}
> -
>  /*
>   * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
>   * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.

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

Re: [PATCH 13/13] VFS: unexport lock_rename(), lock_rename_child(), unlock_rename()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> These three function are now only used in namei.c, so they don't need to
> be exported.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst | 7 +++++++
>  fs/namei.c                            | 9 +++------
>  include/linux/namei.h                 | 3 ---
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed86c95d9d01..5f7008172f14 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1347,3 +1347,10 @@ implementation should set it to generic_setlease().
>  
>  lookup_one_qstr_excl() is no longer exported - use start_creating() or
>  similar.
> +---
> +
> +** mandatory**
> +
> +lock_rename(), lock_rename_child(), unlock_rename() are no
> +longer available.  Use start_renaming() or similar.
> +
> diff --git a/fs/namei.c b/fs/namei.c
> index 307b4d0866b8..0bc82bf90adc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3713,7 +3713,7 @@ static struct dentry *lock_two_directories(struct dentry *p1, struct dentry *p2)
>  /*
>   * p1 and p2 should be directories on the same fs.
>   */
> -struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
> +static struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
>  {
>  	if (p1 == p2) {
>  		inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
> @@ -3723,12 +3723,11 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
>  	mutex_lock(&p1->d_sb->s_vfs_rename_mutex);
>  	return lock_two_directories(p1, p2);
>  }
> -EXPORT_SYMBOL(lock_rename);
>  
>  /*
>   * c1 and p2 should be on the same fs.
>   */
> -struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
> +static struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
>  {
>  	if (READ_ONCE(c1->d_parent) == p2) {
>  		/*
> @@ -3765,9 +3764,8 @@ struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
>  	mutex_unlock(&c1->d_sb->s_vfs_rename_mutex);
>  	return NULL;
>  }
> -EXPORT_SYMBOL(lock_rename_child);
>  
> -void unlock_rename(struct dentry *p1, struct dentry *p2)
> +static void unlock_rename(struct dentry *p1, struct dentry *p2)
>  {
>  	inode_unlock(p1->d_inode);
>  	if (p1 != p2) {
> @@ -3775,7 +3773,6 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
>  		mutex_unlock(&p1->d_sb->s_vfs_rename_mutex);
>  	}
>  }
> -EXPORT_SYMBOL(unlock_rename);
>  
>  /**
>   * __start_renaming - lookup and lock names for rename
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index c7a7288cdd25..2ad6dd9987b9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -165,9 +165,6 @@ extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *path, unsigned int flags);
>  extern int follow_up(struct path *);
>  
> -extern struct dentry *lock_rename(struct dentry *, struct dentry *);
> -extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
> -extern void unlock_rename(struct dentry *, struct dentry *);
>  int start_renaming(struct renamedata *rd, int lookup_flags,
>  		   struct qstr *old_last, struct qstr *new_last);
>  int start_renaming_dentry(struct renamedata *rd, int lookup_flags,

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

Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Instead of explicitly locking the parent and performing a look up in
> apparmor, use simple_start_creating(), and then simple_done_creating()
> to unlock and drop the dentry.
> 
> This removes the need to check for an existing entry (as
> simple_start_creating() acts like an exclusive create and can return
> -EEXIST), simplifies error paths, and keeps dir locking code
> centralised.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 907bd2667e28..7f78c36e6e50 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
>  
>  	dir = d_inode(parent);
>  
> -	inode_lock(dir);
> -	dentry = lookup_noperm(&QSTR(name), parent);
> +	dentry = simple_start_creating(parent, name);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
> -		goto fail_lock;
> -	}
> -
> -	if (d_really_is_positive(dentry)) {
> -		error = -EEXIST;
> -		goto fail_dentry;
> +		goto fail;
>  	}
>  
>  	error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> +	simple_done_creating(dentry);
>  	if (error)
> -		goto fail_dentry;
> -	inode_unlock(dir);
> -
> -	return dentry;
> -
> -fail_dentry:
> -	dput(dentry);
> -
> -fail_lock:
> -	inode_unlock(dir);
> +		goto fail;
> +	return 0;

As KTR points out, this should be "return NULL;"

> +fail:
>  	simple_release_fs(&aafs_mnt, &aafs_count);
> -
>  	return ERR_PTR(error);
>  }
>  
> @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  	if (error)
>  		return error;
>  
> -	inode_lock(d_inode(parent));
> -	dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> +	dentry = simple_start_creating(parent, NULL_FILE_NAME);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
>  		goto out;
> @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  	inode = new_inode(parent->d_inode->i_sb);
>  	if (!inode) {
>  		error = -ENOMEM;
> -		goto out1;
> +		goto out;
>  	}
>  
>  	inode->i_ino = get_next_ino();
> @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
>  	aa_null.dentry = dget(dentry);
>  	aa_null.mnt = mntget(mount);
>  
> -	error = 0;
> -
> -out1:
> -	dput(dentry);
>  out:
> -	inode_unlock(d_inode(parent));
> +	simple_done_creating(dentry);
>  	simple_release_fs(&mount, &count);
>  	return error;
>  }
>  
> -
> -
>  static const char *policy_get_link(struct dentry *dentry,
>  				   struct inode *inode,
>  				   struct delayed_call *done)

Assuming you fix the minor problem above.

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

Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()

[Jeff Layton]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> This function is unused.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/overlayfs.h |  2 --
>  fs/overlayfs/util.c      | 25 -------------------------
>  2 files changed, 27 deletions(-)
> 
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
>  			     struct ovl_metacopy *data);
>  int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
>  	ovl_inode_unlock(inode);
>  }
>  
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper)
> -{
> -	struct dentry *trap;
> -
> -	/* Workdir should not be subdir of upperdir and vice versa */
> -	trap = lock_rename(workdir, upperdir);
> -	if (IS_ERR(trap))
> -		goto err;
> -	if (trap)
> -		goto err_unlock;
> -	if (work && (work->d_parent != workdir || d_unhashed(work)))
> -		goto err_unlock;
> -	if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> -		goto err_unlock;
> -
> -	return 0;
> -
> -err_unlock:
> -	unlock_rename(workdir, upperdir);
> -err:
> -	pr_err("failed to lock workdir+upperdir\n");
> -	return -EIO;
> -}
> -
>  /*
>   * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
>   * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.

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

Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()

[Amir Goldstein]

On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > The primary purpose of this patch is to remove the locking from
> > ovl_lookup_real_one() as part of centralising all locking of directories
> > for name operations.
> >
> > The locking here isn't needed.  By performing consistency tests after
> > the lookup we can be sure that the result of the lookup was valid at
> > least for a moment, which is all the original code promised.
> >
> > lookup_noperm_unlocked() is used for the lookup and it will take the
> > lock if needed only where it is needed.
> >
> > Also:
> >  - don't take a reference to real->d_parent.  The parent is
> >    only use for a pointer comparison, and no reference is needed for
> >    that.
> >  - Several "if" statements have a "goto" followed by "else" - the
> >    else isn't needed: the following statement can directly follow
> >    the "if" as a new statement
> >  - Use a consistent pattern of setting "err" before performing a test
> >    and possibly going to "fail".
> >  - remove the "out" label (now that we don't need to dput(parent) or
> >    unlock) and simply return from fail:.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> >  1 file changed, 26 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 83f80fdb1567..dcd28ffc4705 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> >                                         struct dentry *real,
> >                                         const struct ovl_layer *layer)
> >  {
> > -     struct inode *dir = d_inode(connected);
> > -     struct dentry *this, *parent = NULL;
> > +     struct dentry *this;
> >       struct name_snapshot name;
> >       int err;
> >
> >       /*
> > -      * Lookup child overlay dentry by real name. The dir mutex protects us
> > -      * from racing with overlay rename. If the overlay dentry that is above
> > -      * real has already been moved to a parent that is not under the
> > -      * connected overlay dir, we return -ECHILD and restart the lookup of
> > -      * connected real path from the top.
> > +      * @connected is a directory in the overlay and @real is an object
> > +      * on @layer which is expected to be a child of @connected.
> > +      * The goal is to return a dentry from the overlay which corresponds

As the header comment already says:
"...return a connected overlay dentry whose real dentry is @real"

The wording "corresponds to @real" reduces clarity IMO.

> > +      * to @real.  This is done by looking up the name from @real in
> > +      * @connected and checking that the result meets expectations.
> > +      *
> > +      * Return %-ECHILD if the parent of @real no-longer corresponds to
> > +      * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > +      * correspond to @real.
> >        */

I dislike kernel-doc inside code comments.
I think this is actively discouraged and I haven't found a single example
of this style in fs code.

If you want to keep this format, please lift the comment to function
header comment - it is anyway a very generic comment that explains the
function in general.

> > -     inode_lock_nested(dir, I_MUTEX_PARENT);
> > -     err = -ECHILD;
> > -     parent = dget_parent(real);
> > -     if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > -             goto fail;
> >
> > -     /*
> > -      * We also need to take a snapshot of real dentry name to protect us
> > -      * from racing with underlying layer rename. In this case, we don't
> > -      * care about returning ESTALE, only from dereferencing a free name
> > -      * pointer because we hold no lock on the real dentry.
> > -      */
> >       take_dentry_name_snapshot(&name, real);
> > -     /*
> > -      * No idmap handling here: it's an internal lookup.
> > -      */
> > -     this = lookup_noperm(&name.name, connected);
> > +     this = lookup_noperm_unlocked(&name.name, connected);
> >       release_dentry_name_snapshot(&name);
> > +
> > +     err = -ECHILD;
> > +     if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > +             goto fail;
> > +
> >       err = PTR_ERR(this);
> > -     if (IS_ERR(this)) {
> > +     if (IS_ERR(this))
> >               goto fail;
> > -     } else if (!this || !this->d_inode) {
> > -             dput(this);
> > -             err = -ENOENT;
> > +
> > +     err = -ENOENT;
> > +     if (!this || !this->d_inode)
> >               goto fail;
> > -     } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > -             dput(this);
> > -             err = -ESTALE;
> > +
> > +     err = -ESTALE;
> > +     if (ovl_dentry_real_at(this, layer->idx) != real)
> >               goto fail;
> > -     }
> >
> > -out:
> > -     dput(parent);
> > -     inode_unlock(dir);
> >       return this;
> >
> >  fail:
> >       pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> >                           real, layer->idx, connected, err);
> > -     this = ERR_PTR(err);
> > -     goto out;
> > +     if (!IS_ERR(this))
> > +             dput(this);
> > +     return ERR_PTR(err);
> >  }
> >
> >  static struct dentry *ovl_lookup_real(struct super_block *sb,
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Otherwise, it looks fine.

Thanks,
Amir.

Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > Instead of explicitly locking the parent and performing a look up in
> > apparmor, use simple_start_creating(), and then simple_done_creating()
> > to unlock and drop the dentry.
> > 
> > This removes the need to check for an existing entry (as
> > simple_start_creating() acts like an exclusive create and can return
> > -EEXIST), simplifies error paths, and keeps dir locking code
> > centralised.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
> >  1 file changed, 9 insertions(+), 29 deletions(-)
> > 
> > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> > index 907bd2667e28..7f78c36e6e50 100644
> > --- a/security/apparmor/apparmorfs.c
> > +++ b/security/apparmor/apparmorfs.c
> > @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
> >  
> >  	dir = d_inode(parent);
> >  
> > -	inode_lock(dir);
> > -	dentry = lookup_noperm(&QSTR(name), parent);
> > +	dentry = simple_start_creating(parent, name);
> >  	if (IS_ERR(dentry)) {
> >  		error = PTR_ERR(dentry);
> > -		goto fail_lock;
> > -	}
> > -
> > -	if (d_really_is_positive(dentry)) {
> > -		error = -EEXIST;
> > -		goto fail_dentry;
> > +		goto fail;
> >  	}
> >  
> >  	error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> > +	simple_done_creating(dentry);
> >  	if (error)
> > -		goto fail_dentry;
> > -	inode_unlock(dir);
> > -
> > -	return dentry;
> > -
> > -fail_dentry:
> > -	dput(dentry);
> > -
> > -fail_lock:
> > -	inode_unlock(dir);
> > +		goto fail;
> > +	return 0;
> 
> As KTR points out, this should be "return NULL;"

Actually it should be "return dentry;" which is what the original code
did.
I've no idea how it became 0...
Callers of aafs_create() will silently treat NULL as failure.

> 
> > +fail:
> >  	simple_release_fs(&aafs_mnt, &aafs_count);
> > -
> >  	return ERR_PTR(error);
> >  }
> >  
> > @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
> >  	if (error)
> >  		return error;
> >  
> > -	inode_lock(d_inode(parent));
> > -	dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> > +	dentry = simple_start_creating(parent, NULL_FILE_NAME);
> >  	if (IS_ERR(dentry)) {
> >  		error = PTR_ERR(dentry);
> >  		goto out;
> > @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
> >  	inode = new_inode(parent->d_inode->i_sb);
> >  	if (!inode) {
> >  		error = -ENOMEM;
> > -		goto out1;
> > +		goto out;
> >  	}
> >  
> >  	inode->i_ino = get_next_ino();
> > @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
> >  	aa_null.dentry = dget(dentry);
> >  	aa_null.mnt = mntget(mount);
> >  
> > -	error = 0;
> > -
> > -out1:
> > -	dput(dentry);
> >  out:
> > -	inode_unlock(d_inode(parent));
> > +	simple_done_creating(dentry);
> >  	simple_release_fs(&mount, &count);
> >  	return error;
> >  }
> >  
> > -
> > -
> >  static const char *policy_get_link(struct dentry *dentry,
> >  				   struct inode *inode,
> >  				   struct delayed_call *done)
> 
> Assuming you fix the minor problem above.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 

Thanks,
NeilBrown

Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()

[NeilBrown]

On Fri, 06 Feb 2026, Amir Goldstein wrote:
> On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > >
> > > The primary purpose of this patch is to remove the locking from
> > > ovl_lookup_real_one() as part of centralising all locking of directories
> > > for name operations.
> > >
> > > The locking here isn't needed.  By performing consistency tests after
> > > the lookup we can be sure that the result of the lookup was valid at
> > > least for a moment, which is all the original code promised.
> > >
> > > lookup_noperm_unlocked() is used for the lookup and it will take the
> > > lock if needed only where it is needed.
> > >
> > > Also:
> > >  - don't take a reference to real->d_parent.  The parent is
> > >    only use for a pointer comparison, and no reference is needed for
> > >    that.
> > >  - Several "if" statements have a "goto" followed by "else" - the
> > >    else isn't needed: the following statement can directly follow
> > >    the "if" as a new statement
> > >  - Use a consistent pattern of setting "err" before performing a test
> > >    and possibly going to "fail".
> > >  - remove the "out" label (now that we don't need to dput(parent) or
> > >    unlock) and simply return from fail:.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> > >  1 file changed, 26 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index 83f80fdb1567..dcd28ffc4705 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > >                                         struct dentry *real,
> > >                                         const struct ovl_layer *layer)
> > >  {
> > > -     struct inode *dir = d_inode(connected);
> > > -     struct dentry *this, *parent = NULL;
> > > +     struct dentry *this;
> > >       struct name_snapshot name;
> > >       int err;
> > >
> > >       /*
> > > -      * Lookup child overlay dentry by real name. The dir mutex protects us
> > > -      * from racing with overlay rename. If the overlay dentry that is above
> > > -      * real has already been moved to a parent that is not under the
> > > -      * connected overlay dir, we return -ECHILD and restart the lookup of
> > > -      * connected real path from the top.
> > > +      * @connected is a directory in the overlay and @real is an object
> > > +      * on @layer which is expected to be a child of @connected.
> > > +      * The goal is to return a dentry from the overlay which corresponds
> 
> As the header comment already says:
> "...return a connected overlay dentry whose real dentry is @real"
> 
> The wording "corresponds to @real" reduces clarity IMO.

Ok, I'll rephrase.


> 
> > > +      * to @real.  This is done by looking up the name from @real in
> > > +      * @connected and checking that the result meets expectations.
> > > +      *
> > > +      * Return %-ECHILD if the parent of @real no-longer corresponds to
> > > +      * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > > +      * correspond to @real.
> > >        */
> 
> I dislike kernel-doc inside code comments.
> I think this is actively discouraged and I haven't found a single example
> of this style in fs code.
> 
> If you want to keep this format, please lift the comment to function
> header comment - it is anyway a very generic comment that explains the
> function in general.

OK, I'll remove the formatting or move it - not sure which.
I find that with parameter names like "connected" and "real", some sort
of syntax helps.


> 
> > > -     inode_lock_nested(dir, I_MUTEX_PARENT);
> > > -     err = -ECHILD;
> > > -     parent = dget_parent(real);
> > > -     if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > > -             goto fail;
> > >
> > > -     /*
> > > -      * We also need to take a snapshot of real dentry name to protect us
> > > -      * from racing with underlying layer rename. In this case, we don't
> > > -      * care about returning ESTALE, only from dereferencing a free name
> > > -      * pointer because we hold no lock on the real dentry.
> > > -      */
> > >       take_dentry_name_snapshot(&name, real);
> > > -     /*
> > > -      * No idmap handling here: it's an internal lookup.
> > > -      */
> > > -     this = lookup_noperm(&name.name, connected);
> > > +     this = lookup_noperm_unlocked(&name.name, connected);
> > >       release_dentry_name_snapshot(&name);
> > > +
> > > +     err = -ECHILD;
> > > +     if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > > +             goto fail;
> > > +
> > >       err = PTR_ERR(this);
> > > -     if (IS_ERR(this)) {
> > > +     if (IS_ERR(this))
> > >               goto fail;
> > > -     } else if (!this || !this->d_inode) {
> > > -             dput(this);
> > > -             err = -ENOENT;
> > > +
> > > +     err = -ENOENT;
> > > +     if (!this || !this->d_inode)
> > >               goto fail;
> > > -     } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > > -             dput(this);
> > > -             err = -ESTALE;
> > > +
> > > +     err = -ESTALE;
> > > +     if (ovl_dentry_real_at(this, layer->idx) != real)
> > >               goto fail;
> > > -     }
> > >
> > > -out:
> > > -     dput(parent);
> > > -     inode_unlock(dir);
> > >       return this;
> > >
> > >  fail:
> > >       pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> > >                           real, layer->idx, connected, err);
> > > -     this = ERR_PTR(err);
> > > -     goto out;
> > > +     if (!IS_ERR(this))
> > > +             dput(this);
> > > +     return ERR_PTR(err);
> > >  }
> > >
> > >  static struct dentry *ovl_lookup_real(struct super_block *sb,
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 
> Otherwise, it looks fine.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[NeilBrown]

On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > When ovl_create_real() is used to create a file on the upper filesystem
> > it needs to return the resulting dentry - positive and hashed.
> > It is usually the case the that dentry passed to the create function
> > (e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
> > filesystem may unhash that dentry forcing a repeat lookup next time the
> > name is wanted.
> >
> > So ovl_create_real() must be (and is) aware of this and prepared to
> > perform that lookup to get a hash positive dentry.
> >
> > This is currently done under that same directory lock that provided
> > exclusion for the create.  Proposed changes to locking will make this
> > not possible - as the name, rather than the directory, will be locked.
> > The new APIs provided for lookup and locking do not and cannot support
> > this pattern.
> >
> > The lock isn't needed.  ovl_create_real() can drop the lock and then get
> > a new lock for the lookup - then check that the lookup returned the
> > correct inode.  In a well-behaved configuration where the upper
> > filesystem is not being modified by a third party, this will always work
> > reliably, and if there are separate modification it will fail cleanly.
> >
> > So change ovl_create_real() to drop the lock and call
> > ovl_start_creating_upper() to find the correct dentry.  Note that
> > start_creating doesn't fail if the name already exists.
> >
> > This removes the only remaining use of ovl_lookup_upper, so it is
> > removed.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
> >  fs/overlayfs/overlayfs.h |  7 -------
> >  2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index ff3dbd1ca61f..ec08904d084d 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> >                 err = -EIO;
> >         } else if (d_unhashed(newdentry)) {
> >                 struct dentry *d;
> > +               struct name_snapshot name;
> >                 /*
> >                  * Some filesystems (i.e. casefolded) may return an unhashed
> > -                * negative dentry from the ovl_lookup_upper() call before
> > +                * negative dentry from the ovl_start_creating_upper() call before
> >                  * ovl_create_real().
> 
> 
> According to the new locking rules, if the hashed dentry itself is
> the synchronization object, is it going to be allowed to
> filesystem to unhash the dentry while the dentry still in the
> "creating" scope? It is hard for me to wrap my head around this.

It can be confusing....

It will be important for the name the remain locked (and hashed) until
the operation (create, remove, rename) either succeeds or fails.  So
leaving a dentry unhashed will be OK providing a subsequent lookup will
also succeed or fail in the same way.  The caller must be able to use
the dentry to access the object (i.e.  the inode) on success, but they
is nothing in POSIX that requires that the object still has any
particular name.

> 
> Or do we need this here because some filesystems (casefold in
> particular) are not going to support parallel creations?

There is no reason that a casefolding filesystem would not support parallel
ops. And it isn't just casefolding that acts like this.  At least one of
the special filesystems (tracefs maybe) always unhashes on create.  You
only ever get a hashed positive dentry as a result of lookup.
(overlayfs would never see this case of course).

> 
> >                  * In that case, lookup again after making the newdentry
> >                  * positive, so ovl_create_upper() always returns a hashed
> >                  * positive dentry.
> > +                * As we have to drop the lock before the lookup a race
> > +                * could result in a lookup failure.  In that case we return
> > +                * an error.
> >                  */
> > -               d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > -                                    newdentry->d_name.len);
> > -               dput(newdentry);
> > -               if (IS_ERR_OR_NULL(d))
> > +               take_dentry_name_snapshot(&name, newdentry);
> > +               end_creating_keep(newdentry);
> > +               d = ovl_start_creating_upper(ofs, parent, &name.name);
> > +               release_dentry_name_snapshot(&name);
> 
> OK. not saying no to this (yet) but I have to admit that it is pretty
> ugly that the callers of ovl_create_real() want to create a specific
> stable name, which is could be passed in as const char *name
> and yet we end up doing this weird dance here just to keep the name
> from newdentry.

There are three callers of ovl_create_real()

ovl_lookup_or_create() does have a "const char *name".
ovl_create_upper() has a stable dentry from which it can copy a QSTR
ovl_create_temp() would need some sort of dance to keep hold of the
temporary name that was allocated.

If it weren't for ovl_create_temp() I would agree with you.

Though we could have the three callers of ovl_start_creating_temp() pass a
"char name[OVL_TEMPNAME_SIZE]" in, then ovl_create_temp() would have
easy access.
I could do that if you like.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()

[NeilBrown]

On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > This function is unused.
> >
> 
> I am confused.
> What was this "fix" fixing an unused function:
> 
> e9c70084a64e5 ovl: fail ovl_lock_rename_workdir() if either target is unhashed
> 
> What am I missing?
> 

Commit 833d2b3a072f ("Add start_renaming_two_dentries()")

removed the last use of ovl_lock_rename_workdir() earlier, but in a
different branch.

e9c was committed upstream Nov 28th v6.18~7
833 was committed upstream Dec 1st  v6.19-rc1~240

> Otherwise, feel free to add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Re: [PATCH 03/13] libfs: change simple_done_creating() to use end_creating()

[NeilBrown]

On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > simple_done_creating() and end_creating() are identical.
> > So change the former to use the latter.  This further centralises
> > unlocking of directories.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/libfs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index f1860dff86f2..db18b53fc189 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
> >  /* parent must have been held exclusive since simple_start_creating() */
> >  void simple_done_creating(struct dentry *child)
> >  {
> > -	inode_unlock(child->d_parent->d_inode);
> > -	dput(child);
> > +	end_creating(child);
> >  }
> >  EXPORT_SYMBOL(simple_done_creating);
> 
> nit: seems like it would be better to turn this into a static inline

True ... but then it could have been a static inline anyway.
I'd rather not change it without good reason, or knowing what it was
written that way.

Al: do you have an opinion on this?

Thanks,
NeilBrown

Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[Amir Goldstein]

On Fri, Feb 6, 2026 at 2:11 AM NeilBrown <neilb@ownmail.net> wrote:
>
> On Thu, 05 Feb 2026, Amir Goldstein wrote:
> > On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> > >
> > > From: NeilBrown <neil@brown.name>
> > >
> > > When ovl_create_real() is used to create a file on the upper filesystem
> > > it needs to return the resulting dentry - positive and hashed.
> > > It is usually the case the that dentry passed to the create function
> > > (e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
> > > filesystem may unhash that dentry forcing a repeat lookup next time the
> > > name is wanted.
> > >
> > > So ovl_create_real() must be (and is) aware of this and prepared to
> > > perform that lookup to get a hash positive dentry.
> > >
> > > This is currently done under that same directory lock that provided
> > > exclusion for the create.  Proposed changes to locking will make this
> > > not possible - as the name, rather than the directory, will be locked.
> > > The new APIs provided for lookup and locking do not and cannot support
> > > this pattern.
> > >
> > > The lock isn't needed.  ovl_create_real() can drop the lock and then get
> > > a new lock for the lookup - then check that the lookup returned the
> > > correct inode.  In a well-behaved configuration where the upper
> > > filesystem is not being modified by a third party, this will always work
> > > reliably, and if there are separate modification it will fail cleanly.
> > >
> > > So change ovl_create_real() to drop the lock and call
> > > ovl_start_creating_upper() to find the correct dentry.  Note that
> > > start_creating doesn't fail if the name already exists.
> > >
> > > This removes the only remaining use of ovl_lookup_upper, so it is
> > > removed.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
> > >  fs/overlayfs/overlayfs.h |  7 -------
> > >  2 files changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > > index ff3dbd1ca61f..ec08904d084d 100644
> > > --- a/fs/overlayfs/dir.c
> > > +++ b/fs/overlayfs/dir.c
> > > @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > >                 err = -EIO;
> > >         } else if (d_unhashed(newdentry)) {
> > >                 struct dentry *d;
> > > +               struct name_snapshot name;
> > >                 /*
> > >                  * Some filesystems (i.e. casefolded) may return an unhashed
> > > -                * negative dentry from the ovl_lookup_upper() call before
> > > +                * negative dentry from the ovl_start_creating_upper() call before
> > >                  * ovl_create_real().
> >
> >
> > According to the new locking rules, if the hashed dentry itself is
> > the synchronization object, is it going to be allowed to
> > filesystem to unhash the dentry while the dentry still in the
> > "creating" scope? It is hard for me to wrap my head around this.
>
> It can be confusing....
>
> It will be important for the name the remain locked (and hashed) until
> the operation (create, remove, rename) either succeeds or fails.  So
> leaving a dentry unhashed will be OK providing a subsequent lookup will
> also succeed or fail in the same way.  The caller must be able to use
> the dentry to access the object (i.e.  the inode) on success, but they
> is nothing in POSIX that requires that the object still has any
> particular name.
>
> >
> > Or do we need this here because some filesystems (casefold in
> > particular) are not going to support parallel creations?
>
> There is no reason that a casefolding filesystem would not support parallel
> ops. And it isn't just casefolding that acts like this.  At least one of
> the special filesystems (tracefs maybe) always unhashes on create.  You
> only ever get a hashed positive dentry as a result of lookup.
> (overlayfs would never see this case of course).
>
> >
> > >                  * In that case, lookup again after making the newdentry
> > >                  * positive, so ovl_create_upper() always returns a hashed
> > >                  * positive dentry.
> > > +                * As we have to drop the lock before the lookup a race
> > > +                * could result in a lookup failure.  In that case we return
> > > +                * an error.
> > >                  */
> > > -               d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > > -                                    newdentry->d_name.len);
> > > -               dput(newdentry);
> > > -               if (IS_ERR_OR_NULL(d))
> > > +               take_dentry_name_snapshot(&name, newdentry);
> > > +               end_creating_keep(newdentry);
> > > +               d = ovl_start_creating_upper(ofs, parent, &name.name);
> > > +               release_dentry_name_snapshot(&name);
> >
> > OK. not saying no to this (yet) but I have to admit that it is pretty
> > ugly that the callers of ovl_create_real() want to create a specific
> > stable name, which is could be passed in as const char *name
> > and yet we end up doing this weird dance here just to keep the name
> > from newdentry.
>
> There are three callers of ovl_create_real()
>
> ovl_lookup_or_create() does have a "const char *name".
> ovl_create_upper() has a stable dentry from which it can copy a QSTR
> ovl_create_temp() would need some sort of dance to keep hold of the
> temporary name that was allocated.
>
> If it weren't for ovl_create_temp() I would agree with you.
>
> Though we could have the three callers of ovl_start_creating_temp() pass a
> "char name[OVL_TEMPNAME_SIZE]" in, then ovl_create_temp() would have
> easy access.
> I could do that if you like.

Yes, considering that two of the callers are from the same function
(ovl_whiteout()) I think that would end up looking nicer.

Thanks,
Amir.

Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[Paul Moore]

On Wed, Feb 4, 2026 at 12:08 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
>
> This extends the region that the directory is locked for, and also
> performs a lookup.
> The lock extension is of no real consequence.
> The lookup uses simple_lookup() and so always succeeds.  Thus when
> d_make_persistent() is called the dentry will already be hashed.
> d_make_persistent() handles this case.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  security/selinux/selinuxfs.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

Unless I'm missing something, there is no reason why I couldn't take
just this patch into the SELinux tree once the merge window closes,
yes?

-- 
paul-moore.com

Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

On Sat, 21 Feb 2026, Paul Moore wrote:
> On Wed, Feb 4, 2026 at 12:08 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > Instead of explicitly locking the parent and performing a lookup in
> > selinux, use simple_start_creating(), and then use
> > simple_done_creating() to unlock.
> >
> > This extends the region that the directory is locked for, and also
> > performs a lookup.
> > The lock extension is of no real consequence.
> > The lookup uses simple_lookup() and so always succeeds.  Thus when
> > d_make_persistent() is called the dentry will already be hashed.
> > d_make_persistent() handles this case.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  security/selinux/selinuxfs.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> Unless I'm missing something, there is no reason why I couldn't take
> just this patch into the SELinux tree once the merge window closes,
> yes?

Yes - but ...

Once this series lands (hopefully soon - I will resend after -rc1 is
out) I have another batch which depends on the new start_creating etc
API being used everywhere.  So for Christian to be able to apply that,
he will need to pull in this patch from the SELinux tree.

So if you could apply just this patch to some branch and merge that
branch with your other work however works best for you, and make the
branch available, then I think Christian will be happy to merge that
with whatever vfs branch he includes my work in, and all should be good.

Thanks
NeilBrown


> 
> -- 
> paul-moore.com
> 

Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[Paul Moore]

On Sat, Feb 21, 2026 at 5:28 PM NeilBrown <neilb@ownmail.net> wrote:
> On Sat, 21 Feb 2026, Paul Moore wrote:
> > On Wed, Feb 4, 2026 at 12:08 AM NeilBrown <neilb@ownmail.net> wrote:
> > >
> > > From: NeilBrown <neil@brown.name>
> > >
> > > Instead of explicitly locking the parent and performing a lookup in
> > > selinux, use simple_start_creating(), and then use
> > > simple_done_creating() to unlock.
> > >
> > > This extends the region that the directory is locked for, and also
> > > performs a lookup.
> > > The lock extension is of no real consequence.
> > > The lookup uses simple_lookup() and so always succeeds.  Thus when
> > > d_make_persistent() is called the dentry will already be hashed.
> > > d_make_persistent() handles this case.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  security/selinux/selinuxfs.c | 15 +++++++--------
> > >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > Unless I'm missing something, there is no reason why I couldn't take
> > just this patch into the SELinux tree once the merge window closes,
> > yes?
>
> Yes - but ...
>
> Once this series lands (hopefully soon - I will resend after -rc1 is
> out) I have another batch which depends on the new start_creating etc
> API being used everywhere ...

Okay, thanks for letting me know.  I was curious about something like
that based on the cover letter, but the timing wasn't clear.

Acked-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com

Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

On Mon, 23 Feb 2026, Paul Moore wrote:
> On Sat, Feb 21, 2026 at 5:28 PM NeilBrown <neilb@ownmail.net> wrote:
> > On Sat, 21 Feb 2026, Paul Moore wrote:
> > > On Wed, Feb 4, 2026 at 12:08 AM NeilBrown <neilb@ownmail.net> wrote:
> > > >
> > > > From: NeilBrown <neil@brown.name>
> > > >
> > > > Instead of explicitly locking the parent and performing a lookup in
> > > > selinux, use simple_start_creating(), and then use
> > > > simple_done_creating() to unlock.
> > > >
> > > > This extends the region that the directory is locked for, and also
> > > > performs a lookup.
> > > > The lock extension is of no real consequence.
> > > > The lookup uses simple_lookup() and so always succeeds.  Thus when
> > > > d_make_persistent() is called the dentry will already be hashed.
> > > > d_make_persistent() handles this case.
> > > >
> > > > Signed-off-by: NeilBrown <neil@brown.name>
> > > > ---
> > > >  security/selinux/selinuxfs.c | 15 +++++++--------
> > > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > >
> > > Unless I'm missing something, there is no reason why I couldn't take
> > > just this patch into the SELinux tree once the merge window closes,
> > > yes?
> >
> > Yes - but ...
> >
> > Once this series lands (hopefully soon - I will resend after -rc1 is
> > out) I have another batch which depends on the new start_creating etc
> > API being used everywhere ...
> 
> Okay, thanks for letting me know.  I was curious about something like
> that based on the cover letter, but the timing wasn't clear.
> 
> Acked-by: Paul Moore <paul@paul-moore.com>

Thank!

NeilBrown