]> git.ipfire.org Git - thirdparty/linux.git/commitdiff
VFS/ovl/smb: introduce start_renaming_dentry()
authorNeilBrown <neil@brown.name>
Thu, 13 Nov 2025 00:18:34 +0000 (11:18 +1100)
committerChristian Brauner <brauner@kernel.org>
Fri, 14 Nov 2025 12:15:57 +0000 (13:15 +0100)
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 <linkinjeon@kernel.org>
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-12-neilb@ownmail.net
Tested-by: syzbot@syzkaller.appspotmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
fs/namei.c
fs/overlayfs/copy_up.c
fs/overlayfs/dir.c
fs/overlayfs/overlayfs.h
fs/overlayfs/super.c
fs/overlayfs/util.c
fs/smb/server/vfs.c
include/linux/namei.h

index 0ee0a110b0884a9a720f50c9539b47e058b6d179..5153ceddd37a02e7ada231f1d0c82b67ffd933ca 100644 (file)
@@ -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 @@ out_unlock:
 
 /**
  * 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 @@ out_unlock:
  * 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);
 
index 7a31ca9bdea28b222f5c1f0db1067d913796985f..27014ada11c73180107bf7f42f1d687dfab5d967 100644 (file)
@@ -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 @@ out:
 
        return err;
 
-cleanup:
-       unlock_rename(c->workdir, c->destdir);
 cleanup_unlocked:
        ovl_cleanup(ofs, c->workdir, temp);
        dput(temp);
index 18a4c7a5ddd2db66a8727242617b275f99b07926..ac5b4475533e648f774248217dd9927da384fa11 100644 (file)
@@ -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)
 {
index 3cc85a893b5c7d16cfff6835d82e93f488541a94..746bc4ad7b37d1f297ed202c383e464605b13d9b 100644 (file)
@@ -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);
 
index 6e0816c1147ac931d45426e3e9cbb3737394dd33..a721ef2b90e8c056b24ba518ce0d514de4d6d3cc 100644 (file)
@@ -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 @@ cleanup_temp:
        ovl_cleanup(ofs, workdir, temp);
        release_dentry_name_snapshot(&name);
        dput(temp);
-       dput(dest);
 
        return err;
 }
index f76672f2e686a44b8b417afd6bd9e44e6328ed82..46387aeb6be6a4970c6485326c897eb851077ed6 100644 (file)
@@ -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;
-}
index 148c65d59e423dc912c6050e17b5c8d51a852919..ea5ab5b0adb194528b50787a4c8b3cebd959796f 100644 (file)
@@ -661,7 +661,6 @@ out1:
 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;
@@ -671,7 +670,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;
@@ -682,14 +680,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,
@@ -706,17 +696,14 @@ retry:
        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) {
@@ -729,44 +716,17 @@ retry:
                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:
index 19c3d8e336d566e7e138b85f4dbacae6576cf512..f73001e3719a4f4e8b79f394e5471103d5724c15 100644 (file)
@@ -158,6 +158,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);
 
 /**