Re: [PATCH v5 07/14] VFS: introduce start_removing_dentry()

Jeff Layton
2025-11-12T14:58:16Z

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> 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_removing() can be used to finish the operation.
>
> This is used in cachefiles, overlayfs, smb/server, and apparmor.
>
> There will be other users including ecryptfs.
>
> As start_removing_dentry() takes an extra reference to the dentry (to be
> put by end_removing()), there is no need to explicitly take an extra
> reference to stop d_delete() from using dentry_unlink_inode() to negate
> the dentry - as in cachefiles_delete_object(), and ksmbd_vfs_unlink().
>
> cachefiles_bury_object() now gets an extra ref to the victim, which is
> drops. As it includes the needed end_removing() calls, the caller
> doesn't need them.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
>
> ---
> Changes since v4
> Callers of cachefiles_bury_object() were incorrectly calling
> end_removing() after that call. The dput() was needed, the unlock
> wasn't. The dput() has been effectively moved into
> cachefiles_bury_object() by removing a dget() which is now not needed.
> ---
> fs/cachefiles/interface.c | 11 +++++++----
> fs/cachefiles/namei.c | 30 ++++++++++++++----------------
> fs/cachefiles/volume.c | 9 ++++++---
> fs/namei.c | 33 +++++++++++++++++++++++++++++++++
> 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, 78 insertions(+), 60 deletions(-)
>
> diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
> index 3e63cfe15874..a08250d244ea 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,13 @@ 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];
> + struct dentry *obj;
>
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - cachefiles_bury_object(volume->cache, object, fan,
> - old_file->f_path.dentry,
> - FSCACHE_OBJECT_INVALIDATED);
> + 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);
> }
> fput(old_file);
> }
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index c7f0c6ab9b88..0104ac00485d 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -261,6 +261,7 @@ static int cachefiles_unlink(struct cachefiles_cache *cache,
> * - Directory backed objects are stuffed into the graveyard for userspace to
> * delete
> * On entry dir must be locked. It will be unlocked on exit.
> + * On entry there must be at least 2 refs on rep, one will be dropped on exit.
> */
> int cachefiles_bury_object(struct cachefiles_cache *cache,
> struct cachefiles_object *object,
> @@ -275,12 +276,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>
> _enter(",'%pd','%pd'", dir, rep);
>
> - /* end_removing() will dput() @rep but we need to keep
> - * a ref, so take one now. This also stops the dentry
> - * being negated when unlinked which we need.
> - */
> - dget(rep);
> -
> if (rep->d_parent != dir) {
> end_removing(rep);
> _leave(" = -ESTALE");
> @@ -425,13 +420,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_removing(dentry);
> return ret;
> }
>
> @@ -644,9 +638,13 @@ 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 = PTR_ERR(de);
> + else
> + ret = cachefiles_bury_object(volume->cache, object,
> + fan, de,
> + FSCACHE_OBJECT_IS_WEIRD);
> dput(dentry);
> if (ret < 0)
> return false;
> diff --git a/fs/cachefiles/volume.c b/fs/cachefiles/volume.c
> index 781aac4ef274..90ba926f488e 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,11 @@ 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);
> cachefiles_put_directory(volume->dentry);
> cond_resched();
> goto retry;
> diff --git a/fs/namei.c b/fs/namei.c
> index da01b828ede6..729b42fb143b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3323,6 +3323,39 @@ 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 and positive, a reference is taken and
> + * returned. If not an error is returned.
> + *
> + * end_removing() 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);
> + }
> + if (d_is_negative(child)) {
> + inode_unlock(parent->d_inode);
> + return ERR_PTR(-ENOENT);
> + }
> + 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 20682afdbd20..6d1d0e94e287 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_removing(wdentry);
>
> return 0;
> }
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 1e9792cc557b..77ecc39fc33a 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1242,11 +1242,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_removing(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 891ed2dc2b73..7c4ddc43ab39 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -49,24 +49,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)
> @@ -1084,18 +1066,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_removing(dentry);
> if (err)
> ksmbd_debug(VFS, "failed to delete, err %d\n", err);
> out:

I guess this answers my earlier question.

> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 0441f5921f87..d089e4e8fdd0 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -95,6 +95,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);
>
> /**
> * end_creating - finish action started with start_creating
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 391a586d0557..9d08d103f142 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -355,17 +355,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_removing(dentry);
> simple_release_fs(&aafs_mnt, &aafs_count);
> }
>

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