Hi,
Stacking an overlayfs on top of an overlayfs doens't work when it used to
(tested on v4.9-rc5):
#!/bin/bash -xeu
tmpdir=$(mktemp -d)
pushd ${tmpdir}
mkdir -p {upper,lower,work}
echo 'foo' > lower/bar
mount -t overlay level_zero upper -o lowerdir=lower,upperdir=upper,workdir=work
cat upper/bar
tmpdir2=$(mktemp -d)
pushd ${tmpdir2}
mkdir -p {upper,work}
mount -t overlay level_one upper -o lowerdir=${tmpdir}/upper,upperdir=upper,workdir=work
stat upper/bar # Works fine
cat upper/bar # open() returns ENXIO
I _think_ (I haven't bisected it) the guilty commit is 2d902671ce1c ("vfs:
merge .d_select_inode() into .d_real()"), where vfs_open() -> uses d_real()
with a a NULL inode, which prevents any recursion from happening, when
before d_select_inode() would do the right thing and recurse.
It should be noted this has already been broken and fixed in the past year
in 1c8a47df36d7 ("ovl: fix open in stacked overlay").
Let me know if I can help,
Q
If two overlayfs filesystems are stacked on top of each other, then we need
to recurse when opening a file. This used to work and was first broken by:
4bacc9c9234c ("overlayfs: Make f_path always point to the overlay...")
and fixed by:
1c8a47df36d7 ("ovl: fix open in stacked overlay")
But it looks like it was re-introduced in:
2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
I know close to nothing about VFS/overlayfs so this might be completely
wrong for many reasons but it fixes the following test case for me:
#!/bin/bash -xeu
tmpdir=$(mktemp -d)
pushd ${tmpdir}
mkdir -p {upper,lower,work}
echo -n 'rocks' > lower/ksplice
mount -t overlay level_zero upper -o lowerdir=lower,upperdir=upper,workdir=work
cat upper/ksplice
tmpdir2=$(mktemp -d)
pushd ${tmpdir2}
mkdir -p {upper,work}
mount -t overlay level_one upper -o lowerdir=${tmpdir}/upper,upperdir=upper,workdir=work
ls -l upper/ksplice
cat upper/ksplice
For sanity checking, I've run the test-suite mentionned in
Documentation/filesystems/overlayfs.txt:
git://git.infradead.org/users/dhowells/unionmount-testsuite.git
Though with and without this patch it returned zero so I am assuming it
does not contain any tests with stacked overlayfs.
Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
Cc: Al Viro <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Signed-off-by: Quentin Casasnovas <[email protected]>
---
fs/overlayfs/super.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index edd46a0e951d..ae0b36474a9a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -321,16 +321,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
}
real = ovl_dentry_upper(dentry);
- if (real && (!inode || inode == d_inode(real)))
- return real;
+ if (real && inode && inode == d_inode(real))
+ return real;
+
+ if (real && !inode && d_inode(real))
+ return d_real(real, d_inode(real), open_flags);
real = ovl_dentry_lower(dentry);
if (!real)
goto bug;
- if (!inode || inode == d_inode(real))
+ if (inode && inode == d_inode(real))
return real;
+ if (!inode && d_inode(real))
+ return d_real(real, d_inode(real), open_flags);
+
/* Handle recursion */
return d_real(real, inode, open_flags);
bug:
--
2.11.0.rc2
On Fri, Nov 25, 2016 at 06:09:23PM +0100, Quentin Casasnovas wrote:
> If two overlayfs filesystems are stacked on top of each other, then we need
> to recurse when opening a file. This used to work and was first broken by:
>
> 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay...")
>
> and fixed by:
>
> 1c8a47df36d7 ("ovl: fix open in stacked overlay")
>
> But it looks like it was re-introduced in:
>
> 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
>
> I know close to nothing about VFS/overlayfs
And indeed I've proven it here - this tentative patch doesn't work for the
general case, it just fixes the simple test case embedded in the commit
description.
Any help appreciated!
Q
On Fri, Nov 25, 2016 at 08:28:47PM +0100, Quentin Casasnovas wrote:
> On Fri, Nov 25, 2016 at 06:09:23PM +0100, Quentin Casasnovas wrote:
> > If two overlayfs filesystems are stacked on top of each other, then we need
> > to recurse when opening a file. This used to work and was first broken by:
> >
> > 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay...")
> >
> > and fixed by:
> >
> > 1c8a47df36d7 ("ovl: fix open in stacked overlay")
> >
> > But it looks like it was re-introduced in:
> >
> > 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
Thanks for the report.
Following patch should fix it (it's there in your patch, so you weren't too far
off). It needs more testing and review, but I think it fixes the basic problem.
Thanks,
Miklos
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index edd46a0e951d..f4d2f63fa53a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -328,7 +328,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
if (!real)
goto bug;
- if (!inode || inode == d_inode(real))
+ if (inode && inode == d_inode(real))
return real;
/* Handle recursion */
On Mon, Nov 28, 2016 at 10:45:18AM +0100, Miklos Szeredi wrote:
> On Fri, Nov 25, 2016 at 08:28:47PM +0100, Quentin Casasnovas wrote:
> > On Fri, Nov 25, 2016 at 06:09:23PM +0100, Quentin Casasnovas wrote:
> > > If two overlayfs filesystems are stacked on top of each other, then we need
> > > to recurse when opening a file. This used to work and was first broken by:
> > >
> > > 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay...")
> > >
> > > and fixed by:
> > >
> > > 1c8a47df36d7 ("ovl: fix open in stacked overlay")
> > >
> > > But it looks like it was re-introduced in:
> > >
> > > 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
>
>
> Following patch should fix it (it's there in your patch, so you weren't too far
> off). It needs more testing and review, but I think it fixes the basic problem.
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index edd46a0e951d..f4d2f63fa53a 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -328,7 +328,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> if (!real)
> goto bug;
>
> - if (!inode || inode == d_inode(real))
> + if (inode && inode == d_inode(real))
> return real;
>
> /* Handle recursion */
The above patch works for me, thanks!
Tested-by: Quentin Casasnovas <[email protected]>
Q
On Mon, Nov 28, 2016 at 12:06:09PM +0100, Quentin Casasnovas wrote:
> > > > But it looks like it was re-introduced in:
> > > >
> > > > 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
Here's a slightly different patch. It should work exactly the same, but the
error handling is hopefully less broken.
Thanks,
Miklos
---
From: Miklos Szeredi <[email protected]>
Subject: ovl: fix d_real() for stacked fs
Handling of recursion in d_real() is completely broken. Recursion is only
done in the 'inode != NULL' case. But when opening the file we have
'inode == NULL' hence d_real() will return an overlay dentry. This won't
work since overlayfs doesn't define its own file operations, so all file
ops will fail.
Fix by doing the recursion first and the check against the inode second.
Bash script to reproduce the issue written by Quentin:
- 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -
tmpdir=$(mktemp -d)
pushd ${tmpdir}
mkdir -p {upper,lower,work}
echo -n 'rocks' > lower/ksplice
mount -t overlay level_zero upper -o lowerdir=lower,upperdir=upper,workdir=work
cat upper/ksplice
tmpdir2=$(mktemp -d)
pushd ${tmpdir2}
mkdir -p {upper,work}
mount -t overlay level_one upper -o lowerdir=${tmpdir}/upper,upperdir=upper,workdir=work
ls -l upper/ksplice
cat upper/ksplice
- 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -
Reported-by: Quentin Casasnovas <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
Cc: <[email protected]> # v4.8+
---
fs/overlayfs/super.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -328,11 +328,11 @@ static struct dentry *ovl_d_real(struct
if (!real)
goto bug;
+ /* Handle recursion */
+ real = d_real(real, inode, open_flags);
+
if (!inode || inode == d_inode(real))
return real;
-
- /* Handle recursion */
- return d_real(real, inode, open_flags);
bug:
WARN(1, "ovl_d_real(%pd4, %s:%lu): real dentry not found\n", dentry,
inode ? inode->i_sb->s_id : "NULL", inode ? inode->i_ino : 0);
On Tue, Nov 29, 2016 at 11:32 AM, Miklos Szeredi <[email protected]> wrote:
> On Mon, Nov 28, 2016 at 12:06:09PM +0100, Quentin Casasnovas wrote:
>
>> > > > But it looks like it was re-introduced in:
>> > > >
>> > > > 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
>
> Here's a slightly different patch. It should work exactly the same, but the
> error handling is hopefully less broken.
>
> Thanks,
> Miklos
> ---
>
> From: Miklos Szeredi <[email protected]>
> Subject: ovl: fix d_real() for stacked fs
>
> Handling of recursion in d_real() is completely broken. Recursion is only
> done in the 'inode != NULL' case. But when opening the file we have
> 'inode == NULL' hence d_real() will return an overlay dentry. This won't
> work since overlayfs doesn't define its own file operations, so all file
> ops will fail.
>
> Fix by doing the recursion first and the check against the inode second.
>
> Bash script to reproduce the issue written by Quentin:
>
> - 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -
> tmpdir=$(mktemp -d)
> pushd ${tmpdir}
>
> mkdir -p {upper,lower,work}
> echo -n 'rocks' > lower/ksplice
> mount -t overlay level_zero upper -o lowerdir=lower,upperdir=upper,workdir=work
This double-up of upper is confusing to the average reader (me).
Best keep it in private scripts and out of commit message.
> cat upper/ksplice
>
> tmpdir2=$(mktemp -d)
> pushd ${tmpdir2}
>
> mkdir -p {upper,work}
> mount -t overlay level_one upper -o lowerdir=${tmpdir}/upper,upperdir=upper,workdir=work
> ls -l upper/ksplice
> cat upper/ksplice
> - 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -
>
> Reported-by: Quentin Casasnovas <[email protected]>
> Signed-off-by: Miklos Szeredi <[email protected]>
> Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
> Cc: <[email protected]> # v4.8+
> ---
> fs/overlayfs/super.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -328,11 +328,11 @@ static struct dentry *ovl_d_real(struct
> if (!real)
> goto bug;
>
> + /* Handle recursion */
> + real = d_real(real, inode, open_flags);
> +
IMO, we should verify that we don't pass WRITE/TRUNC flags
to lower overlayfs (or whatever fs is underneath us).
Although current code paths seem unlikely to reach here with real in lower,
this may change in the future.
Suggest to either clear the WRITE/TRUNC flags before recursion
or WARN_ON for this case (or both).
e.g.:
real = ovl_dentry_upper(dentry);
if (real && (!inode || inode == d_inode(real)))
return real;
+ if (!real && (OPEN_FMODE(open_flags) & FMODE_WRITE) || (open_flags
& O_TRUNC))
+ goto bug;
> if (!inode || inode == d_inode(real))
> return real;
> -
> - /* Handle recursion */
> - return d_real(real, inode, open_flags);
> bug:
> WARN(1, "ovl_d_real(%pd4, %s:%lu): real dentry not found\n", dentry,
> inode ? inode->i_sb->s_id : "NULL", inode ? inode->i_ino : 0);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 29, 2016 at 10:32:29AM +0100, Miklos Szeredi wrote:
> On Mon, Nov 28, 2016 at 12:06:09PM +0100, Quentin Casasnovas wrote:
>
> > > > > But it looks like it was re-introduced in:
> > > > >
> > > > > 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
>
> Here's a slightly different patch. It should work exactly the same, but the
> error handling is hopefully less broken.
>
Tested-by: Quentin Casasnovas <[email protected]>
Thanks Miklos!