Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756799AbcK2KD3 (ORCPT ); Tue, 29 Nov 2016 05:03:29 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34089 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753397AbcK2KDU (ORCPT ); Tue, 29 Nov 2016 05:03:20 -0500 MIME-Version: 1.0 In-Reply-To: <20161129093229.GD2622@veci.piliscsaba.szeredi.hu> References: <20161129093229.GD2622@veci.piliscsaba.szeredi.hu> From: Amir Goldstein Date: Tue, 29 Nov 2016 12:03:18 +0200 Message-ID: Subject: Re: [PATCH] ovl: tentative fix for broken vfs_open() on stacked overlayfs. To: Miklos Szeredi Cc: Quentin Casasnovas , linux-kernel , linux-unionfs@vger.kernel.org, Al Viro Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3291 Lines: 96 On Tue, Nov 29, 2016 at 11:32 AM, 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. > > Thanks, > Miklos > --- > > From: Miklos Szeredi > 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 > Signed-off-by: Miklos Szeredi > Fixes: 2d902671ce1c ("vfs: merge .d_select_inode() into .d_real()") > Cc: # 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