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

Groups > linux.kernel > #1532051 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked  overlayfs. Miklos Szeredi <miklos@szeredi.hu> - 2016-11-29 10:40 +0100
    Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs. Amir Goldstein <amir73il@gmail.com> - 2016-11-29 11:10 +0100
    Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked  overlayfs. Quentin Casasnovas <quentin.casasnovas@oracle.com> - 2016-12-01 17:20 +0100

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

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 <mszeredi@redhat.com>
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 <quentin.casasnovas@oracle.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
Cc: <stable@vger.kernel.org> # 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);

[toc] | [next] | [standalone]

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

On Tue, Nov 29, 2016 at 11:32 AM, Miklos Szeredi <miklos@szeredi.hu> 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 <mszeredi@redhat.com>
> 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 <quentin.casasnovas@oracle.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()")
> Cc: <stable@vger.kernel.org> # 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1534251

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

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

Thanks Miklos!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web