Re: overlayfs: overlapping upperdir path

[Miklos Szeredi <miklos@szeredi.hu>]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Thu, Apr 1, 2021 at 4:30 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Apr 1, 2021 at 4:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > Commit 146d62e5a586 ("ovl: detect overlapping layers") made sure we
> > don't have overapping layers, but it also broke the arguably valid use
> > case of
> >
> >  mount -olowerdir=/,upperdir=/subdir,..
> >
> > where subdir also resides on the root fs.
>
> How is 'ls /merged/subdir' expected to behave in that use case?
> Error?

-ELOOP is the error returned.

>
> >
> > I also see that we check for a trap at lookup time, so the question is
> > what does the up-front layer check buy us?
> >
>
> I'm not sure. I know it bought us silence from syzbot that started
> mutating many overlapping layers repos....
> Will the lookup trap have stopped it too? maybe. We did not try.
>
> In general I think that if we can error out to user on mount time
> it is preferred, but if we need to make that use case work, I'd try
> to relax as minimum as possible from the check.

Certainly.  Like lower inside upper makes zero sense, OTOH upper
inside lower does.   So I think we just need to relax the
upperdir/workdir layer check in this case.

Like attached patch.

Thanks,
Miklos

[-- Attachment #2: ovl-allow-upperdir-inside-lowerdir.patch --]
[-- Type: text/x-patch, Size: 1882 bytes --]

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fdd72f1a9c5e..8cf343335029 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1826,7 +1826,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
  * - upper/work dir of any overlayfs instance
  */
 static int ovl_check_layer(struct super_block *sb, struct ovl_fs *ofs,
-			   struct dentry *dentry, const char *name)
+			   struct dentry *dentry, const char *name,
+			   bool is_lower)
 {
 	struct dentry *next = dentry, *parent;
 	int err = 0;
@@ -1838,7 +1839,7 @@ static int ovl_check_layer(struct super_block *sb, struct ovl_fs *ofs,
 
 	/* Walk back ancestors to root (inclusive) looking for traps */
 	while (!err && parent != next) {
-		if (ovl_lookup_trap_inode(sb, parent)) {
+		if (is_lower && ovl_lookup_trap_inode(sb, parent)) {
 			err = -ELOOP;
 			pr_err("overlapping %s path\n", name);
 		} else if (ovl_is_inuse(parent)) {
@@ -1864,7 +1865,7 @@ static int ovl_check_overlapping_layers(struct super_block *sb,
 
 	if (ovl_upper_mnt(ofs)) {
 		err = ovl_check_layer(sb, ofs, ovl_upper_mnt(ofs)->mnt_root,
-				      "upperdir");
+				      "upperdir", false);
 		if (err)
 			return err;
 
@@ -1875,7 +1876,8 @@ static int ovl_check_overlapping_layers(struct super_block *sb,
 		 * workbasedir.  In that case, we already have their traps in
 		 * inode cache and we will catch that case on lookup.
 		 */
-		err = ovl_check_layer(sb, ofs, ofs->workbasedir, "workdir");
+		err = ovl_check_layer(sb, ofs, ofs->workbasedir, "workdir",
+				      false);
 		if (err)
 			return err;
 	}
@@ -1883,7 +1885,7 @@ static int ovl_check_overlapping_layers(struct super_block *sb,
 	for (i = 1; i < ofs->numlayer; i++) {
 		err = ovl_check_layer(sb, ofs,
 				      ofs->layers[i].mnt->mnt_root,
-				      "lowerdir");
+				      "lowerdir", true);
 		if (err)
 			return err;
 	}