On Thu, Apr 1, 2021 at 4:30 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, Apr 1, 2021 at 4:37 PM Miklos Szeredi <miklos@xxxxxxxxxx> 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
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;
}
