ovl: take lower dir inode mutex outside upper sb_writers lock

The functions ovl_lower_positive() and ovl_check_empty_dir() both take
inode mutex on the real lower dir under ovl_want_write() which takes
the upper_mnt sb_writers lock.

While this is not a clear locking order or layering violation, it creates
an undesired lock dependency between two unrelated layers for no good
reason.

This lock dependency materializes to a false(?) positive lockdep warning
when calling rmdir() on a nested overlayfs, where both nested and
underlying overlayfs both use the same fs type as upper layer.

rmdir() on the nested overlayfs creates the lock chain:
  sb_writers of upper_mnt (e.g. tmpfs) in ovl_do_remove()
  ovl_i_mutex_dir_key[] of lower overlay dir in ovl_lower_positive()

rmdir() on the underlying overlayfs creates the lock chain in
reverse order:
  ovl_i_mutex_dir_key[] of lower overlay dir in vfs_rmdir()
  sb_writers of nested upper_mnt (e.g. tmpfs) in ovl_do_remove()

To rid of the unneeded locking dependency, move both ovl_lower_positive()
and ovl_check_empty_dir() to before ovl_want_write() in rmdir() and
rename() implementation.

This change spreads the pieces of ovl_check_empty_and_clear() directly
inside the rmdir()/rename() implementations so the helper is no longer
needed and removed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index f9788bc116a8d1b5137f805d9955995e68b31884..a1a7606d4891b58cbe3199b510485c61e6125c50 100644 (file)
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -181,11 +181,6 @@ static bool ovl_type_origin(struct dentry *dentry)
        return OVL_TYPE_ORIGIN(ovl_path_type(dentry));
 }
 
-static bool ovl_may_have_whiteouts(struct dentry *dentry)
-{
-       return ovl_test_flag(OVL_WHITEOUTS, d_inode(dentry));
-}
-
 static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
                            struct cattr *attr, struct dentry *hardlink)
 {
@@ -301,37 +296,6 @@ out:
        return ERR_PTR(err);
 }
 
-static struct dentry *ovl_check_empty_and_clear(struct dentry *dentry)
-{
-       int err;
-       struct dentry *ret = NULL;
-       LIST_HEAD(list);
-
-       err = ovl_check_empty_dir(dentry, &list);
-       if (err) {
-               ret = ERR_PTR(err);
-               goto out_free;
-       }
-
-       /*
-        * When removing an empty opaque directory, then it makes no sense to
-        * replace it with an exact replica of itself.
-        *
-        * If upperdentry has whiteouts, clear them.
-        *
-        * Can race with copy-up, since we don't hold the upperdir mutex.
-        * Doesn't matter, since copy-up can't create a non-empty directory
-        * from an empty one.
-        */
-       if (!list_empty(&list))
-               ret = ovl_clear_empty(dentry, &list);
-
-out_free:
-       ovl_cache_free(&list);
-
-       return ret;
-}
-
 static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name,
                             const struct posix_acl *acl)
 {
@@ -623,7 +587,8 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
        return d_inode(ovl_dentry_upper(dentry)) == d_inode(upper);
 }
 
-static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
+static int ovl_remove_and_whiteout(struct dentry *dentry,
+                                  struct list_head *list)
 {
        struct dentry *workdir = ovl_workdir(dentry);
        struct inode *wdir = workdir->d_inode;
@@ -638,8 +603,8 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
        if (WARN_ON(!workdir))
                return -EROFS;
 
-       if (is_dir) {
-               opaquedir = ovl_check_empty_and_clear(dentry);
+       if (!list_empty(list)) {
+               opaquedir = ovl_clear_empty(dentry, list);
                err = PTR_ERR(opaquedir);
                if (IS_ERR(opaquedir))
                        goto out;
@@ -694,7 +659,8 @@ kill_whiteout:
        goto out_d_drop;
 }
 
-static int ovl_remove_upper(struct dentry *dentry, bool is_dir)
+static int ovl_remove_upper(struct dentry *dentry, bool is_dir,
+                           struct list_head *list)
 {
        struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
        struct inode *dir = upperdir->d_inode;
@@ -702,10 +668,8 @@ static int ovl_remove_upper(struct dentry *dentry, bool is_dir)
        struct dentry *opaquedir = NULL;
        int err;
 
-       /* Redirect/origin dir can be !ovl_lower_positive && not clean */
-       if (is_dir && (ovl_dentry_get_redirect(dentry) ||
-                      ovl_may_have_whiteouts(dentry))) {
-               opaquedir = ovl_check_empty_and_clear(dentry);
+       if (!list_empty(list)) {
+               opaquedir = ovl_clear_empty(dentry, list);
                err = PTR_ERR(opaquedir);
                if (IS_ERR(opaquedir))
                        goto out;
@@ -746,11 +710,26 @@ out:
        return err;
 }
 
+static bool ovl_pure_upper(struct dentry *dentry)
+{
+       return !ovl_dentry_lower(dentry) &&
+              !ovl_test_flag(OVL_WHITEOUTS, d_inode(dentry));
+}
+
 static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 {
        int err;
        bool locked = false;
        const struct cred *old_cred;
+       bool lower_positive = ovl_lower_positive(dentry);
+       LIST_HEAD(list);
+
+       /* No need to clean pure upper removed by vfs_rmdir() */
+       if (is_dir && (lower_positive || !ovl_pure_upper(dentry))) {
+               err = ovl_check_empty_dir(dentry, &list);
+               if (err)
+                       goto out;
+       }
 
        err = ovl_want_write(dentry);
        if (err)
@@ -765,10 +744,10 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
                goto out_drop_write;
 
        old_cred = ovl_override_creds(dentry->d_sb);
-       if (!ovl_lower_positive(dentry))
-               err = ovl_remove_upper(dentry, is_dir);
+       if (!lower_positive)
+               err = ovl_remove_upper(dentry, is_dir, &list);
        else
-               err = ovl_remove_and_whiteout(dentry, is_dir);
+               err = ovl_remove_and_whiteout(dentry, &list);
        revert_creds(old_cred);
        if (!err) {
                if (is_dir)
@@ -780,6 +759,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 out_drop_write:
        ovl_drop_write(dentry);
 out:
+       ovl_cache_free(&list);
        return err;
 }
 
@@ -915,6 +895,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
        bool samedir = olddir == newdir;
        struct dentry *opaquedir = NULL;
        const struct cred *old_cred = NULL;
+       LIST_HEAD(list);
 
        err = -EINVAL;
        if (flags & ~(RENAME_EXCHANGE | RENAME_NOREPLACE))
@@ -929,6 +910,27 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
        if (!overwrite && !ovl_can_move(new))
                goto out;
 
+       if (overwrite && new_is_dir && !ovl_pure_upper(new)) {
+               err = ovl_check_empty_dir(new, &list);
+               if (err)
+                       goto out;
+       }
+
+       if (overwrite) {
+               if (ovl_lower_positive(old)) {
+                       if (!ovl_dentry_is_whiteout(new)) {
+                               /* Whiteout source */
+                               flags |= RENAME_WHITEOUT;
+                       } else {
+                               /* Switch whiteouts */
+                               flags |= RENAME_EXCHANGE;
+                       }
+               } else if (is_dir && ovl_dentry_is_whiteout(new)) {
+                       flags |= RENAME_EXCHANGE;
+                       cleanup_whiteout = true;
+               }
+       }
+
        err = ovl_want_write(old);
        if (err)
                goto out;
@@ -952,9 +954,8 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 
        old_cred = ovl_override_creds(old->d_sb);
 
-       if (overwrite && new_is_dir && (ovl_type_merge_or_lower(new) ||
-                                       ovl_may_have_whiteouts(new))) {
-               opaquedir = ovl_check_empty_and_clear(new);
+       if (!list_empty(&list)) {
+               opaquedir = ovl_clear_empty(new, &list);
                err = PTR_ERR(opaquedir);
                if (IS_ERR(opaquedir)) {
                        opaquedir = NULL;
@@ -962,21 +963,6 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
                }
        }
 
-       if (overwrite) {
-               if (ovl_lower_positive(old)) {
-                       if (!ovl_dentry_is_whiteout(new)) {
-                               /* Whiteout source */
-                               flags |= RENAME_WHITEOUT;
-                       } else {
-                               /* Switch whiteouts */
-                               flags |= RENAME_EXCHANGE;
-                       }
-               } else if (is_dir && ovl_dentry_is_whiteout(new)) {
-                       flags |= RENAME_EXCHANGE;
-                       cleanup_whiteout = true;
-               }
-       }
-
        old_upperdir = ovl_dentry_upper(old->d_parent);
        new_upperdir = ovl_dentry_upper(new->d_parent);
 
@@ -1094,6 +1080,7 @@ out_drop_write:
        ovl_drop_write(old);
 out:
        dput(opaquedir);
+       ovl_cache_free(&list);
        return err;
 }
 

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index beb945e1963c0aac86fbce312ca9814821a1c33d..926248e1de045ee3dc72bf6f793769d0d7344949 100644 (file)
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -774,6 +774,7 @@ bool ovl_lower_positive(struct dentry *dentry)
        struct ovl_entry *oe = dentry->d_fsdata;
        struct ovl_entry *poe = dentry->d_parent->d_fsdata;
        const struct qstr *name = &dentry->d_name;
+       const struct cred *old_cred;
        unsigned int i;
        bool positive = false;
        bool done = false;
@@ -789,6 +790,7 @@ bool ovl_lower_positive(struct dentry *dentry)
        if (!ovl_dentry_upper(dentry))
                return true;
 
+       old_cred = ovl_override_creds(dentry->d_sb);
        /* Positive upper -> have to look up lower to see whether it exists */
        for (i = 0; !done && !positive && i < poe->numlower; i++) {
                struct dentry *this;
@@ -818,6 +820,7 @@ bool ovl_lower_positive(struct dentry *dentry)
                        dput(this);
                }
        }
+       revert_creds(old_cred);
 
        return positive;
 }

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index a7e45e6cd732937ce787c6c2010fb8249ae0dbac..7dfe381c2cd8706516d625eb8121be5a086074a0 100644 (file)
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -862,8 +862,11 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
        int err;
        struct ovl_cache_entry *p, *n;
        struct rb_root root = RB_ROOT;
+       const struct cred *old_cred;
 
+       old_cred = ovl_override_creds(dentry->d_sb);
        err = ovl_dir_read_merged(dentry, list, &root);
+       revert_creds(old_cred);
        if (err)
                return err;