Re: overlayfs: mounting overlayfs on top of overlayfs

[Miklos Szeredi <miklos@xxxxxxxxxx>]

Jordi Pujol <jordipujolp@xxxxxxxxx> writes:

> A Dimarts, 7 de juny de 2011 10:08:48, Miklos Szeredi va escriure:
>> Well, this is not correct.  We *do* want to check for write permission.
> Well, another of my patches that solve the problem making a workaround,
>
>> The above would simply ignore the access control.
> only in some case.
>
>> What are the permission bits on the file in question?
>
> the problem is that sed can't create the temporary file in the same
> directory, normally sed will create it using umask permissions, 0022
> in my system,
>
> try the automated test contained in my previous message, it creates a
> log file that shows the errors.

Thanks for the script, it helped.

The attached patch should fix it.  It's not a generic solution and other
filesystems may still not function correctly as a read-only lower layer
(e.g. I can see that btrfs has some sort of "r/o volume flag").

That can be fixed by checking file mode and acl instead of calling
->permission() on the lower inode.  Will add that as a FIXME item.

Thanks,
Miklos

commit 0896db57a7e8ab075278c8d46a2620a3f32d866f
Author: Miklos Szeredi <mszeredi@xxxxxxx>
Date:   Wed Jun 8 16:53:50 2011 +0200

    ovl: fix overlayfs over overlayfs
    
    Overlayfs expects ->permission() to not check for readonliness (which
    is normally checked by the VFS) and so not return with -EROFS.  This
    is not true of some filesystems, notably overlayfs itself.
    
    The following patch should fix this by making sure that if the upper
    layer is read-only (such as squashfs) then it will mark overlayfs
    read-only too and by making ovl_permission() only return EROFS in the
    excpetional case where the upper filesystem became r/o after the
    overlay was constructed.
    
    Reported-by: Jordi Pujol <jordipujolp@xxxxxxxxx>
    Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxx>

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 289006e..ce39fab 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -90,9 +90,18 @@ int ovl_permission(struct inode *inode, int mask, unsigned int flags)
 		/*
 		 * Writes will always be redirected to upper layer, so
 		 * ignore lower layer being read-only.
+		 *
+		 * If the overlay itself is read-only then proceed
+		 * with the permission check, don't return EROFS.
+		 * This will only happen if this is the lower layer of
+		 * another overlayfs.
+		 *
+		 * If upper fs becomes read-only after the overlay was
+		 * constructed return EROFS to prevent modification of
+		 * upper layer.
 		 */
 		err = -EROFS;
-		if (is_upper && IS_RDONLY(realinode) &&
+		if (is_upper && !IS_RDONLY(inode) && IS_RDONLY(realinode) &&
 		    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
 			goto out_dput;
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7109b45..c741b17 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -548,6 +548,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_put_upper_mnt;
 	}
 
+	/* If the upper fs is r/o, we mark overlayfs r/o too */
+	if (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY)
+		sb->s_flags |= MS_RDONLY;
+
 	if (!(sb->s_flags & MS_RDONLY)) {
 		err = mnt_want_write(ufs->upper_mnt);
 		if (err)