ovl: fix regression caused by exclusive upper/work dir protection

PATCH In review Amir Goldstein 2 participants 3 messages Sep 29, 2017 – Oct 6, 2017

Subsystems (3)

Stable branch Documentation Overlay filesystem

lore

Share & tools

lore

Amir Goldstein

author maintainer

2017-09-29 08:21 UTC #0 lore raw

PATCH 1/1 ovl: fix regression caused by exclusive upper/work dir protection

Enforcing exclusive ownership on upper/work dirs caused a docker
regression: https://github.com/moby/moby/issues/34672.

Euan spotted the regression and pointed to the offending commit. Vivek has brought the regression to my attention and provided this reproducer:

Terminal 1:

mount -t overlay -o workdir=work,lowerdir=lower,upperdir=upper none
merged/

Terminal 2:

unshare -m

Terminal 1:

umount merged
mount -t overlay -o workdir=work,lowerdir=lower,upperdir=upper none
merged/
mount: /root/overlay-testing/merged: none already mounted or mount point
busy

To fix the regression, I replaced the error with an alarming warning. With index feature enabled, mount does fail, but logs a suggestion to override exclusive dir protection by disabling index.
Note that index=off mount does take the inuse locks, so a concurrent index=off will issue the warning and a concurrent index=on mount will fail.

Documentation was updated to reflect this change.

Fixes 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")

Cc <[email protected]> # v4.13

Reported-by Euan Kemp <[email protected]>

Reported-by Vivek Goyal <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 Documentation/filesystems/overlayfs.txt |  5 ++++-
 fs/overlayfs/ovl_entry.h                |  3 +++
 fs/overlayfs/super.c                    | 27 +++++++++++++++++++--------
 3 files changed, 26 insertions(+), 9 deletions(-)

Documentation/filesystems/overlayfs.txt +4 -1

index 36f528a7fdd6..8caa60734647 100644
@@ -210,8 +210,11 @@ path as another overlay mount and it may use a lower layer path that is
 beneath or above the path of another overlay lower layer path.
 
 Using an upper layer path and/or a workdir path that are already used by
-another overlay mount is not allowed and will fail with EBUSY.  Using
+another overlay mount is not allowed and may fail with EBUSY.  Using
 partially overlapping paths is not allowed but will not fail with EBUSY.
+If files are accessed from two overlayfs mounts which share or overlap the
+upper layer and/or workdir path the behavior of the overlay is undefined,
+though it will not result in a crash or deadlock.
 
 Mounting an overlay using an upper layer path, where the upper layer path
 was previously used by another mounted overlay in combination with a

fs/overlayfs/ovl_entry.h +3 -0

index 878a750986dd..25d9b5adcd42 100644
@@ -37,6 +37,9 @@ struct ovl_fs {
 	bool noxattr;
 	/* sb common to all layers */
 	struct super_block *same_sb;
+	/* Did we take the inuse lock? */
+	bool upperdir_locked;
+	bool workdir_locked;
 };
 
 /* private information held for every overlayfs dentry */

fs/overlayfs/super.c +19 -8

index f2447745a5b7..d161f97d6e86 100644
@@ -211,9 +211,10 @@ static void ovl_put_super(struct super_block *sb)
 
 	dput(ufs->indexdir);
 	dput(ufs->workdir);
-	ovl_inuse_unlock(ufs->workbasedir);
+	if (ufs->workdir_locked)
+		ovl_inuse_unlock(ufs->workbasedir);
 	dput(ufs->workbasedir);
-	if (ufs->upper_mnt)
+	if (ufs->upper_mnt && ufs->upperdir_locked)
 		ovl_inuse_unlock(ufs->upper_mnt->mnt_root);
 	mntput(ufs->upper_mnt);
 	for (i = 0; i < ufs->numlower; i++)

@@ -893,9 +894,13 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			goto out_put_upperpath;
 
 		err = -EBUSY;
-		if (!ovl_inuse_trylock(upperpath.dentry)) {
-			pr_err("overlayfs: upperdir is in-use by another mount\n");
+		if (ovl_inuse_trylock(upperpath.dentry)) {
+			ufs->upperdir_locked = true;
+		} else if (ufs->config.index) {
+			pr_err("overlayfs: upperdir is in-use by another mount, mount with '-o index=off' to override exclusive upperdir protection.\n");
 			goto out_put_upperpath;
+		} else {
+			pr_warn("overlayfs: upperdir is in-use by another mount, accessing files from both mounts will result in undefined behavior.\n");
 		}
 
 		err = ovl_mount_dir(ufs->config.workdir, &workpath);

@@ -913,9 +918,13 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		}
 
 		err = -EBUSY;
-		if (!ovl_inuse_trylock(workpath.dentry)) {
-			pr_err("overlayfs: workdir is in-use by another mount\n");
+		if (ovl_inuse_trylock(workpath.dentry)) {
+			ufs->workdir_locked = true;
+		} else if (ufs->config.index) {
+			pr_err("overlayfs: workdir is in-use by another mount, mount with '-o index=off' to override exclusive workdir protection.\n");
 			goto out_put_workpath;
+		} else {
+			pr_warn("overlayfs: workdir is in-use by another mount, accessing files from both mounts will result in undefined behavior.\n");
 		}
 
 		ufs->workbasedir = workpath.dentry;

@@ -1168,11 +1177,13 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 out_free_lowertmp:
 	kfree(lowertmp);
 out_unlock_workdentry:
-	ovl_inuse_unlock(workpath.dentry);
+	if (ufs->workdir_locked)
+		ovl_inuse_unlock(workpath.dentry);
 out_put_workpath:
 	path_put(&workpath);
 out_unlock_upperdentry:
-	ovl_inuse_unlock(upperpath.dentry);
+	if (ufs->upperdir_locked)
+		ovl_inuse_unlock(upperpath.dentry);
 out_put_upperpath:
 	path_put(&upperpath);
 out_free_config:

Signature · 1 line

2.7.4

Amir Goldstein

author maintainer

2017-09-29 08:31 UTC #1 lore raw

Quoted 30 lines · Amir Goldstein

Enforcing exclusive ownership on upper/work dirs caused a docker
regression: https://github.com/moby/moby/issues/34672.
Euan spotted the regression and pointed to the offending commit. Vivek has brought the regression to my attention and provided this reproducer:
Terminal 1:
mount -t overlay -o workdir=work,lowerdir=lower,upperdir=upper none
merged/
Terminal 2:
unshare -m
Terminal 1:
umount merged
mount -t overlay -o workdir=work,lowerdir=lower,upperdir=upper none
merged/
mount: /root/overlay-testing/merged: none already mounted or mount point
busy
To fix the regression, I replaced the error with an alarming warning. With index feature enabled, mount does fail, but logs a suggestion to override exclusive dir protection by disabling index.
Note that index=off mount does take the inuse locks, so a concurrent index=off will issue the warning and a concurrent index=on mount will fail.

Quoted 6 lines

Documentation was updated to reflect this change.
Fixes 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")
Cc <[email protected]> # v4.13
Reported-by Euan Kemp <[email protected]>
Reported-by Vivek Goyal <[email protected]>

Euan, Vivek

Apparently, git send-email doesn't pick up the Reported-by tags, so did not CC you.
Care to test this patch?

I am going to update xfstest overlay/036 to reflect this change:
https://github.com/amir73il/xfstests/commit/c8098b949a0f3338e6f45e94e28ee6b7baa284e7

Vivek,

Do you mind making your reproducer into an xfstest?
If you do, please note that there is currently no _require_namespace in xfstests and the only test that does use unshare (overlay/020) just fails when kernel has no namespaces support, so this will have to be fixed.

Thanks!
Amir.

Euan Kemp

2017-10-06 17:16 UTC #2 lore raw

Quoted 1 line · Amir Goldstein

Care to test this patch?

I can verify this patch resolves the issue I reported against the moby project (https://github.com/moby/moby/issues/34672).

I think including this in the stable kernel makes sense; without this there's noticeable userspace impact.

- Euan