From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:36110 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750793AbeELB3J (ORCPT ); Fri, 11 May 2018 21:29:09 -0400 Date: Sat, 12 May 2018 02:29:07 +0100 From: Al Viro To: linux-fsdevel@vger.kernel.org Cc: Miklos Szeredi Subject: [PATCH][RFC] ovl_create_real(): make it cope with vfs_mkdir() safely Message-ID: <20180512012907.GK30522@ZenIV.linux.org.uk> References: <20180512012557.GJ30522@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180512012557.GJ30522@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: [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. 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: