Add start_renaming_two_dentries()

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 not open up any interesting races.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
Link: https://patch.msgid.link/20251113002050.676694-13-neilb@ownmail.net
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f241b9df642aa57b2b37a502faea3e6eac34b678..532bd7c46baf839afe6dc15e473b2f1b40eb6404 100644 (file)
--- 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 5153ceddd37a02e7ada231f1d0c82b67ffd933ca..4a4b8b96c1927544114c2a747d7d9bcc6228a45b 100644 (file)
--- 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 ac5b4475533e648f774248217dd9927da384fa11..b7f443932d933434e0f927f8ab21222572b6d9a1 100644 (file)
--- 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 @@ out:
        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 f73001e3719a4f4e8b79f394e5471103d5724c15..a99ac8b7e24a23989a6f347d32e7a51f206d2122 100644 (file)
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -160,6 +160,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 232e087bce3eeafe9f3ee03999bf6ca9e74f973d..404e08bf60ba949fd24852a3bad789b6417a9f43 100644 (file)
--- 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,9 +540,14 @@ 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 */
+       ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
+       if (ret)
+               goto out;
+
        d_exchange(tmp_bool_dir, fsi->bool_dir);
 
        swap(fsi->bool_num, bool_num);
@@ -549,12 +555,17 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
        swap(fsi->bool_pending_values, bool_values);
 
        fsi->bool_dir = tmp_bool_dir;
+       end_renaming(&rd);
 
        /* classes */
+       ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
+       if (ret)
+               goto out;
+
        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);