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

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <[email protected]>
> 
> 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 <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> 
> ---
> 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 <[email protected]>