ovl: reorder ovl_want_write() after ovl_inode_lock()

Make the locking order of ovl_inode_lock() strictly between the two
vfs stacked layers, i.e.:
- ovl vfs locks: sb_writers, inode_lock, ...
- ovl_inode_lock
- upper vfs locks: sb_writers, inode_lock, ...

To that effect, move ovl_want_write() into the helpers ovl_nlink_start()
and ovl_copy_up_start which currently take the ovl_inode_lock() after
ovl_want_write().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index ada3fcc9c6d5015ac65929ecf6738720b51615d6..a999ae207e7432a9eca1bf1c8f6c09de606854b4 100644 (file)
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1170,17 +1170,10 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 
 int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 {
-       int err = 0;
-
-       if (ovl_open_need_copy_up(dentry, flags)) {
-               err = ovl_want_write(dentry);
-               if (!err) {
-                       err = ovl_copy_up_flags(dentry, flags);
-                       ovl_drop_write(dentry);
-               }
-       }
+       if (!ovl_open_need_copy_up(dentry, flags))
+               return 0;
 
-       return err;
+       return ovl_copy_up_flags(dentry, flags);
 }
 
 int ovl_copy_up_with_data(struct dentry *dentry)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 033fc0458a3d824b3ba6bb6079ad103c45fa4689..768120c20f85dbbbd3e51a17eab1b3e16bd4840f 100644 (file)
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -559,10 +559,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
        struct cred *override_cred;
        struct dentry *parent = dentry->d_parent;
 
-       err = ovl_copy_up(parent);
-       if (err)
-               return err;
-
        old_cred = ovl_override_creds(dentry->d_sb);
 
        /*
@@ -626,6 +622,10 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
                .link = link,
        };
 
+       err = ovl_copy_up(dentry->d_parent);
+       if (err)
+               return err;
+
        err = ovl_want_write(dentry);
        if (err)
                goto out;
@@ -700,28 +700,24 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
        int err;
        struct inode *inode;
 
-       err = ovl_want_write(old);
+       err = ovl_copy_up(old);
        if (err)
                goto out;
 
-       err = ovl_copy_up(old);
+       err = ovl_copy_up(new->d_parent);
        if (err)
-               goto out_drop_write;
+               goto out;
 
-       err = ovl_copy_up(new->d_parent);
+       err = ovl_nlink_start(old);
        if (err)
-               goto out_drop_write;
+               goto out;
 
        if (ovl_is_metacopy_dentry(old)) {
                err = ovl_set_link_redirect(old);
                if (err)
-                       goto out_drop_write;
+                       goto out_nlink_end;
        }
 
-       err = ovl_nlink_start(old);
-       if (err)
-               goto out_drop_write;
-
        inode = d_inode(old);
        ihold(inode);
 
@@ -731,9 +727,8 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
        if (err)
                iput(inode);
 
+out_nlink_end:
        ovl_nlink_end(old);
-out_drop_write:
-       ovl_drop_write(old);
 out:
        return err;
 }
@@ -891,17 +886,13 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
                        goto out;
        }
 
-       err = ovl_want_write(dentry);
-       if (err)
-               goto out;
-
        err = ovl_copy_up(dentry->d_parent);
        if (err)
-               goto out_drop_write;
+               goto out;
 
        err = ovl_nlink_start(dentry);
        if (err)
-               goto out_drop_write;
+               goto out;
 
        old_cred = ovl_override_creds(dentry->d_sb);
        if (!lower_positive)
@@ -926,8 +917,6 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
        if (ovl_dentry_upper(dentry))
                ovl_copyattr(d_inode(dentry));
 
-out_drop_write:
-       ovl_drop_write(dentry);
 out:
        ovl_cache_free(&list);
        return err;
@@ -1131,29 +1120,32 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
                }
        }
 
-       err = ovl_want_write(old);
-       if (err)
-               goto out;
-
        err = ovl_copy_up(old);
        if (err)
-               goto out_drop_write;
+               goto out;
 
        err = ovl_copy_up(new->d_parent);
        if (err)
-               goto out_drop_write;
+               goto out;
        if (!overwrite) {
                err = ovl_copy_up(new);
                if (err)
-                       goto out_drop_write;
+                       goto out;
        } else if (d_inode(new)) {
                err = ovl_nlink_start(new);
                if (err)
-                       goto out_drop_write;
+                       goto out;
 
                update_nlink = true;
        }
 
+       if (!update_nlink) {
+               /* ovl_nlink_start() took ovl_want_write() */
+               err = ovl_want_write(old);
+               if (err)
+                       goto out;
+       }
+
        old_cred = ovl_override_creds(old->d_sb);
 
        if (!list_empty(&list)) {
@@ -1286,8 +1278,8 @@ out_revert_creds:
        revert_creds(old_cred);
        if (update_nlink)
                ovl_nlink_end(new);
-out_drop_write:
-       ovl_drop_write(old);
+       else
+               ovl_drop_write(old);
 out:
        dput(opaquedir);
        ovl_cache_free(&list);

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 26b782c53910b538cb26d770fdb6b9416d0f19dd..7e16bbcad95e631921b5fe5fe97d220dc10fa08a 100644 (file)
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -23,12 +23,7 @@ static int ovl_encode_maybe_copy_up(struct dentry *dentry)
        if (ovl_dentry_upper(dentry))
                return 0;
 
-       err = ovl_want_write(dentry);
-       if (!err) {
-               err = ovl_copy_up(dentry);
-               ovl_drop_write(dentry);
-       }
-
+       err = ovl_copy_up(dentry);
        if (err) {
                pr_warn_ratelimited("failed to copy up on encode (%pd2, err=%i)\n",
                                    dentry, err);

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b6e98a7d36ce8f96e27b21a5c34ea06e42dfb226..9e9192aa6e07d6689a22d39c5f2ea23b97128fdf 100644 (file)
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -32,10 +32,6 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
        if (err)
                return err;
 
-       err = ovl_want_write(dentry);
-       if (err)
-               goto out;
-
        if (attr->ia_valid & ATTR_SIZE) {
                /* Truncate should trigger data copy up as well */
                full_copy_up = true;
@@ -54,7 +50,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
                        winode = d_inode(upperdentry);
                        err = get_write_access(winode);
                        if (err)
-                               goto out_drop_write;
+                               goto out;
                }
 
                if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
@@ -78,6 +74,10 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
                 */
                attr->ia_valid &= ~ATTR_OPEN;
 
+               err = ovl_want_write(dentry);
+               if (err)
+                       goto out_put_write;
+
                inode_lock(upperdentry->d_inode);
                old_cred = ovl_override_creds(dentry->d_sb);
                err = ovl_do_notify_change(ofs, upperdentry, attr);
@@ -85,12 +85,12 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
                if (!err)
                        ovl_copyattr(dentry->d_inode);
                inode_unlock(upperdentry->d_inode);
+               ovl_drop_write(dentry);
 
+out_put_write:
                if (winode)
                        put_write_access(winode);
        }
-out_drop_write:
-       ovl_drop_write(dentry);
 out:
        return err;
 }
@@ -361,27 +361,27 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
        struct path realpath;
        const struct cred *old_cred;
 
-       err = ovl_want_write(dentry);
-       if (err)
-               goto out;
-
        if (!value && !upperdentry) {
                ovl_path_lower(dentry, &realpath);
                old_cred = ovl_override_creds(dentry->d_sb);
                err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
                revert_creds(old_cred);
                if (err < 0)
-                       goto out_drop_write;
+                       goto out;
        }
 
        if (!upperdentry) {
                err = ovl_copy_up(dentry);
                if (err)
-                       goto out_drop_write;
+                       goto out;
 
                realdentry = ovl_dentry_upper(dentry);
        }
 
+       err = ovl_want_write(dentry);
+       if (err)
+               goto out;
+
        old_cred = ovl_override_creds(dentry->d_sb);
        if (value) {
                err = ovl_do_setxattr(ofs, realdentry, name, value, size,
@@ -391,12 +391,10 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
                err = ovl_do_removexattr(ofs, realdentry, name);
        }
        revert_creds(old_cred);
+       ovl_drop_write(dentry);
 
        /* copy c/mtime */
        ovl_copyattr(inode);
-
-out_drop_write:
-       ovl_drop_write(dentry);
 out:
        return err;
 }
@@ -611,10 +609,6 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
        struct dentry *upperdentry = ovl_dentry_upper(dentry);
        struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
 
-       err = ovl_want_write(dentry);
-       if (err)
-               return err;
-
        /*
         * If ACL is to be removed from a lower file, check if it exists in
         * the first place before copying it up.
@@ -630,7 +624,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
                revert_creds(old_cred);
                if (IS_ERR(real_acl)) {
                        err = PTR_ERR(real_acl);
-                       goto out_drop_write;
+                       goto out;
                }
                posix_acl_release(real_acl);
        }
@@ -638,23 +632,26 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
        if (!upperdentry) {
                err = ovl_copy_up(dentry);
                if (err)
-                       goto out_drop_write;
+                       goto out;
 
                realdentry = ovl_dentry_upper(dentry);
        }
 
+       err = ovl_want_write(dentry);
+       if (err)
+               goto out;
+
        old_cred = ovl_override_creds(dentry->d_sb);
        if (acl)
                err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
        else
                err = ovl_do_remove_acl(ofs, realdentry, acl_name);
        revert_creds(old_cred);
+       ovl_drop_write(dentry);
 
        /* copy c/mtime */
        ovl_copyattr(inode);
-
-out_drop_write:
-       ovl_drop_write(dentry);
+out:
        return err;
 }
 
@@ -778,14 +775,14 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
        unsigned int flags;
        int err;
 
-       err = ovl_want_write(dentry);
-       if (err)
-               goto out;
-
        err = ovl_copy_up(dentry);
        if (!err) {
                ovl_path_real(dentry, &upperpath);
 
+               err = ovl_want_write(dentry);
+               if (err)
+                       goto out;
+
                old_cred = ovl_override_creds(inode->i_sb);
                /*
                 * Store immutable/append-only flags in xattr and clear them
@@ -798,6 +795,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
                if (!err)
                        err = ovl_real_fileattr_set(&upperpath, fa);
                revert_creds(old_cred);
+               ovl_drop_write(dentry);
 
                /*
                 * Merge real inode flags with inode flags read from
@@ -812,7 +810,6 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
                /* Update ctime */
                ovl_copyattr(inode);
        }
-       ovl_drop_write(dentry);
 out:
        return err;
 }

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 80f20ca853442a14d927e33a3e1aef88e1d37f34..af11c83b7a2542807353606d6202f50ac52d7511 100644 (file)
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -676,16 +676,26 @@ int ovl_copy_up_start(struct dentry *dentry, int flags)
        int err;
 
        err = ovl_inode_lock_interruptible(inode);
-       if (!err && ovl_already_copied_up_locked(dentry, flags)) {
+       if (err)
+               return err;
+
+       if (ovl_already_copied_up_locked(dentry, flags))
                err = 1; /* Already copied up */
-               ovl_inode_unlock(inode);
-       }
+       else
+               err = ovl_want_write(dentry);
+       if (err)
+               goto out_unlock;
+
+       return 0;
 
+out_unlock:
+       ovl_inode_unlock(inode);
        return err;
 }
 
 void ovl_copy_up_end(struct dentry *dentry)
 {
+       ovl_drop_write(dentry);
        ovl_inode_unlock(d_inode(dentry));
 }
 
@@ -1088,8 +1098,12 @@ int ovl_nlink_start(struct dentry *dentry)
        if (err)
                return err;
 
+       err = ovl_want_write(dentry);
+       if (err)
+               goto out_unlock;
+
        if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
-               goto out;
+               return 0;
 
        old_cred = ovl_override_creds(dentry->d_sb);
        /*
@@ -1100,10 +1114,15 @@ int ovl_nlink_start(struct dentry *dentry)
         */
        err = ovl_set_nlink_upper(dentry);
        revert_creds(old_cred);
-
-out:
        if (err)
-               ovl_inode_unlock(inode);
+               goto out_drop_write;
+
+       return 0;
+
+out_drop_write:
+       ovl_drop_write(dentry);
+out_unlock:
+       ovl_inode_unlock(inode);
 
        return err;
 }
@@ -1120,6 +1139,7 @@ void ovl_nlink_end(struct dentry *dentry)
                revert_creds(old_cred);
        }
 
+       ovl_drop_write(dentry);
        ovl_inode_unlock(inode);
 }