Commit 6d0a8a90 authored by Amir Goldstein's avatar Amir Goldstein Committed by Miklos Szeredi
Browse files

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: default avatarAmir Goldstein <amir73il@gmail.com>
Signed-off-by: default avatarMiklos Szeredi <mszeredi@redhat.com>
parent d796e77f

fs/overlayfs/dir.c

+52 −65
Original line number Diff line number Diff line
@@ -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;

}

fs/overlayfs/namei.c

+3 −0
Original line number Diff line number Diff line
@@ -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;

}

fs/overlayfs/readdir.c

+3 −0
Original line number Diff line number Diff line
@@ -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;

CRA Git | Maintained and supported by SUSTech CRA and CCSE