From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-f195.google.com ([209.85.213.195]:46826 "EHLO mail-yb0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750722AbeELFOA (ORCPT ); Sat, 12 May 2018 01:14:00 -0400 Received: by mail-yb0-f195.google.com with SMTP id f3-v6so2517312ybg.13 for ; Fri, 11 May 2018 22:14:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180512012907.GK30522@ZenIV.linux.org.uk> References: <20180512012557.GJ30522@ZenIV.linux.org.uk> <20180512012907.GK30522@ZenIV.linux.org.uk> From: Amir Goldstein Date: Sat, 12 May 2018 08:13:59 +0300 Message-ID: Subject: Re: [PATCH][RFC] ovl_create_real(): make it cope with vfs_mkdir() safely To: Al Viro Cc: linux-fsdevel , Miklos Szeredi Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sat, May 12, 2018 at 4:29 AM, Al Viro wrote: > [this one really, really needs a review by overlayfs folks; > all I can promise is that it compiles.] > > vfs_mkdir() may succeed and leave the dentry passed to it unhashed and > negative. ovl_create_real() is the last caller breaking when that > happens. > > Make ovl_create_real() return the dentry to be used or ERR_PTR(-E...) > in case of error; usually that'll be the dentry passed to it, but > it might be a different one - result of lookup in case of vfs_mkdir() > leaving looking the inode, etc. up to the next lookup to come. > > In that case (as well as in case of an error), original dentry is > dropped. That simplifies the callers. Moreover, passing ERR_PTR() > as dentry leads to immediate return of the same ERR_PTR(). That > simplifies the callers even more. So in the beginning I had mixed feelings about the internal interface change.. it feels shady, but it does simplify the callers... but then I realized the correct way to simplify the callers would be a helper ovl_create_real_temp(), because in most of the call sites ovl_lookup_temp() is the argument to ovl_create_real(), so your patch shouldn't bother with it. With that in mind, I think your mkdir fix should go into ovl_do_mkdir(), hopefully, calling a vfs helper vfs_mkdir_hashed(), where all of the above, including ovl_create_real() pass in a struct dentry ** I can prep and test the ovl patches if you like. Thanks, Amir. > > Signed-off-by: Al Viro > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > index cdd8f8816d2a..e0939d69470f 100644 > --- a/fs/overlayfs/copy_up.c > +++ b/fs/overlayfs/copy_up.c > @@ -517,22 +517,14 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp) > if (new_creds) > old_creds = override_creds(new_creds); > > - if (c->tmpfile) { > + if (c->tmpfile) > temp = ovl_do_tmpfile(c->workdir, c->stat.mode); > - if (IS_ERR(temp)) > - goto temp_err; > - } else { > - temp = ovl_lookup_temp(c->workdir); > - if (IS_ERR(temp)) > - goto temp_err; > - > - err = ovl_create_real(d_inode(c->workdir), temp, &cattr, > - NULL, true); > - if (err) { > - dput(temp); > - goto out; > - } > - } > + else > + temp = ovl_create_real(d_inode(c->workdir), > + ovl_lookup_temp(c->workdir), > + &cattr, NULL, true); > + if (IS_ERR(temp)) > + goto temp_err; > err = 0; > *tempp = temp; > out: > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 839709c7803a..bc07bdac013f 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -114,13 +114,18 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir, > goto out; > } > > -int ovl_create_real(struct inode *dir, struct dentry *newdentry, > +struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry, > struct cattr *attr, struct dentry *hardlink, bool debug) > { > int err; > > - if (newdentry->d_inode) > - return -ESTALE; > + if (IS_ERR(newdentry)) > + return newdentry; > + > + if (newdentry->d_inode) { > + dput(newdentry); > + return ERR_PTR(-ESTALE); > + } > > if (hardlink) { > err = ovl_do_link(hardlink, dir, newdentry, debug); > @@ -132,6 +137,16 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry, > > case S_IFDIR: > err = ovl_do_mkdir(dir, newdentry, attr->mode, debug); > + if (!err && unlikely(d_unhashed(newdentry))) { > + struct dentry *d; > + d = lookup_one_len(newdentry->d_name.name, > + newdentry->d_parent, > + newdentry->d_name.len); > + dput(newdentry); > + if (IS_ERR(d)) > + return d; > + newdentry = d; > + } > break; > > case S_IFCHR: > @@ -157,7 +172,11 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry, > */ > err = -ENOENT; > } > - return err; > + if (err) { > + dput(newdentry); > + return ERR_PTR(err); > + } > + return newdentry; > } > > static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper, > @@ -218,20 +237,21 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, > struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent); > struct inode *udir = upperdir->d_inode; > struct dentry *newdentry; > - int err; > + int err = 0; > > if (!hardlink && !IS_POSIXACL(udir)) > attr->mode &= ~current_umask(); > > inode_lock_nested(udir, I_MUTEX_PARENT); > - newdentry = lookup_one_len(dentry->d_name.name, upperdir, > - dentry->d_name.len); > - err = PTR_ERR(newdentry); > - if (IS_ERR(newdentry)) > + newdentry = ovl_create_real(udir, > + lookup_one_len(dentry->d_name.name, > + upperdir, > + dentry->d_name.len), > + attr, hardlink, false); > + if (IS_ERR(newdentry)) { > + err = PTR_ERR(newdentry); > goto out_unlock; > - err = ovl_create_real(udir, newdentry, attr, hardlink, false); > - if (err) > - goto out_dput; > + } > > if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) { > /* Setting opaque here is just an optimization, allow to fail */ > @@ -239,9 +259,6 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, > } > > ovl_instantiate(dentry, inode, newdentry, !!hardlink); > - newdentry = NULL; > -out_dput: > - dput(newdentry); > out_unlock: > inode_unlock(udir); > return err; > @@ -280,15 +297,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry, > if (upper->d_parent->d_inode != udir) > goto out_unlock; > > - opaquedir = ovl_lookup_temp(workdir); > - err = PTR_ERR(opaquedir); > - if (IS_ERR(opaquedir)) > - goto out_unlock; > - > - err = ovl_create_real(wdir, opaquedir, > + opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir), > &(struct cattr){.mode = stat.mode}, NULL, true); > - if (err) > - goto out_dput; > + if (IS_ERR(opaquedir)) { > + err = PTR_ERR(opaquedir); > + goto out_unlock; > + } > > err = ovl_copy_xattr(upper, opaquedir); > if (err) > @@ -319,7 +333,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry, > > out_cleanup: > ovl_cleanup(wdir, opaquedir); > -out_dput: > dput(opaquedir); > out_unlock: > unlock_rename(workdir, upperdir); > @@ -380,20 +393,18 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > if (err) > goto out; > > - newdentry = ovl_lookup_temp(workdir); > - err = PTR_ERR(newdentry); > - if (IS_ERR(newdentry)) > - goto out_unlock; > - > upper = lookup_one_len(dentry->d_name.name, upperdir, > dentry->d_name.len); > err = PTR_ERR(upper); > if (IS_ERR(upper)) > - goto out_dput; > + goto out_unlock; > > - err = ovl_create_real(wdir, newdentry, cattr, hardlink, true); > - if (err) > - goto out_dput2; > + newdentry = ovl_create_real(wdir, ovl_lookup_temp(workdir), > + cattr, hardlink, true); > + > + err = PTR_ERR(newdentry); > + if (IS_ERR(newdentry)) > + goto out_dput; > > /* > * mode could have been mutilated due to umask (e.g. sgid directory) > @@ -442,9 +453,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > ovl_instantiate(dentry, inode, newdentry, !!hardlink); > newdentry = NULL; > out_dput2: > - dput(upper); > -out_dput: > dput(newdentry); > +out_dput: > + dput(upper); > out_unlock: > unlock_rename(workdir, upperdir); > out: > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index e0b7de799f6b..a618f530c74e 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -360,7 +360,7 @@ struct cattr { > umode_t mode; > const char *link; > }; > -int ovl_create_real(struct inode *dir, struct dentry *newdentry, > +struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry, > struct cattr *attr, > struct dentry *hardlink, bool debug); > int ovl_cleanup(struct inode *dir, struct dentry *dentry); > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index e8551c97de51..c5aee822fd6a 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -611,11 +611,13 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs, > goto retry; > } > > - err = ovl_create_real(dir, work, > + work = ovl_create_real(dir, work, > &(struct cattr){.mode = S_IFDIR | 0}, > NULL, true); > - if (err) > - goto out_dput; > + if (IS_ERR(work)) { > + err = PTR_ERR(work); > + goto out_err; > + } > > /* > * Try to remove POSIX ACL xattrs from workdir. We are good if: