Re: [PATCH v5 11/14] Add start_renaming_two_dentries()

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <[email protected]>
> 
> A few callers want to lock for a rename and already have both dentries.
> Also debugfs does want to perform a lookup but doesn't want permission
> checking, so start_renaming_dentry() cannot be used.
> 
> This patch introduces start_renaming_two_dentries() which is given both
> dentries.  debugfs performs one lookup itself.  As it will only continue
> with a negative dentry and as those cannot be renamed or unlinked, it is
> safe to do the lookup before getting the rename locks.
> 
> overlayfs uses start_renaming_two_dentries() in three places and  selinux
> uses it twice in sel_make_policy_nodes().
> 
> In sel_make_policy_nodes() we now lock for rename twice instead of just
> once so the combined operation is no longer atomic w.r.t the parent
> directory locks.  As selinux_state.policy_mutex is held across the whole
> operation this does open up any interesting races.
> 
> Reviewed-by: Amir Goldstein <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> 
> ---
> changes since v3:
>  added missing assignment to rd.mnt_idmap in ovl_cleanup_and_whiteout
> ---
>  fs/debugfs/inode.c           | 48 ++++++++++++--------------
>  fs/namei.c                   | 65 ++++++++++++++++++++++++++++++++++++
>  fs/overlayfs/dir.c           | 43 ++++++++++++++++--------
>  include/linux/namei.h        |  2 ++
>  security/selinux/selinuxfs.c | 27 ++++++++++-----
>  5 files changed, 136 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index f241b9df642a..532bd7c46baf 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -842,7 +842,8 @@ int __printf(2, 3) debugfs_change_name(struct dentry *dentry, const char *fmt, .
>  	int error = 0;
>  	const char *new_name;
>  	struct name_snapshot old_name;
> -	struct dentry *parent, *target;
> +	struct dentry *target;
> +	struct renamedata rd = {};
>  	struct inode *dir;
>  	va_list ap;
>  
> @@ -855,36 +856,31 @@ int __printf(2, 3) debugfs_change_name(struct dentry *dentry, const char *fmt, .
>  	if (!new_name)
>  		return -ENOMEM;
>  
> -	parent = dget_parent(dentry);
> -	dir = d_inode(parent);
> -	inode_lock(dir);
> +	rd.old_parent = dget_parent(dentry);
> +	rd.new_parent = rd.old_parent;
> +	rd.flags = RENAME_NOREPLACE;
> +	target = lookup_noperm_unlocked(&QSTR(new_name), rd.new_parent);
> +	if (IS_ERR(target))
> +		return PTR_ERR(target);
>  
> -	take_dentry_name_snapshot(&old_name, dentry);
> -
> -	if (WARN_ON_ONCE(dentry->d_parent != parent)) {
> -		error = -EINVAL;
> -		goto out;
> -	}
> -	if (strcmp(old_name.name.name, new_name) == 0)
> -		goto out;
> -	target = lookup_noperm(&QSTR(new_name), parent);
> -	if (IS_ERR(target)) {
> -		error = PTR_ERR(target);
> -		goto out;
> -	}
> -	if (d_really_is_positive(target)) {
> -		dput(target);
> -		error = -EINVAL;
> +	error = start_renaming_two_dentries(&rd, dentry, target);
> +	if (error) {
> +		if (error == -EEXIST && target == dentry)
> +			/* it isn't an error to rename a thing to itself */
> +			error = 0;
>  		goto out;
>  	}
> -	simple_rename_timestamp(dir, dentry, dir, target);
> -	d_move(dentry, target);
> -	dput(target);
> +
> +	dir = d_inode(rd.old_parent);
> +	take_dentry_name_snapshot(&old_name, dentry);
> +	simple_rename_timestamp(dir, dentry, dir, rd.new_dentry);
> +	d_move(dentry, rd.new_dentry);
>  	fsnotify_move(dir, dir, &old_name.name, d_is_dir(dentry), NULL, dentry);
> -out:
>  	release_dentry_name_snapshot(&old_name);
> -	inode_unlock(dir);
> -	dput(parent);
> +	end_renaming(&rd);
> +out:
> +	dput(rd.old_parent);
> +	dput(target);
>  	kfree_const(new_name);
>  	return error;
>  }
> diff --git a/fs/namei.c b/fs/namei.c
> index 4b740048df97..7f0384ceb976 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3877,6 +3877,71 @@ int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
>  }
>  EXPORT_SYMBOL(start_renaming_dentry);
>  
> +/**
> + * start_renaming_two_dentries - Lock to dentries in given parents for rename
> + * @rd:           rename data containing parent
> + * @old_dentry:   dentry of name to move
> + * @new_dentry:   dentry to move to
> + *
> + * Ensure locks are in place for rename and check parentage is still correct.
> + *
> + * On success the two dentries are stored in @rd.old_dentry and
> + * @rd.new_dentry and @rd.old_parent and @rd.new_parent are confirmed to
> + * be the parents of the dentries.
> + *
> + * References and the lock can be dropped with end_renaming()
> + *
> + * Returns: zero or an error.
> + */
> +int
> +start_renaming_two_dentries(struct renamedata *rd,
> +			    struct dentry *old_dentry, struct dentry *new_dentry)
> +{
> +	struct dentry *trap;
> +	int err;
> +
> +	/* 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);
> +	err = -EINVAL;
> +	if (d_unhashed(old_dentry) ||
> +	    (rd->old_parent && rd->old_parent != old_dentry->d_parent))
> +		/* old_dentry was removed, or moved and explicit parent requested */
> +		goto out_unlock;
> +	if (d_unhashed(new_dentry) ||
> +	    rd->new_parent != new_dentry->d_parent)
> +		/* new_dentry was removed or moved */
> +		goto out_unlock;
> +
> +	if (old_dentry == trap)
> +		/* source is an ancestor of target */
> +		goto out_unlock;
> +
> +	if (new_dentry == trap) {
> +		/* target is an ancestor of source */
> +		if (rd->flags & RENAME_EXCHANGE)
> +			err = -EINVAL;
> +		else
> +			err = -ENOTEMPTY;
> +		goto out_unlock;
> +	}
> +
> +	err = -EEXIST;
> +	if (d_is_positive(new_dentry) && (rd->flags & RENAME_NOREPLACE))
> +		goto out_unlock;
> +
> +	rd->old_dentry = dget(old_dentry);
> +	rd->new_dentry = dget(new_dentry);
> +	rd->old_parent = dget(old_dentry->d_parent);
> +	return 0;
> +
> +out_unlock:
> +	unlock_rename(old_dentry->d_parent, rd->new_parent);
> +	return err;
> +}
> +EXPORT_SYMBOL(start_renaming_two_dentries);
> +
>  void end_renaming(struct renamedata *rd)
>  {
>  	unlock_rename(rd->old_parent, rd->new_parent);
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 6b2f88edb497..61e9484e4ab8 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -123,6 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
>  			     struct dentry *dentry)
>  {
>  	struct dentry *whiteout;
> +	struct renamedata rd = {};
>  	int err;
>  	int flags = 0;
>  
> @@ -134,10 +135,14 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
>  	if (d_is_dir(dentry))
>  		flags = RENAME_EXCHANGE;
>  
> -	err = ovl_lock_rename_workdir(ofs->workdir, whiteout, dir, dentry);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = ofs->workdir;
> +	rd.new_parent = dir;
> +	rd.flags = flags;
> +	err = start_renaming_two_dentries(&rd, whiteout, dentry);
>  	if (!err) {
> -		err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> -		unlock_rename(ofs->workdir, dir);
> +		err = ovl_do_rename_rd(&rd);
> +		end_renaming(&rd);
>  	}
>  	if (err)
>  		goto kill_whiteout;
> @@ -388,6 +393,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>  	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>  	struct dentry *workdir = ovl_workdir(dentry);
>  	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> +	struct renamedata rd = {};
>  	struct path upperpath;
>  	struct dentry *upper;
>  	struct dentry *opaquedir;
> @@ -413,7 +419,11 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>  	if (IS_ERR(opaquedir))
>  		goto out;
>  
> -	err = ovl_lock_rename_workdir(workdir, opaquedir, upperdir, upper);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = workdir;
> +	rd.new_parent = upperdir;
> +	rd.flags = RENAME_EXCHANGE;
> +	err = start_renaming_two_dentries(&rd, opaquedir, upper);
>  	if (err)
>  		goto out_cleanup_unlocked;
>  
> @@ -431,8 +441,8 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>  	if (err)
>  		goto out_cleanup;
>  
> -	err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
> -	unlock_rename(workdir, upperdir);
> +	err = ovl_do_rename_rd(&rd);
> +	end_renaming(&rd);
>  	if (err)
>  		goto out_cleanup_unlocked;
>  
> @@ -445,7 +455,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>  	return opaquedir;
>  
>  out_cleanup:
> -	unlock_rename(workdir, upperdir);
> +	end_renaming(&rd);
>  out_cleanup_unlocked:
>  	ovl_cleanup(ofs, workdir, opaquedir);
>  	dput(opaquedir);
> @@ -468,6 +478,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>  	struct dentry *workdir = ovl_workdir(dentry);
>  	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> +	struct renamedata rd = {};
>  	struct dentry *upper;
>  	struct dentry *newdentry;
>  	int err;
> @@ -499,7 +510,11 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  	if (IS_ERR(newdentry))
>  		goto out_dput;
>  
> -	err = ovl_lock_rename_workdir(workdir, newdentry, upperdir, upper);
> +	rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> +	rd.old_parent = workdir;
> +	rd.new_parent = upperdir;
> +	rd.flags = 0;
> +	err = start_renaming_two_dentries(&rd, newdentry, upper);
>  	if (err)
>  		goto out_cleanup_unlocked;
>  
> @@ -536,16 +551,16 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  		if (err)
>  			goto out_cleanup;
>  
> -		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
> -				    RENAME_EXCHANGE);
> -		unlock_rename(workdir, upperdir);
> +		rd.flags = RENAME_EXCHANGE;
> +		err = ovl_do_rename_rd(&rd);
> +		end_renaming(&rd);
>  		if (err)
>  			goto out_cleanup_unlocked;
>  
>  		ovl_cleanup(ofs, workdir, upper);
>  	} else {
> -		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
> -		unlock_rename(workdir, upperdir);
> +		err = ovl_do_rename_rd(&rd);
> +		end_renaming(&rd);
>  		if (err)
>  			goto out_cleanup_unlocked;
>  	}
> @@ -565,7 +580,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  	return err;
>  
>  out_cleanup:
> -	unlock_rename(workdir, upperdir);
> +	end_renaming(&rd);
>  out_cleanup_unlocked:
>  	ovl_cleanup(ofs, workdir, newdentry);
>  	dput(newdentry);
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index c47713e9867c..9104c7104191 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -161,6 +161,8 @@ 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);
> +int start_renaming_two_dentries(struct renamedata *rd,
> +				struct dentry *old_dentry, struct dentry *new_dentry);
>  void end_renaming(struct renamedata *rd);
>  
>  /**
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 232e087bce3e..a224ef9bb831 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -506,6 +506,7 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
>  {
>  	int ret = 0;
>  	struct dentry *tmp_parent, *tmp_bool_dir, *tmp_class_dir;
> +	struct renamedata rd = {};
>  	unsigned int bool_num = 0;
>  	char **bool_names = NULL;
>  	int *bool_values = NULL;
> @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
>  	if (ret)
>  		goto out;
>  
> -	lock_rename(tmp_parent, fsi->sb->s_root);
> +	rd.old_parent = tmp_parent;
> +	rd.new_parent = fsi->sb->s_root;
>  
>  	/* booleans */
> -	d_exchange(tmp_bool_dir, fsi->bool_dir);
> +	ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> +	if (!ret) {
> +		d_exchange(tmp_bool_dir, fsi->bool_dir);
>  
> -	swap(fsi->bool_num, bool_num);
> -	swap(fsi->bool_pending_names, bool_names);
> -	swap(fsi->bool_pending_values, bool_values);
> +		swap(fsi->bool_num, bool_num);
> +		swap(fsi->bool_pending_names, bool_names);
> +		swap(fsi->bool_pending_values, bool_values);
>  
> -	fsi->bool_dir = tmp_bool_dir;
> +		fsi->bool_dir = tmp_bool_dir;
> +		end_renaming(&rd);
> +	}
>  
>  	/* classes */
> -	d_exchange(tmp_class_dir, fsi->class_dir);
> -	fsi->class_dir = tmp_class_dir;
> +	ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> +	if (ret == 0) {
> +		d_exchange(tmp_class_dir, fsi->class_dir);
> +		fsi->class_dir = tmp_class_dir;
>  
> -	unlock_rename(tmp_parent, fsi->sb->s_root);
> +		end_renaming(&rd);
> +	}
>  
>  out:
>  	sel_remove_old_bool_data(bool_num, bool_names, bool_values);

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