]> git.ipfire.org Git - thirdparty/linux.git/commitdiff
ovl: simplify an error path in ovl_copy_up_workdir()
authorNeilBrown <neil@brown.name>
Wed, 16 Jul 2025 00:44:12 +0000 (10:44 +1000)
committerChristian Brauner <brauner@kernel.org>
Fri, 18 Jul 2025 09:10:40 +0000 (11:10 +0200)
If ovl_copy_up_data() fails the error is not immediately handled but the
code continues on to call ovl_start_write() and lock_rename(),
presumably because both of these locks are needed for the cleanup.
Only then (if the lock was successful) is the error checked.

This makes the code a little hard to follow and could be fragile.

This patch changes to handle the error after the ovl_start_write()
(which cannot fail, so there aren't multiple errors to deail with).  A
new ovl_cleanup_unlocked() is created which takes the required directory
lock.  This will be used extensively in later patches.

In general we need to check the parent is still correct after taking the
lock (as ovl_copy_up_workdir() does after a successful lock_rename()) so
that is included in ovl_cleanup_unlocked() using new ovl_parent_lock()
and ovl_parent_unlock() calls (it is planned to move this API into VFS code
eventually, though in a slightly different form).

Signed-off-by: NeilBrown <neil@brown.name>
Link: https://lore.kernel.org/20250716004725.1206467-2-neil@brown.name
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
fs/overlayfs/copy_up.c
fs/overlayfs/dir.c
fs/overlayfs/overlayfs.h
fs/overlayfs/util.c

index 8a3c0d18ec2ea64fe205bf12014edbef25493da6..79f41ef6ffa7dcdb33839feb0f8e340caf199a0c 100644 (file)
@@ -794,23 +794,24 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
         */
        path.dentry = temp;
        err = ovl_copy_up_data(c, &path);
+       ovl_start_write(c->dentry);
+       if (err)
+               goto cleanup_unlocked;
+
        /*
         * We cannot hold lock_rename() throughout this helper, because of
         * lock ordering with sb_writers, which shouldn't be held when calling
         * ovl_copy_up_data(), so lock workdir and destdir and make sure that
         * temp wasn't moved before copy up completion or cleanup.
         */
-       ovl_start_write(c->dentry);
        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);
                err = -EIO;
-               if (IS_ERR(trap))
-                       goto out;
-               goto unlock;
-       } else if (err) {
-               goto cleanup;
+               if (!IS_ERR(trap))
+                       unlock_rename(c->workdir, c->destdir);
+               goto out;
        }
 
        err = ovl_copy_up_metadata(c, temp);
@@ -846,7 +847,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
        ovl_inode_update(inode, temp);
        if (S_ISDIR(inode->i_mode))
                ovl_set_flag(OVL_WHITEOUTS, inode);
-unlock:
        unlock_rename(c->workdir, c->destdir);
 out:
        ovl_end_write(c->dentry);
@@ -854,9 +854,11 @@ out:
        return err;
 
 cleanup:
-       ovl_cleanup(ofs, wdir, temp);
+       unlock_rename(c->workdir, c->destdir);
+cleanup_unlocked:
+       ovl_cleanup_unlocked(ofs, c->workdir, temp);
        dput(temp);
-       goto unlock;
+       goto out;
 }
 
 /* Copyup using O_TMPFILE which does not require cross dir locking */
index 4fc221ea6480c8a0de4b4366063d4ab604952d98..67cad3dba8ad639e86cadb57da5c3e39a1ae79c2 100644 (file)
@@ -43,6 +43,21 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
        return err;
 }
 
+int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
+                        struct dentry *wdentry)
+{
+       int err;
+
+       err = ovl_parent_lock(workdir, wdentry);
+       if (err)
+               return err;
+
+       ovl_cleanup(ofs, workdir->d_inode, wdentry);
+       ovl_parent_unlock(workdir);
+
+       return 0;
+}
+
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
 {
        struct dentry *temp;
index 3c52ecddfc9cc06ef817aa2c9f6d5552f09b74ce..a8f18a06a19e0330b14e100c38111b7418f1a244 100644 (file)
@@ -414,6 +414,11 @@ 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);
@@ -847,6 +852,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
                               struct inode *dir, struct dentry *newdentry,
                               struct ovl_cattr *attr);
 int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
+int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
 struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
                               struct ovl_cattr *attr);
index cc793c8f001f13cde04e1ddfae3333fc879e2738..130aba055ffe54099b89298bfadff3267f323a05 100644 (file)
@@ -1551,3 +1551,13 @@ 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 || child->d_parent == parent)
+               return 0;
+
+       inode_unlock(parent->d_inode);
+       return -EINVAL;
+}