start_removing_dentry() is similar to start_removing() but instead of
providing a name for lookup, the target dentry is given.
start_removing_dentry() checks that the dentry is still hashed and in
the parent, and if so it locks and increases the refcount so that
end_dirop() can be used to finish the operation.
This is used in cachefiles, overlayfs, smb/server and apparmor.
There will be other users including ecryptfs.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/cachefiles/interface.c | 14 +++++++++-----
fs/cachefiles/namei.c | 22 ++++++++++++----------
fs/cachefiles/volume.c | 10 +++++++---
fs/namei.c | 29 +++++++++++++++++++++++++++++
fs/overlayfs/dir.c | 10 ++++------
fs/overlayfs/readdir.c | 8 ++++----
fs/smb/server/vfs.c | 27 ++++-----------------------
include/linux/namei.h | 2 ++
security/apparmor/apparmorfs.c | 8 ++++----
9 files changed, 75 insertions(+), 55 deletions(-)
diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index 3e63cfe15874..763d7d55b1f9 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -9,6 +9,7 @@
#include <linux/mount.h>
#include <linux/xattr.h>
#include <linux/file.h>
+#include <linux/namei.h>
#include <linux/falloc.h>
#include <trace/events/fscache.h>
#include "internal.h"
@@ -428,11 +429,14 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
if (!old_tmpfile) {
struct cachefiles_volume *volume = object->volume;
struct dentry *fan = volume->fanout[(u8)cookie->key_hash];
-
- inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
- cachefiles_bury_object(volume->cache, object, fan,
- old_file->f_path.dentry,
- FSCACHE_OBJECT_INVALIDATED);
+ struct dentry *obj;
+
+ obj = start_removing_dentry(fan, old_file->f_path.dentry);
+ if (!IS_ERR(obj))
+ cachefiles_bury_object(volume->cache, object,
+ fan, obj,
+ FSCACHE_OBJECT_INVALIDATED);
+ end_dirop(obj);
}
fput(old_file);
}
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index ddced50afb66..cc6dccd606ea 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -424,13 +424,12 @@ int cachefiles_delete_object(struct cachefiles_object *object,
_enter(",OBJ%x{%pD}", object->debug_id, object->file);
- /* Stop the dentry being negated if it's only pinned by a file struct. */
- dget(dentry);
-
- inode_lock_nested(d_backing_inode(fan), I_MUTEX_PARENT);
- ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
- inode_unlock(d_backing_inode(fan));
- dput(dentry);
+ dentry = start_removing_dentry(fan, dentry);
+ if (IS_ERR(dentry))
+ ret = PTR_ERR(dentry);
+ else
+ ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
+ end_dirop(dentry);
return ret;
}
@@ -643,9 +642,12 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
if (!d_is_reg(dentry)) {
pr_err("%pd is not a file\n", dentry);
- inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
- ret = cachefiles_bury_object(volume->cache, object, fan, dentry,
- FSCACHE_OBJECT_IS_WEIRD);
+ struct dentry *de = start_removing_dentry(fan, dentry);
+ if (!IS_ERR(de))
+ ret = cachefiles_bury_object(volume->cache, object,
+ fan, de,
+ FSCACHE_OBJECT_IS_WEIRD);
+ end_dirop(de);
dput(dentry);
if (ret < 0)
return false;
diff --git a/fs/cachefiles/volume.c b/fs/cachefiles/volume.c
index 781aac4ef274..8c29f3db3fae 100644
--- a/fs/cachefiles/volume.c
+++ b/fs/cachefiles/volume.c
@@ -7,6 +7,7 @@
#include <linux/fs.h>
#include <linux/slab.h>
+#include <linux/namei.h>
#include "internal.h"
#include <trace/events/fscache.h>
@@ -58,9 +59,12 @@ void cachefiles_acquire_volume(struct fscache_volume *vcookie)
if (ret < 0) {
if (ret != -ESTALE)
goto error_dir;
- inode_lock_nested(d_inode(cache->store), I_MUTEX_PARENT);
- cachefiles_bury_object(cache, NULL, cache->store, vdentry,
- FSCACHE_VOLUME_IS_WEIRD);
+ vdentry = start_removing_dentry(cache->store, vdentry);
+ if (!IS_ERR(vdentry))
+ cachefiles_bury_object(cache, NULL, cache->store,
+ vdentry,
+ FSCACHE_VOLUME_IS_WEIRD);
+ end_dirop(vdentry);
cachefiles_put_directory(volume->dentry);
cond_resched();
goto retry;
diff --git a/fs/namei.c b/fs/namei.c
index 34895487045e..af56bc39c4d5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3261,6 +3261,35 @@ struct dentry *start_removing_noperm(struct dentry *parent,
}
EXPORT_SYMBOL(start_removing_noperm);
+/**
+ * start_removing_dentry - prepare to remove a given dentry
+ * @parent - directory from which dentry should be removed
+ * @child - the dentry to be removed
+ *
+ * A lock is taken to protect the dentry again other dirops and
+ * the validity of the dentry is checked: correct parent and still hashed.
+ *
+ * If the dentry is valid a reference is taken and returned. If not
+ * an error is returned.
+ *
+ * end_dirop() should be called when removal is complete, or aborted.
+ *
+ * Returns: the valid dentry, or an error.
+ */
+struct dentry *start_removing_dentry(struct dentry *parent,
+ struct dentry *child)
+{
+ inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
+ if (unlikely(IS_DEADDIR(parent->d_inode) ||
+ child->d_parent != parent ||
+ d_unhashed(child))) {
+ inode_unlock(parent->d_inode);
+ return ERR_PTR(-EINVAL);
+ }
+ return dget(child);
+}
+EXPORT_SYMBOL(start_removing_dentry);
+
#ifdef CONFIG_UNIX98_PTYS
int path_pts(struct path *path)
{
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 70b8687dc45e..b8f0d409e841 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -47,14 +47,12 @@ static int ovl_cleanup_locked(struct ovl_fs *ofs, struct inode *wdir,
int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
struct dentry *wdentry)
{
- int err;
-
- err = ovl_parent_lock(workdir, wdentry);
- if (err)
- return err;
+ wdentry = start_removing_dentry(workdir, wdentry);
+ if (IS_ERR(wdentry))
+ return PTR_ERR(wdentry);
ovl_cleanup_locked(ofs, workdir->d_inode, wdentry);
- ovl_parent_unlock(workdir);
+ end_dirop(wdentry);
return 0;
}
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index b65cdfce31ce..20348be4b98f 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1158,11 +1158,11 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
if (!d_is_dir(dentry) || level > 1)
return ovl_cleanup(ofs, parent, dentry);
- err = ovl_parent_lock(parent, dentry);
- if (err)
- return err;
+ dentry = start_removing_dentry(parent, dentry);
+ if (IS_ERR(dentry))
+ return PTR_ERR(dentry);
err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
- ovl_parent_unlock(parent);
+ end_dirop(dentry);
if (err) {
struct path path = { .mnt = mnt, .dentry = dentry };
diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index 07739055ac9f..04030a204d5a 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -48,24 +48,6 @@ static void ksmbd_vfs_inherit_owner(struct ksmbd_work *work,
i_uid_write(inode, i_uid_read(parent_inode));
}
-/**
- * ksmbd_vfs_lock_parent() - lock parent dentry if it is stable
- * @parent: parent dentry
- * @child: child dentry
- *
- * Returns: %0 on success, %-ENOENT if the parent dentry is not stable
- */
-int ksmbd_vfs_lock_parent(struct dentry *parent, struct dentry *child)
-{
- inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
- if (child->d_parent != parent) {
- inode_unlock(d_inode(parent));
- return -ENOENT;
- }
-
- return 0;
-}
-
static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
char *pathname, unsigned int flags,
struct path *path, bool do_lock)
@@ -1083,18 +1065,17 @@ int ksmbd_vfs_unlink(struct file *filp)
return err;
dir = dget_parent(dentry);
- err = ksmbd_vfs_lock_parent(dir, dentry);
- if (err)
+ dentry = start_removing_dentry(dir, dentry);
+ err = PTR_ERR(dentry);
+ if (IS_ERR(dentry))
goto out;
- dget(dentry);
if (S_ISDIR(d_inode(dentry)->i_mode))
err = vfs_rmdir(idmap, d_inode(dir), dentry);
else
err = vfs_unlink(idmap, d_inode(dir), dentry, NULL);
- dput(dentry);
- inode_unlock(d_inode(dir));
+ end_dirop(dentry);
if (err)
ksmbd_debug(VFS, "failed to delete, err %d\n", err);
out:
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d765a23c87e4..b89be0ac5e87 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -87,6 +87,8 @@ struct dentry *start_removing(struct mnt_idmap *idmap, struct dentry *parent,
struct qstr *name);
struct dentry *start_creating_noperm(struct dentry *parent, struct qstr *name);
struct dentry *start_removing_noperm(struct dentry *parent, struct qstr *name);
+struct dentry *start_removing_dentry(struct dentry *parent,
+ struct dentry *child);
void end_dirop(struct dentry *de);
void end_dirop_mkdir(struct dentry *de, struct dentry *parent);
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 13260352198f..f33fc19c99a7 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -343,17 +343,17 @@ static void aafs_remove(struct dentry *dentry)
if (!dentry || IS_ERR(dentry))
return;
+ /* ->d_parent is stable as rename is not supported */
dir = d_inode(dentry->d_parent);
- inode_lock(dir);
- if (simple_positive(dentry)) {
+ dentry = start_removing_dentry(dentry->d_parent, dentry);
+ if (!IS_ERR(dentry) && simple_positive(dentry)) {
if (d_is_dir(dentry))
simple_rmdir(dir, dentry);
else
simple_unlink(dir, dentry);
d_delete(dentry);
- dput(dentry);
}
- inode_unlock(dir);
+ end_dirop(dentry);
simple_release_fs(&aafs_mnt, &aafs_count);
}
--
2.50.0.107.gf914562f5916.dirtyOn Fri, Aug 22, 2025 at 2:11 AM NeilBrown <neil@brown.name> wrote:
>
> start_removing_dentry() is similar to start_removing() but instead of
> providing a name for lookup, the target dentry is given.
>
> start_removing_dentry() checks that the dentry is still hashed and in
> the parent, and if so it locks and increases the refcount so that
> end_dirop() can be used to finish the operation.
>
> This is used in cachefiles, overlayfs, smb/server and apparmor.
>
> There will be other users including ecryptfs.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/cachefiles/interface.c | 14 +++++++++-----
> fs/cachefiles/namei.c | 22 ++++++++++++----------
> fs/cachefiles/volume.c | 10 +++++++---
> fs/namei.c | 29 +++++++++++++++++++++++++++++
> fs/overlayfs/dir.c | 10 ++++------
> fs/overlayfs/readdir.c | 8 ++++----
> fs/smb/server/vfs.c | 27 ++++-----------------------
> include/linux/namei.h | 2 ++
> security/apparmor/apparmorfs.c | 8 ++++----
> 9 files changed, 75 insertions(+), 55 deletions(-)
>
> diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
> index 3e63cfe15874..763d7d55b1f9 100644
> --- a/fs/cachefiles/interface.c
> +++ b/fs/cachefiles/interface.c
> @@ -9,6 +9,7 @@
> #include <linux/mount.h>
> #include <linux/xattr.h>
> #include <linux/file.h>
> +#include <linux/namei.h>
> #include <linux/falloc.h>
> #include <trace/events/fscache.h>
> #include "internal.h"
> @@ -428,11 +429,14 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
> if (!old_tmpfile) {
> struct cachefiles_volume *volume = object->volume;
> struct dentry *fan = volume->fanout[(u8)cookie->key_hash];
> -
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - cachefiles_bury_object(volume->cache, object, fan,
> - old_file->f_path.dentry,
> - FSCACHE_OBJECT_INVALIDATED);
> + struct dentry *obj;
> +
> + obj = start_removing_dentry(fan, old_file->f_path.dentry);
> + if (!IS_ERR(obj))
> + cachefiles_bury_object(volume->cache, object,
> + fan, obj,
> + FSCACHE_OBJECT_INVALIDATED);
> + end_dirop(obj);
> }
> fput(old_file);
> }
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ddced50afb66..cc6dccd606ea 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -424,13 +424,12 @@ int cachefiles_delete_object(struct cachefiles_object *object,
>
> _enter(",OBJ%x{%pD}", object->debug_id, object->file);
>
> - /* Stop the dentry being negated if it's only pinned by a file struct. */
> - dget(dentry);
> -
> - inode_lock_nested(d_backing_inode(fan), I_MUTEX_PARENT);
> - ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
> - inode_unlock(d_backing_inode(fan));
> - dput(dentry);
> + dentry = start_removing_dentry(fan, dentry);
> + if (IS_ERR(dentry))
> + ret = PTR_ERR(dentry);
> + else
> + ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
> + end_dirop(dentry);
> return ret;
> }
>
> @@ -643,9 +642,12 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
>
> if (!d_is_reg(dentry)) {
> pr_err("%pd is not a file\n", dentry);
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - ret = cachefiles_bury_object(volume->cache, object, fan, dentry,
> - FSCACHE_OBJECT_IS_WEIRD);
> + struct dentry *de = start_removing_dentry(fan, dentry);
> + if (!IS_ERR(de))
> + ret = cachefiles_bury_object(volume->cache, object,
> + fan, de,
> + FSCACHE_OBJECT_IS_WEIRD);
> + end_dirop(de);
> dput(dentry);
> if (ret < 0)
> return false;
> diff --git a/fs/cachefiles/volume.c b/fs/cachefiles/volume.c
> index 781aac4ef274..8c29f3db3fae 100644
> --- a/fs/cachefiles/volume.c
> +++ b/fs/cachefiles/volume.c
> @@ -7,6 +7,7 @@
>
> #include <linux/fs.h>
> #include <linux/slab.h>
> +#include <linux/namei.h>
> #include "internal.h"
> #include <trace/events/fscache.h>
>
> @@ -58,9 +59,12 @@ void cachefiles_acquire_volume(struct fscache_volume *vcookie)
> if (ret < 0) {
> if (ret != -ESTALE)
> goto error_dir;
> - inode_lock_nested(d_inode(cache->store), I_MUTEX_PARENT);
> - cachefiles_bury_object(cache, NULL, cache->store, vdentry,
> - FSCACHE_VOLUME_IS_WEIRD);
> + vdentry = start_removing_dentry(cache->store, vdentry);
> + if (!IS_ERR(vdentry))
> + cachefiles_bury_object(cache, NULL, cache->store,
> + vdentry,
> + FSCACHE_VOLUME_IS_WEIRD);
> + end_dirop(vdentry);
> cachefiles_put_directory(volume->dentry);
> cond_resched();
> goto retry;
> diff --git a/fs/namei.c b/fs/namei.c
> index 34895487045e..af56bc39c4d5 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3261,6 +3261,35 @@ struct dentry *start_removing_noperm(struct dentry *parent,
> }
> EXPORT_SYMBOL(start_removing_noperm);
>
> +/**
> + * start_removing_dentry - prepare to remove a given dentry
> + * @parent - directory from which dentry should be removed
> + * @child - the dentry to be removed
> + *
> + * A lock is taken to protect the dentry again other dirops and
> + * the validity of the dentry is checked: correct parent and still hashed.
> + *
> + * If the dentry is valid a reference is taken and returned. If not
> + * an error is returned.
> + *
> + * end_dirop() should be called when removal is complete, or aborted.
> + *
> + * Returns: the valid dentry, or an error.
> + */
> +struct dentry *start_removing_dentry(struct dentry *parent,
> + struct dentry *child)
> +{
> + inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> + if (unlikely(IS_DEADDIR(parent->d_inode) ||
> + child->d_parent != parent ||
> + d_unhashed(child))) {
> + inode_unlock(parent->d_inode);
> + return ERR_PTR(-EINVAL);
> + }
> + return dget(child);
> +}
> +EXPORT_SYMBOL(start_removing_dentry);
> +
> #ifdef CONFIG_UNIX98_PTYS
> int path_pts(struct path *path)
> {
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 70b8687dc45e..b8f0d409e841 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -47,14 +47,12 @@ static int ovl_cleanup_locked(struct ovl_fs *ofs, struct inode *wdir,
> int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
> struct dentry *wdentry)
> {
> - int err;
> -
> - err = ovl_parent_lock(workdir, wdentry);
> - if (err)
> - return err;
> + wdentry = start_removing_dentry(workdir, wdentry);
> + if (IS_ERR(wdentry))
> + return PTR_ERR(wdentry);
>
> ovl_cleanup_locked(ofs, workdir->d_inode, wdentry);
> - ovl_parent_unlock(workdir);
> + end_dirop(wdentry);
>
> return 0;
> }
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index b65cdfce31ce..20348be4b98f 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1158,11 +1158,11 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
> if (!d_is_dir(dentry) || level > 1)
> return ovl_cleanup(ofs, parent, dentry);
>
> - err = ovl_parent_lock(parent, dentry);
> - if (err)
> - return err;
> + dentry = start_removing_dentry(parent, dentry);
> + if (IS_ERR(dentry))
> + return PTR_ERR(dentry);
> err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
> - ovl_parent_unlock(parent);
> + end_dirop(dentry);
> if (err) {
> struct path path = { .mnt = mnt, .dentry = dentry };
>
I'm sorry I keep nagging about this semantic point, but when I request
that code remains "balanced", I mean "balanced to a human eye".
If we are going to delegate reviews to LLM, then we can feed LLM
the documentation that says which start_ pairs with which end_
and go on to our retirement plans, but as long as I need to review
code, I need a human readable signal about what pairs with what.
Therefore, IMO it is better to have semantic wrappers
end_removing(), end_creating(), than having to rely on humans
to understand that start_removing_dentry() pairs with end_dirop().
Alternatively, use start_dirop_remove*, start_dirop_create*,
so it is clear that they can all match end_dirop() (can they???).
And repeating my request again: I will insist for overlayfs patches
but I think it is a good practice for all of your patches -
Please keep the code balance by introducing start_ together
with end_ in the same patch, so that it is clear from context of
a single patch review, that the callers were converted correctly.
Otherwise, the only way to verify that is to review the end result
and that is not the idea of a patch series.
Thanks,
Amir.
© 2016 - 2026 Red Hat, Inc.