From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail06.adl6.internode.on.net ([150.101.137.145]:50672 "EHLO ipmail06.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750746AbeEJW4K (ORCPT ); Thu, 10 May 2018 18:56:10 -0400 Date: Fri, 11 May 2018 08:56:07 +1000 From: Dave Chinner To: Al Viro Cc: linux-fsdevel@vger.kernel.org, Linus Torvalds Subject: Re: [RFC][PATCH] do d_instantiate/unlock_new_inode combinations safely Message-ID: <20180510225607.GU23861@dastard> References: <20180510182058.GP30522@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180510182058.GP30522@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, May 10, 2018 at 07:20:58PM +0100, Al Viro wrote: > [in the spirit of "don't put 'em in without posting for review; the > this is present in vfs.git#for-linus, if you prefer to look in git. > > Background: a bunch of nfsd races fixes from back in 2008 had > problems with lockdep enabled; in 2012 that got "fixed", unfortunately > reopening a narrow race window. The patch below does *NOT* fix > all filesystems, but it does fix most of the exported local ones > and it is easy to backport, so it makes for a sane starting point. Do you have a pointer to the commits/test case for this? XFS has a fairly significant separation between unlock_new_inode() and dentry cache instantiation for some paths.... > If anyone has objections, this is your chance to yell. > ] > > For anything NFS-exported we do _not_ want to unlock new inode > before it has grown an alias; original set of fixes got the > ordering right, but missed the nasty complication in case of > lockdep being enabled - unlock_new_inode() does > lockdep_annotate_inode_mutex_key(inode) > which can only be done before anyone gets a chance to touch > ->i_mutex. Unfortunately, flipping the order and doing > unlock_new_inode() before d_instantiate() opens a window when > mkdir can race with open-by-fhandle on a guessed fhandle, leading > to multiple aliases for a directory inode and all the breakage > that follows from that. > > Correct solution: a new primitive (d_instantiate_new()) > combining these two in the right order - lockdep annotate, then > d_instantiate(), then the rest of unlock_new_inode(). All > combinations of d_instantiate() with unlock_new_inode() should > be converted to that. Ok, so this seems to touch only the paths that create new inodes (mkdir, mknod, etc). Is the lookup path that does: unlock_new_inode() ..... d_splice_alias(inode, dentry); OK? Cheers, Dave. -- Dave Chinner david@fromorbit.com