MIME-Version: 1.0
In-Reply-To: <20161129093229.GD2622@veci.piliscsaba.szeredi.hu>
References: <20161129093229.GD2622@veci.piliscsaba.szeredi.hu>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 29 Nov 2016 12:03:18 +0200
Message-ID: <CAOQ4uxhmWkqdL83ohXK6XE=RvbE_zUvHSXipihAPnYvu3ecocA@mail.gmail.com>
Subject: Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs.
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-unionfs@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3291
Lines: 96

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