From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751694AbdDCCVF (ORCPT ); Sun, 2 Apr 2017 22:21:05 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:49380 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751194AbdDCCVE (ORCPT ); Sun, 2 Apr 2017 22:21:04 -0400 Date: Mon, 3 Apr 2017 03:21:00 +0100 From: Al Viro To: Linus Torvalds Cc: Linux Kernel Mailing List , linux-fsdevel , Eric Biederman Subject: Re: [git pull] vfs fixes Message-ID: <20170403022059.GM29622@ZenIV.linux.org.uk> References: <20170402170148.GI29622@ZenIV.linux.org.uk> <20170403003045.GK29622@ZenIV.linux.org.uk> <20170403004314.GL29622@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Apr 02, 2017 at 05:58:41PM -0700, Linus Torvalds wrote: > I had to go and double-check that "DCACHE_DIRECTORY_TYPE" is what > d_can_lookup() actually checks, so _that_ part is perhaps a bit > subtle, and might be worth noting in that comment that you edited. > > So the real "rule" ends up being that we only ever look up things from > dentries of type DCACHE_DIRECTORY_TYPE set, and those had better have > DCACHE_RCUACCESS bit set. > > And the only reason path_init() only checks it for that case is that > nd->root and nd->pwd both have to be of type d_can_lookup(). > > Do we check that when we set it? I hope/assume we do. For chdir()/chroot()/pivot_root() it's done by LOOKUP_DIRECTORY in lookup flags; fchdir() is slightly different - there we check S_ISDIR of inode of opened file. Which is almost the same thing, except for kinda-sorta directories that have no ->lookup() - we have them for NFS referral points. It should be impossible to end up with one of those opened - not even with O_PATH; follow_managed() will be called and we'll either fail or cross into whatever ends up overmounting them. Still, it might be cleaner to turn that check into d_can_lookup(f.file->f_path.dentry) simply for consistency sake. The thing I really don't like is mntns_install(). With sufficiently nasty nfsroot setup it might be possible to end up with referral point as one's root/pwd; getting out of such state would be interesting... Smells like that place should be a solitary follow_down(), not a loop of follow_down_one(), but I want Eric's opinion on that one; userns stuff is weird. diff --git a/fs/dcache.c b/fs/dcache.c index 95d71eda8142..05550139a8a6 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1757,7 +1757,13 @@ static unsigned d_flags_for_inode(struct inode *inode) return DCACHE_MISS_TYPE; if (S_ISDIR(inode->i_mode)) { - add_flags = DCACHE_DIRECTORY_TYPE; + /* + * Any potential starting point of lookup should have + * DCACHE_RCUACCESS; currently directory dentries + * come from d_alloc() anyway, but it costs us nothing + * to enforce it here. + */ + add_flags = DCACHE_DIRECTORY_TYPE | DCACHE_RCUACCESS; if (unlikely(!(inode->i_opflags & IOP_LOOKUP))) { if (unlikely(!inode->i_op->lookup)) add_flags = DCACHE_AUTODIR_TYPE; diff --git a/fs/namei.c b/fs/namei.c index d41fab78798b..19dcf62133cc 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2145,6 +2145,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags) int retval = 0; const char *s = nd->name->name; + if (!*s) + flags &= ~LOOKUP_RCU; + nd->last_type = LAST_ROOT; /* if there are only slashes... */ nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT; nd->depth = 0; diff --git a/fs/namespace.c b/fs/namespace.c index cc1375eff88c..31ec9a79d2d4 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3467,6 +3467,7 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns) struct fs_struct *fs = current->fs; struct mnt_namespace *mnt_ns = to_mnt_ns(ns); struct path root; + int err; if (!ns_capable(mnt_ns->user_ns, CAP_SYS_ADMIN) || !ns_capable(current_user_ns(), CAP_SYS_CHROOT) || @@ -3484,15 +3485,14 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns) root.mnt = &mnt_ns->root->mnt; root.dentry = mnt_ns->root->mnt.mnt_root; path_get(&root); - while(d_mountpoint(root.dentry) && follow_down_one(&root)) - ; - - /* Update the pwd and root */ - set_fs_pwd(fs, &root); - set_fs_root(fs, &root); - + err = follow_down(&root); + if (likely(!err)) { + /* Update the pwd and root */ + set_fs_pwd(fs, &root); + set_fs_root(fs, &root); + } path_put(&root); - return 0; + return err; } static struct user_namespace *mntns_owner(struct ns_common *ns) diff --git a/fs/open.c b/fs/open.c index 949cef29c3bb..217b5db588c8 100644 --- a/fs/open.c +++ b/fs/open.c @@ -459,20 +459,17 @@ SYSCALL_DEFINE1(chdir, const char __user *, filename) SYSCALL_DEFINE1(fchdir, unsigned int, fd) { struct fd f = fdget_raw(fd); - struct inode *inode; int error = -EBADF; error = -EBADF; if (!f.file) goto out; - inode = file_inode(f.file); - error = -ENOTDIR; - if (!S_ISDIR(inode->i_mode)) + if (!d_can_lookup(f.file->f_path.dentry)) goto out_putf; - error = inode_permission(inode, MAY_EXEC | MAY_CHDIR); + error = inode_permission(file_inode(f.file), MAY_EXEC | MAY_CHDIR); if (!error) set_fs_pwd(current->fs, &f.file->f_path); out_putf: