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

Jeff Layton
2025-11-12T19:38:47Z

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> 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 <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
>
> ---
> 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 <jlayton@kernel.org>