From patchwork Thu Mar 17 15:23:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 8612281 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 1C12CC0553 for ; Thu, 17 Mar 2016 15:22:10 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id EE5F12034B for ; Thu, 17 Mar 2016 15:22:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 81EDA2034A for ; Thu, 17 Mar 2016 15:22:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031389AbcCQPWB (ORCPT ); Thu, 17 Mar 2016 11:22:01 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33021 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031360AbcCQPVu (ORCPT ); Thu, 17 Mar 2016 11:21:50 -0400 Received: by mail-wm0-f67.google.com with SMTP id x188so7372670wmg.0 for ; Thu, 17 Mar 2016 08:21:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=YhKXZO35GZGrGapgEMs4WcA4FGFd2k0tJi+PlBIQ2oE=; b=IRNuUas0XYseeqe7mOv8zkXS/jXIqzQH/8JUpyGmab5sA6vsqvHIRAZPbOD9/nOwgV YXnzJ2wL+zLt6o0VUoE33p3fCHDVAvnkfIwkoI7g2l4PY4U/jzD6jcXXa+qml6ulsdTs M0GsDJiwBQIvDfBXz9RDSqSv60uLOldxgnZ6Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=YhKXZO35GZGrGapgEMs4WcA4FGFd2k0tJi+PlBIQ2oE=; b=YGmUhytZ3RmF/60FREBp2qgStuwpJACmDE1z0TH0ucdJb+hthRBQAJE15dQGa9S5N6 03m9cCKV6hxDDLvNzMJ789XsyjLpdNPvTpNZXhr0notmOCIID+osqJ1EFDiKgMD8fmGq bCcsbOHStq7R9gwn7pgQztmATninNsKrV4Mm+U/uBXCav6HrMxM9+WpWWXitH9IN4l4q qRFNPuZVhJWC4fYXoNBZ13yGu0DFuFgsFdvrSB6yi4668bbEXrRIumV3QmG6d0T819kL zG9ie/7Z8MiYxhDk16f8XmjQrgieUIJZkS7SmpITCeSv33bPOwIT130qLZjaMf+vsgfC a91A== X-Gm-Message-State: AD7BkJJXKhYF5BSLlthmh92GHVaFzF4umQN2oHnG0u/lFhfQnAI12nbErzo1HtHtJeJIpg== X-Received: by 10.28.126.4 with SMTP id z4mr36906898wmc.29.1458228106771; Thu, 17 Mar 2016 08:21:46 -0700 (PDT) Received: from tucsk (pool-dsl-2c-0018.externet.hu. [217.173.44.24]) by smtp.gmail.com with ESMTPSA id w133sm8613565wmd.3.2016.03.17.08.21.44 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 17 Mar 2016 08:21:45 -0700 (PDT) Date: Thu, 17 Mar 2016 16:23:01 +0100 From: Miklos Szeredi To: Ignacy =?utf-8?B?R2F3xJlkemtp?= , Vivek Goyal , linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, David Howells Subject: Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup. Message-ID: <20160317152301.GQ8655@tucsk> References: <20160224135552.GB8422@zenon.in.qult.net> <20160226194143.GB13054@redhat.com> <20160228110942.GA19509@zenon.in.qult.net> <20160229162546.GA3335@redhat.com> <20160229165440.GA28299@zenon.in.qult.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160229165440.GA28299@zenon.in.qult.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 29, 2016 at 05:54:40PM +0100, Ignacy Gaw?dzki wrote: > On Mon, Feb 29, 2016 at 11:25:46AM -0500, thus spake Vivek Goyal: > > I agree that semantics should be more consistent. I don't know that > > if upper layer should override lower layer checks or not. > > > > One could also argue that if root did chown, then changes effectively > > happened in upper layer and anything in upper layer should become > > visible to unpriviliged user but not the one in lower layer. > > > > I just don't know. I guess those who have more background on this > > could pitch in and clarify that was is supposed to be the design > > intention. I haven't thought about this aspect yet. [thinking] We cannot actually respect lower layer search permissions in a merged directory, because we cannot selectively allow or deny directory search permissions on individual directory entries (there's simply not enough information in the ->permission() call). The only sane thing to do is to override search permission by the upper layer permissions. We can do that with the cap_raise(... CAP_DAC_OVERRIDE) thing, which is what overlayfs usually does when it needs elevated permissions to do something. Or we can a special lookup_one_len() type helper. But we already have the dentry hash in the overlay dentry, so no need to recalculate that (we've also made sure that underlying fs doesnt have ->d_hash()). The sanity checks on 'name' are also unnecessary. And that leaves us with __lookup_hash() where we can just get rid of the underscores and export to modules. Attaching folded patch. Will post split out ones for review. Thanks, Miklos --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/namei.c b/fs/namei.c index 9c590e0f66e9..80fe2a68f361 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1447,7 +1447,7 @@ static int follow_dotdot(struct nameidata *nd) * * dir->d_inode->i_mutex must be held */ -static struct dentry *lookup_dcache(struct qstr *name, struct dentry *dir, +static struct dentry *lookup_dcache(const struct qstr *name, struct dentry *dir, unsigned int flags, bool *need_lookup) { struct dentry *dentry; @@ -1506,8 +1506,8 @@ static struct dentry *lookup_real(struct inode *dir, struct dentry *dentry, return dentry; } -static struct dentry *__lookup_hash(struct qstr *name, - struct dentry *base, unsigned int flags) +struct dentry *lookup_hash(const struct qstr *name, + struct dentry *base, unsigned int flags) { bool need_lookup; struct dentry *dentry; @@ -1518,6 +1518,7 @@ static struct dentry *__lookup_hash(struct qstr *name, return lookup_real(base->d_inode, dentry, flags); } +EXPORT_SYMBOL(lookup_hash); /* * It's more convoluted than I'd like it to be, but... it's still fairly @@ -1630,7 +1631,7 @@ static int lookup_slow(struct nameidata *nd, struct path *path) BUG_ON(nd->inode != parent->d_inode); inode_lock(parent->d_inode); - dentry = __lookup_hash(&nd->last, parent, nd->flags); + dentry = lookup_hash(&nd->last, parent, nd->flags); inode_unlock(parent->d_inode); if (IS_ERR(dentry)) return PTR_ERR(dentry); @@ -2235,7 +2236,7 @@ struct dentry *kern_path_locked(const char *name, struct path *path) return ERR_PTR(-EINVAL); } inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); - d = __lookup_hash(&last, path->dentry, 0); + d = lookup_hash(&last, path->dentry, 0); if (IS_ERR(d)) { inode_unlock(path->dentry->d_inode); path_put(path); @@ -2319,7 +2320,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) if (err) return ERR_PTR(err); - return __lookup_hash(&this, base, 0); + return lookup_hash(&this, base, 0); } EXPORT_SYMBOL(lookup_one_len); @@ -2386,7 +2387,7 @@ struct dentry *lookup_one_len_unlocked(const char *name, return ret; inode_lock(base->d_inode); - ret = __lookup_hash(&this, base, 0); + ret = lookup_hash(&this, base, 0); inode_unlock(base->d_inode); return ret; } @@ -3498,7 +3499,7 @@ static struct dentry *filename_create(int dfd, struct filename *name, */ lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); - dentry = __lookup_hash(&last, path->dentry, lookup_flags); + dentry = lookup_hash(&last, path->dentry, lookup_flags); if (IS_ERR(dentry)) goto unlock; @@ -3803,7 +3804,7 @@ retry: goto exit1; inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT); - dentry = __lookup_hash(&last, path.dentry, lookup_flags); + dentry = lookup_hash(&last, path.dentry, lookup_flags); error = PTR_ERR(dentry); if (IS_ERR(dentry)) goto exit2; @@ -3925,7 +3926,7 @@ retry: goto exit1; retry_deleg: inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT); - dentry = __lookup_hash(&last, path.dentry, lookup_flags); + dentry = lookup_hash(&last, path.dentry, lookup_flags); error = PTR_ERR(dentry); if (!IS_ERR(dentry)) { /* Why not before? Because we want correct error value */ @@ -4443,7 +4444,7 @@ retry: retry_deleg: trap = lock_rename(new_path.dentry, old_path.dentry); - old_dentry = __lookup_hash(&old_last, old_path.dentry, lookup_flags); + old_dentry = lookup_hash(&old_last, old_path.dentry, lookup_flags); error = PTR_ERR(old_dentry); if (IS_ERR(old_dentry)) goto exit3; @@ -4451,7 +4452,7 @@ retry_deleg: error = -ENOENT; if (d_is_negative(old_dentry)) goto exit4; - new_dentry = __lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags); + new_dentry = lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags); error = PTR_ERR(new_dentry); if (IS_ERR(new_dentry)) goto exit4; diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index d894e7cd9a86..0cf45a0e904e 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -220,8 +220,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, if (IS_ERR(newdentry)) goto out; - upper = lookup_one_len(dentry->d_name.name, upperdir, - dentry->d_name.len); + upper = lookup_hash(&dentry->d_name, upperdir, 0); err = PTR_ERR(upper); if (IS_ERR(upper)) goto out1; diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 52f6de5d40a9..888b5fe29fdf 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -168,8 +168,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, int err; inode_lock_nested(udir, I_MUTEX_PARENT); - newdentry = lookup_one_len(dentry->d_name.name, upperdir, - dentry->d_name.len); + newdentry = lookup_hash(&dentry->d_name, upperdir, 0); err = PTR_ERR(newdentry); if (IS_ERR(newdentry)) goto out_unlock; @@ -337,8 +336,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, if (IS_ERR(newdentry)) goto out_unlock; - upper = lookup_one_len(dentry->d_name.name, upperdir, - dentry->d_name.len); + upper = lookup_hash(&dentry->d_name, upperdir, 0); err = PTR_ERR(upper); if (IS_ERR(upper)) goto out_dput; @@ -547,8 +545,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir) upper = ovl_dentry_upper(dentry); if (!upper) { - upper = lookup_one_len(dentry->d_name.name, upperdir, - dentry->d_name.len); + upper = lookup_hash(&dentry->d_name, upperdir, 0); err = PTR_ERR(upper); if (IS_ERR(upper)) goto kill_whiteout; @@ -851,8 +848,7 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old, } } else { new_create = true; - newdentry = lookup_one_len(new->d_name.name, new_upperdir, - new->d_name.len); + newdentry = lookup_hash(&new->d_name, new_upperdir, 0); err = PTR_ERR(newdentry); if (IS_ERR(newdentry)) goto out_unlock; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 619ad4b016d2..c16eeb3e5021 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -379,7 +379,7 @@ static inline struct dentry *ovl_lookup_real(struct dentry *dir, struct dentry *dentry; inode_lock(dir->d_inode); - dentry = lookup_one_len(name->name, dir, name->len); + dentry = lookup_hash(name, dir, 0); inode_unlock(dir->d_inode); if (IS_ERR(dentry)) { diff --git a/include/linux/namei.h b/include/linux/namei.h index d0f25d81b46a..22cc2cece000 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -78,6 +78,8 @@ extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int); extern struct dentry *lookup_one_len(const char *, struct dentry *, int); extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int); +struct qstr; +extern struct dentry *lookup_hash(const struct qstr *, struct dentry *, unsigned int); extern int follow_down_one(struct path *); extern int follow_down(struct path *);