Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530310 > unrolled thread

opening a file on a stacked overlayfs is broken.

Back to article view | Back to linux.kernel

Contents

  opening a file on a stacked overlayfs is broken. Quentin Casasnovas <quentin.casasnovas@oracle.com> - 2016-11-25 16:00 +0100
    [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs. Quentin Casasnovas <quentin.casasnovas@oracle.com> - 2016-11-25 18:10 +0100
      Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked  overlayfs. Quentin Casasnovas <quentin.casasnovas@oracle.com> - 2016-11-25 20:30 +0100
        Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked  overlayfs. Miklos Szeredi <miklos@szeredi.hu> - 2016-11-28 10:50 +0100
          Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked  overlayfs. Quentin Casasnovas <quentin.casasnovas@oracle.com> - 2016-11-28 12:10 +0100

#1530310 — opening a file on a stacked overlayfs is broken.

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1530442 — [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs.

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 <viro@zeniv.linux.org.uk>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1530499 — Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs.

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1531160 — Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs.

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 */

[toc] | [prev] | [next] | [standalone]

#1531213 — Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs.

[Multipart message — attachments visible in raw view] — view raw

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 <quentin.casasnovas@oracle.com>

Q

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web