From patchwork Mon Oct 8 04:25:23 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Amir Goldstein X-Patchwork-Id: 10629969 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id BF49513BB for ; Mon, 8 Oct 2018 04:25:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A30E928C04 for ; Mon, 8 Oct 2018 04:25:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9400428C81; Mon, 8 Oct 2018 04:25:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F3F7928C04 for ; Mon, 8 Oct 2018 04:25:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725973AbeJHLfO (ORCPT ); Mon, 8 Oct 2018 07:35:14 -0400 Received: from mail-wm1-f68.google.com ([209.85.128.68]:51375 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725760AbeJHLfO (ORCPT ); Mon, 8 Oct 2018 07:35:14 -0400 Received: by mail-wm1-f68.google.com with SMTP id 143-v6so6677031wmf.1; Sun, 07 Oct 2018 21:25:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=tY/s/BlWKCF28msUHn+wZS0j8mWbUE4Wg1XdalavABo=; b=F1RxbFw8PajKAh06Uuu88pyxB6nej0hO/q2n1tCIX0hk1Kkauev/7X3wwuuazlWjZ3 UyfTINGBST1b3/0s0OCQ5fAK010R7BPUs77LdLC7yluUyJBAgls9+EukII0JD1J2QooN c10b1uGui0TULbfC6lVNcGryPaP266yGJuF+y9x11ybq2QjwqEgnjTe/RzP/Ab3h7uaV GrFZm+Qpgswpi4TOJvaBsnS6qMxDxHKqhVsJdXyC/eyY1lQx8UVxJYwOJqYmIaVKA7JL OwhgMe2hfovR8G2TfEBOxSqE6zNkePPmlIspVzPasiuExqciLDYZxymb4XczGI3dvQ+Q JLUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=tY/s/BlWKCF28msUHn+wZS0j8mWbUE4Wg1XdalavABo=; b=sMqCXESM2hNStFpBUhlF63ZvXvI1w0KagXN0ROtisOxOXcY8po60o1Y+4NQIshZ33p E2Fr5eEIM7yZCMEybvMM/P9jVOK9UCtGtU/ucbRVm8x6i6c+xDurWzBfvxqstHNVPsKD W1D0lf5uysdtMo8MEVAR5vByDkg4SP+oy5n+2YvTQMDcgEkfiPUk7y+eopxqLHNAgdtU cBzY7+DDxxHtvDKMYUheYIDHoJK7OWES8gJQ9zaua41JZALcUME6rAEUab14VCgZuF9n tZbf43BwHf/aCLDWORhF7ioQdRxDJUJTsYD+K1WKfPcwlf0jeSTwFACS06mCqjejGQ3Y xJqw== X-Gm-Message-State: ABuFfohw39S8YYkoHlCN3dfedJx7ZsIUwDYfL8imHYmLYNV17dkS0Ppu y9vLlID4Ywxq0TJfZPL1yU8= X-Google-Smtp-Source: ACcGV612NhV3aUS3mbkP3Awd/6bXpV1x5wNdaqkjKjfzHSQw5FSPx1Bf/ZXaRe06Plj2BilPAGVwLg== X-Received: by 2002:a1c:88cd:: with SMTP id k196-v6mr13461099wmd.17.1538972730779; Sun, 07 Oct 2018 21:25:30 -0700 (PDT) Received: from localhost.localdomain ([141.226.14.107]) by smtp.gmail.com with ESMTPSA id n11-v6sm18213326wra.26.2018.10.07.21.25.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 07 Oct 2018 21:25:29 -0700 (PDT) From: Amir Goldstein To: Miklos Szeredi Cc: Al Viro , linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: [PATCH v2] ovl: untangle copy up call chain Date: Mon, 8 Oct 2018 07:25:23 +0300 Message-Id: <20181008042523.4134-1-amir73il@gmail.com> X-Mailer: git-send-email 2.17.1 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP In an attempt to dedup ~100 LOC, we ended up creating a tangled call chain, whose branches merge and diverge in several points according to the immutable c->tmpfile copy up mode. This call chain was hard to analyse for locking correctness because the locking requirements for the c->tmpfile flow were very different from the locking requirements for the !c->tmpfile flow (i.e. directory vs. regulare file copy up). Split the copy up helpers of the c->tmpfile flow from those of the !c->tmpfile (i.e. workdir) flow and remove the c->tmpfile mode from copy up context. Suggested-by: Al Viro Signed-off-by: Amir Goldstein --- Miklos, I've managed to mess up error handling in 3 out of 6 helpers in v1. Better take a close look that I did not miss more... Thanks, Amir. fs/overlayfs/copy_up.c | 155 ++++++++++++++++++++++++++++++----------- 1 file changed, 114 insertions(+), 41 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 1cc797a08a5b..78a5049f7a5a 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -395,7 +395,6 @@ struct ovl_copy_up_ctx { struct dentry *destdir; struct qstr destname; struct dentry *workdir; - bool tmpfile; bool origin; bool indexed; bool metacopy; @@ -440,8 +439,14 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c) return err; } -static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, - struct dentry **newdentry) +/* + * Move temp file from workdir into place on upper dir. + * Used when copying up directories and when upper fs doesn't support O_TMPFILE. + * + * Caller must hold ovl_lock_rename_workdir(). + */ +static int ovl_rename_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, + struct dentry **newdentry) { int err; struct dentry *upper; @@ -451,19 +456,40 @@ static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, if (IS_ERR(upper)) return PTR_ERR(upper); - if (c->tmpfile) - err = ovl_do_link(temp, udir, upper); - else - err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0); + err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0); + if (!err) + *newdentry = dget(temp); + dput(upper); + return err; +} + +/* Link O_TMPFILE into place on upper dir */ +static int ovl_link_tmpfile(struct ovl_copy_up_ctx *c, struct dentry *temp, + struct dentry **newdentry) +{ + int err; + struct dentry *upper = NULL; + struct inode *udir = d_inode(c->destdir); + + inode_lock_nested(udir, I_MUTEX_PARENT); + + upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); + err = PTR_ERR(upper); + if (IS_ERR(upper)) + goto out; + + err = ovl_do_link(temp, udir, upper); if (!err) - *newdentry = dget(c->tmpfile ? upper : temp); + *newdentry = dget(upper); dput(upper); +out: + inode_unlock(udir); return err; } -static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) +static struct dentry *ovl_get_workdir_temp(struct ovl_copy_up_ctx *c) { int err; struct dentry *temp; @@ -484,10 +510,32 @@ static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) if (new_creds) old_creds = override_creds(new_creds); - if (c->tmpfile) - temp = ovl_do_tmpfile(c->workdir, c->stat.mode); - else - temp = ovl_create_temp(c->workdir, &cattr); + temp = ovl_create_temp(c->workdir, &cattr); +out: + if (new_creds) { + revert_creds(old_creds); + put_cred(new_creds); + } + + return temp; +} + +static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) +{ + int err; + struct dentry *temp; + const struct cred *old_creds = NULL; + struct cred *new_creds = NULL; + + err = security_inode_copy_up(c->dentry, &new_creds); + temp = ERR_PTR(err); + if (err < 0) + goto out; + + if (new_creds) + old_creds = override_creds(new_creds); + + temp = ovl_do_tmpfile(c->destdir, c->stat.mode); out: if (new_creds) { revert_creds(old_creds); @@ -548,37 +596,39 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) return err; } -static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) +/* + * Copyup using workdir to prepare temp file. + * Used when copying up directories and when upper fs doesn't support O_TMPFILE. + */ +static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) { - struct inode *udir = c->destdir->d_inode; struct inode *inode; struct dentry *newdentry = NULL; struct dentry *temp; int err; - temp = ovl_get_tmpfile(c); + err = ovl_lock_rename_workdir(c->workdir, c->destdir); + if (err) + return err; + + temp = ovl_get_workdir_temp(c); + err = PTR_ERR(temp); if (IS_ERR(temp)) - return PTR_ERR(temp); + goto out; err = ovl_copy_up_inode(c, temp); if (err) - goto out; + goto out_cleanup; if (S_ISDIR(c->stat.mode) && c->indexed) { err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp); if (err) - goto out; + goto out_cleanup; } - if (c->tmpfile) { - inode_lock_nested(udir, I_MUTEX_PARENT); - err = ovl_install_temp(c, temp, &newdentry); - inode_unlock(udir); - } else { - err = ovl_install_temp(c, temp, &newdentry); - } + err = ovl_rename_temp(c, temp, &newdentry); if (err) - goto out; + goto out_cleanup; if (!c->metacopy) ovl_set_upperdata(d_inode(c->dentry)); @@ -587,10 +637,41 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) if (S_ISDIR(inode->i_mode)) ovl_set_flag(OVL_WHITEOUTS, inode); -out: - if (err && !c->tmpfile) +out_cleanup: + if (err) ovl_cleanup(d_inode(c->workdir), temp); dput(temp); +out: + unlock_rename(c->workdir, c->destdir); + return err; + +} + +/* Copyup using O_TMPFILE which does not require cross dir locking */ +static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c) +{ + struct dentry *newdentry = NULL; + struct dentry *temp; + int err; + + temp = ovl_get_tmpfile(c); + if (IS_ERR(temp)) + return PTR_ERR(temp); + + err = ovl_copy_up_inode(c, temp); + if (err) + goto out; + + err = ovl_link_tmpfile(c, temp, &newdentry); + if (err) + goto out; + + if (!c->metacopy) + ovl_set_upperdata(d_inode(c->dentry)); + ovl_inode_update(d_inode(c->dentry), newdentry); + +out: + dput(temp); return err; } @@ -646,18 +727,10 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c) } /* Should we copyup with O_TMPFILE or with workdir? */ - if (S_ISREG(c->stat.mode) && ofs->tmpfile) { - c->tmpfile = true; - err = ovl_copy_up_locked(c); - } else { - err = ovl_lock_rename_workdir(c->workdir, c->destdir); - if (!err) { - err = ovl_copy_up_locked(c); - unlock_rename(c->workdir, c->destdir); - } - } - - + if (S_ISREG(c->stat.mode) && ofs->tmpfile) + err = ovl_copy_up_tmpfile(c); + else + err = ovl_copy_up_workdir(c); if (err) goto out;