Re: [PATCH v5 10/14] VFS/ovl/smb: introduce start_renaming_dentry()

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <[email protected]>
> 
> Several callers perform a rename on a dentry they already have, and only
> require lookup for the target name.  This includes smb/server and a few
> different places in overlayfs.
> 
> start_renaming_dentry() performs the required lookup and takes the
> required lock using lock_rename_child()
> 
> It is used in three places in overlayfs and in ksmbd_vfs_rename().
> 
> In the ksmbd case, the parent of the source is not important - the
> source must be renamed from wherever it is.  So start_renaming_dentry()
> allows rd->old_parent to be NULL and only checks it if it is non-NULL.
> On success rd->old_parent will be the parent of old_dentry with an extra
> reference taken.  Other start_renaming function also now take the extra
> reference and end_renaming() now drops this reference as well.
> 
> ovl_lookup_temp(), ovl_parent_lock(), and ovl_parent_unlock() are
> all removed as they are no longer needed.
> 
> OVL_TEMPNAME_SIZE and ovl_tempname() are now declared in overlayfs.h so
> that ovl_check_rename_whiteout() can access them.
> 
> ovl_copy_up_workdir() now always cleans up on error.
> 
> Reviewed-by: Namjae Jeon <[email protected]> (for ksmbd part)
> Reviewed-by: Amir Goldstein <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> ---
>  fs/namei.c               | 108 ++++++++++++++++++++++++++++++++++++---
>  fs/overlayfs/copy_up.c   |  54 +++++++++-----------
>  fs/overlayfs/dir.c       |  19 +------
>  fs/overlayfs/overlayfs.h |   8 +--
>  fs/overlayfs/super.c     |  22 ++++----
>  fs/overlayfs/util.c      |  11 ----
>  fs/smb/server/vfs.c      |  60 ++++------------------
>  include/linux/namei.h    |   2 +
>  8 files changed, 150 insertions(+), 134 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index bad6c9d540f9..4b740048df97 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3669,7 +3669,7 @@ EXPORT_SYMBOL(unlock_rename);
>  
>  /**
>   * __start_renaming - lookup and lock names for rename
> - * @rd:           rename data containing parent and flags, and
> + * @rd:           rename data containing parents and flags, and
>   *                for receiving found dentries
>   * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
>   *                LOOKUP_NO_SYMLINKS etc).
> @@ -3680,8 +3680,8 @@ EXPORT_SYMBOL(unlock_rename);
>   * rename.
>   *
>   * On success the found dentries are stored in @rd.old_dentry,
> - * @rd.new_dentry.  These references and the lock are dropped by
> - * end_renaming().
> + * @rd.new_dentry and an extra ref is taken on @rd.old_parent.
> + * These references and the lock are dropped by end_renaming().
>   *
>   * The passed in qstrs must have the hash calculated, and no permission
>   * checking is performed.
> @@ -3735,6 +3735,7 @@ __start_renaming(struct renamedata *rd, int lookup_flags,
>  
>  	rd->old_dentry = d1;
>  	rd->new_dentry = d2;
> +	dget(rd->old_parent);
>  	return 0;
>  
>  out_dput_d2:
> @@ -3748,7 +3749,7 @@ __start_renaming(struct renamedata *rd, int lookup_flags,
>  
>  /**
>   * start_renaming - lookup and lock names for rename with permission checking
> - * @rd:           rename data containing parent and flags, and
> + * @rd:           rename data containing parents and flags, and
>   *                for receiving found dentries
>   * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
>   *                LOOKUP_NO_SYMLINKS etc).
> @@ -3759,8 +3760,8 @@ __start_renaming(struct renamedata *rd, int lookup_flags,
>   * rename.
>   *
>   * On success the found dentries are stored in @rd.old_dentry,
> - * @rd.new_dentry.  These references and the lock are dropped by
> - * end_renaming().
> + * @rd.new_dentry.  Also the refcount on @rd->old_parent is increased.
> + * These references and the lock are dropped by end_renaming().
>   *
>   * The passed in qstrs need not have the hash calculated, and basic
>   * eXecute permission checking is performed against @rd.mnt_idmap.
> @@ -3782,11 +3783,106 @@ int start_renaming(struct renamedata *rd, int lookup_flags,
>  }
>  EXPORT_SYMBOL(start_renaming);
>  
> +static int
> +__start_renaming_dentry(struct renamedata *rd, int lookup_flags,
> +			struct dentry *old_dentry, struct qstr *new_last)
> +{
> +	struct dentry *trap;
> +	struct dentry *d2;
> +	int target_flags = LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
> +	int err;
> +
> +	if (rd->flags & RENAME_EXCHANGE)
> +		target_flags = 0;
> +	if (rd->flags & RENAME_NOREPLACE)
> +		target_flags |= LOOKUP_EXCL;
> +
> +	/* Already have the dentry - need to be sure to lock the correct parent */
> +	trap = lock_rename_child(old_dentry, rd->new_parent);
> +	if (IS_ERR(trap))
> +		return PTR_ERR(trap);
> +	if (d_unhashed(old_dentry) ||
> +	    (rd->old_parent && rd->old_parent != old_dentry->d_parent)) {
> +		/* dentry was removed, or moved and explicit parent requested */
> +		err = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	d2 = lookup_one_qstr_excl(new_last, rd->new_parent,
> +				  lookup_flags | target_flags);
> +	err = PTR_ERR(d2);
> +	if (IS_ERR(d2))
> +		goto out_unlock;
> +
> +	if (old_dentry == trap) {
> +		/* source is an ancestor of target */
> +		err = -EINVAL;
> +		goto out_dput_d2;
> +	}
> +
> +	if (d2 == trap) {
> +		/* target is an ancestor of source */
> +		if (rd->flags & RENAME_EXCHANGE)
> +			err = -EINVAL;
> +		else
> +			err = -ENOTEMPTY;
> +		goto out_dput_d2;
> +	}
> +
> +	rd->old_dentry = dget(old_dentry);
> +	rd->new_dentry = d2;
> +	rd->old_parent = dget(old_dentry->d_parent);
> +	return 0;
> +
> +out_dput_d2:
> +	dput(d2);
> +out_unlock:
> +	unlock_rename(old_dentry->d_parent, rd->new_parent);
> +	return err;
> +}
> +
> +/**
> + * start_renaming_dentry - lookup and lock name for rename with permission checking
> + * @rd:           rename data containing parents and flags, and
> + *                for receiving found dentries
> + * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
> + *                LOOKUP_NO_SYMLINKS etc).
> + * @old_dentry:   dentry of name to move
> + * @new_last:     name of target in @rd.new_parent
> + *
> + * Look up target name and ensure locks are in place for
> + * rename.
> + *
> + * On success the found dentry is stored in @rd.new_dentry and
> + * @rd.old_parent is confirmed to be the parent of @old_dentry.  If it
> + * was originally %NULL, it is set.  In either case a reference is taken
> + * so that end_renaming() can have a stable reference to unlock.
> + *
> + * References and the lock can be dropped with end_renaming()
> + *
> + * The passed in qstr need not have the hash calculated, and basic
> + * eXecute permission checking is performed against @rd.mnt_idmap.
> + *
> + * Returns: zero or an error.
> + */
> +int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
> +			  struct dentry *old_dentry, struct qstr *new_last)
> +{
> +	int err;
> +
> +	err = lookup_one_common(rd->mnt_idmap, new_last, rd->new_parent);
> +	if (err)
> +		return err;
> +	return __start_renaming_dentry(rd, lookup_flags, old_dentry, new_last);
> +}
> +EXPORT_SYMBOL(start_renaming_dentry);
> +
>  void end_renaming(struct renamedata *rd)
>  {
>  	unlock_rename(rd->old_parent, rd->new_parent);
>  	dput(rd->old_dentry);
>  	dput(rd->new_dentry);
> +	dput(rd->old_parent);
>  }
>  EXPORT_SYMBOL(end_renaming);
>  
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index e2bdac4317e7..9911a346b477 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -523,8 +523,8 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
>  {
>  	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>  	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> -	struct dentry *index = NULL;
>  	struct dentry *temp = NULL;
> +	struct renamedata rd = {};
>  	struct qstr name = { };
>  	int err;
>  
> @@ -556,17 +556,15 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
>  	if (err)
>  		goto out;
>  
> -	err = ovl_parent_lock(indexdir, temp);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = indexdir;
> +	rd.new_parent = indexdir;
> +	err = start_renaming_dentry(&rd, 0, temp, &name);
>  	if (err)
>  		goto out;
> -	index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> -	if (IS_ERR(index)) {
> -		err = PTR_ERR(index);
> -	} else {
> -		err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> -		dput(index);
> -	}
> -	ovl_parent_unlock(indexdir);
> +
> +	err = ovl_do_rename_rd(&rd);
> +	end_renaming(&rd);
>  out:
>  	if (err)
>  		ovl_cleanup(ofs, indexdir, temp);
> @@ -763,7 +761,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>  	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
>  	struct inode *inode;
>  	struct path path = { .mnt = ovl_upper_mnt(ofs) };
> -	struct dentry *temp, *upper, *trap;
> +	struct renamedata rd = {};
> +	struct dentry *temp;
>  	struct ovl_cu_creds cc;
>  	int err;
>  	struct ovl_cattr cattr = {
> @@ -807,29 +806,24 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>  	 * ovl_copy_up_data(), so lock workdir and destdir and make sure that
>  	 * temp wasn't moved before copy up completion or cleanup.
>  	 */
> -	trap = lock_rename(c->workdir, c->destdir);
> -	if (trap || temp->d_parent != c->workdir) {
> -		/* temp or workdir moved underneath us? abort without cleanup */
> -		dput(temp);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = c->workdir;
> +	rd.new_parent = c->destdir;
> +	rd.flags = 0;
> +	err = start_renaming_dentry(&rd, 0, temp,
> +				    &QSTR_LEN(c->destname.name, c->destname.len));
> +	if (err) {
> +		/* temp or workdir moved underneath us? map to -EIO */
>  		err = -EIO;
> -		if (!IS_ERR(trap))
> -			unlock_rename(c->workdir, c->destdir);
> -		goto out;
>  	}
> -
> -	err = ovl_copy_up_metadata(c, temp);
>  	if (err)
> -		goto cleanup;
> +		goto cleanup_unlocked;
>  
> -	upper = ovl_lookup_upper(ofs, c->destname.name, c->destdir,
> -				 c->destname.len);
> -	err = PTR_ERR(upper);
> -	if (IS_ERR(upper))
> -		goto cleanup;
> +	err = ovl_copy_up_metadata(c, temp);
> +	if (!err)
> +		err = ovl_do_rename_rd(&rd);
> +	end_renaming(&rd);
>  
> -	err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> -	unlock_rename(c->workdir, c->destdir);
> -	dput(upper);
>  	if (err)
>  		goto cleanup_unlocked;
>  
> @@ -850,8 +844,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>  
>  	return err;
>  
> -cleanup:
> -	unlock_rename(c->workdir, c->destdir);
>  cleanup_unlocked:
>  	ovl_cleanup(ofs, c->workdir, temp);
>  	dput(temp);
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index cf6fc48459f3..6b2f88edb497 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -57,8 +57,7 @@ int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
>  	return 0;
>  }
>  
> -#define OVL_TEMPNAME_SIZE 20
> -static void ovl_tempname(char name[OVL_TEMPNAME_SIZE])
> +void ovl_tempname(char name[OVL_TEMPNAME_SIZE])
>  {
>  	static atomic_t temp_id = ATOMIC_INIT(0);
>  
> @@ -66,22 +65,6 @@ static void ovl_tempname(char name[OVL_TEMPNAME_SIZE])
>  	snprintf(name, OVL_TEMPNAME_SIZE, "#%x", atomic_inc_return(&temp_id));
>  }
>  
> -struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
> -{
> -	struct dentry *temp;
> -	char name[OVL_TEMPNAME_SIZE];
> -
> -	ovl_tempname(name);
> -	temp = ovl_lookup_upper(ofs, name, workdir, strlen(name));
> -	if (!IS_ERR(temp) && temp->d_inode) {
> -		pr_err("workdir/%s already exists\n", name);
> -		dput(temp);
> -		temp = ERR_PTR(-EIO);
> -	}
> -
> -	return temp;
> -}
> -
>  static struct dentry *ovl_start_creating_temp(struct ovl_fs *ofs,
>  					      struct dentry *workdir)
>  {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 3cc85a893b5c..746bc4ad7b37 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -447,11 +447,6 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
>  }
>  
>  /* util.c */
> -int ovl_parent_lock(struct dentry *parent, struct dentry *child);
> -static inline void ovl_parent_unlock(struct dentry *parent)
> -{
> -	inode_unlock(parent->d_inode);
> -}
>  int ovl_get_write_access(struct dentry *dentry);
>  void ovl_put_write_access(struct dentry *dentry);
>  void ovl_start_write(struct dentry *dentry);
> @@ -888,7 +883,8 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
>  			       struct dentry *parent, struct dentry *newdentry,
>  			       struct ovl_cattr *attr);
>  int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
> -struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
> +#define OVL_TEMPNAME_SIZE 20
> +void ovl_tempname(char name[OVL_TEMPNAME_SIZE]);
>  struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
>  			       struct ovl_cattr *attr);
>  
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 6e0816c1147a..a721ef2b90e8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -566,9 +566,10 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>  {
>  	struct dentry *workdir = ofs->workdir;
>  	struct dentry *temp;
> -	struct dentry *dest;
>  	struct dentry *whiteout;
>  	struct name_snapshot name;
> +	struct renamedata rd = {};
> +	char name2[OVL_TEMPNAME_SIZE];
>  	int err;
>  
>  	temp = ovl_create_temp(ofs, workdir, OVL_CATTR(S_IFREG | 0));
> @@ -576,23 +577,21 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>  	if (IS_ERR(temp))
>  		return err;
>  
> -	err = ovl_parent_lock(workdir, temp);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = workdir;
> +	rd.new_parent = workdir;
> +	rd.flags = RENAME_WHITEOUT;
> +	ovl_tempname(name2);
> +	err = start_renaming_dentry(&rd, 0, temp, &QSTR(name2));
>  	if (err) {
>  		dput(temp);
>  		return err;
>  	}
> -	dest = ovl_lookup_temp(ofs, workdir);
> -	err = PTR_ERR(dest);
> -	if (IS_ERR(dest)) {
> -		dput(temp);
> -		ovl_parent_unlock(workdir);
> -		return err;
> -	}
>  
>  	/* Name is inline and stable - using snapshot as a copy helper */
>  	take_dentry_name_snapshot(&name, temp);
> -	err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
> -	ovl_parent_unlock(workdir);
> +	err = ovl_do_rename_rd(&rd);
> +	end_renaming(&rd);
>  	if (err) {
>  		if (err == -EINVAL)
>  			err = 0;
> @@ -616,7 +615,6 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>  	ovl_cleanup(ofs, workdir, temp);
>  	release_dentry_name_snapshot(&name);
>  	dput(temp);
> -	dput(dest);
>  
>  	return err;
>  }
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 2da1c035f716..fffc22859211 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1548,14 +1548,3 @@ void ovl_copyattr(struct inode *inode)
>  	i_size_write(inode, i_size_read(realinode));
>  	spin_unlock(&inode->i_lock);
>  }
> -
> -int ovl_parent_lock(struct dentry *parent, struct dentry *child)
> -{
> -	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> -	if (!child ||
> -	    (!d_unhashed(child) && child->d_parent == parent))
> -		return 0;
> -
> -	inode_unlock(parent->d_inode);
> -	return -EINVAL;
> -}
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index 7c4ddc43ab39..f54b5b0aaba2 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -663,7 +663,6 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
>  int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  		     char *newname, int flags)
>  {
> -	struct dentry *old_parent, *new_dentry, *trap;
>  	struct dentry *old_child = old_path->dentry;
>  	struct path new_path;
>  	struct qstr new_last;
> @@ -673,7 +672,6 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  	struct ksmbd_file *parent_fp;
>  	int new_type;
>  	int err, lookup_flags = LOOKUP_NO_SYMLINKS;
> -	int target_lookup_flags = LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
>  
>  	if (ksmbd_override_fsids(work))
>  		return -ENOMEM;
> @@ -684,14 +682,6 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  		goto revert_fsids;
>  	}
>  
> -	/*
> -	 * explicitly handle file overwrite case, for compatibility with
> -	 * filesystems that may not support rename flags (e.g: fuse)
> -	 */
> -	if (flags & RENAME_NOREPLACE)
> -		target_lookup_flags |= LOOKUP_EXCL;
> -	flags &= ~(RENAME_NOREPLACE);
> -
>  retry:
>  	err = vfs_path_parent_lookup(to, lookup_flags | LOOKUP_BENEATH,
>  				     &new_path, &new_last, &new_type,
> @@ -708,17 +698,14 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  	if (err)
>  		goto out2;
>  
> -	trap = lock_rename_child(old_child, new_path.dentry);
> -	if (IS_ERR(trap)) {
> -		err = PTR_ERR(trap);
> +	rd.mnt_idmap		= mnt_idmap(old_path->mnt);
> +	rd.old_parent		= NULL;
> +	rd.new_parent		= new_path.dentry;
> +	rd.flags		= flags;
> +	rd.delegated_inode	= NULL,
> +	err = start_renaming_dentry(&rd, lookup_flags, old_child, &new_last);
> +	if (err)
>  		goto out_drop_write;
> -	}
> -
> -	old_parent = dget(old_child->d_parent);
> -	if (d_unhashed(old_child)) {
> -		err = -EINVAL;
> -		goto out3;
> -	}
>  
>  	parent_fp = ksmbd_lookup_fd_inode(old_child->d_parent);
>  	if (parent_fp) {
> @@ -731,44 +718,17 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  		ksmbd_fd_put(work, parent_fp);
>  	}
>  
> -	new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
> -					  lookup_flags | target_lookup_flags);
> -	if (IS_ERR(new_dentry)) {
> -		err = PTR_ERR(new_dentry);
> -		goto out3;
> -	}
> -
> -	if (d_is_symlink(new_dentry)) {
> +	if (d_is_symlink(rd.new_dentry)) {
>  		err = -EACCES;
> -		goto out4;
> -	}
> -
> -	if (old_child == trap) {
> -		err = -EINVAL;
> -		goto out4;
> -	}
> -
> -	if (new_dentry == trap) {
> -		err = -ENOTEMPTY;
> -		goto out4;
> +		goto out3;
>  	}
>  
> -	rd.mnt_idmap		= mnt_idmap(old_path->mnt),
> -	rd.old_parent		= old_parent,
> -	rd.old_dentry		= old_child,
> -	rd.new_parent		= new_path.dentry,
> -	rd.new_dentry		= new_dentry,
> -	rd.flags		= flags,
> -	rd.delegated_inode	= NULL,
>  	err = vfs_rename(&rd);
>  	if (err)
>  		ksmbd_debug(VFS, "vfs_rename failed err %d\n", err);
>  
> -out4:
> -	dput(new_dentry);
>  out3:
> -	dput(old_parent);
> -	unlock_rename(old_parent, new_path.dentry);
> +	end_renaming(&rd);
>  out_drop_write:
>  	mnt_drop_write(old_path->mnt);
>  out2:
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 7fdd9fdcbd2b..c47713e9867c 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -159,6 +159,8 @@ 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,
> +			  struct dentry *old_dentry, struct qstr *new_last);
>  void end_renaming(struct renamedata *rd);
>  
>  /**

Reviewed-by: Jeff Layton <[email protected]>