Date:	Thu, 7 Jun 2012 13:43:29 -0700 (PDT)
From:	Sage Weil <sage@...tank.com>
To:	Al Viro <viro@...IV.linux.org.uk>
cc:	Dave Jones <davej@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Miklos Szeredi <mszeredi@...e.cz>, Jan Kara <jack@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-fsdevel@...r.kernel.org,
	"J. Bruce Fields" <bfields@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>, elder@...tank.com
Subject: Re: processes hung after sys_renameat, and 'missing' processes

Hi Al,

On Thu, 7 Jun 2012, Al Viro wrote:
> [maintainers of assorted code involved Cc'd]
> 
> On Thu, Jun 07, 2012 at 02:29:00AM +0100, Al Viro wrote:
> > On Wed, Jun 06, 2012 at 09:19:15PM -0400, Dave Jones wrote:
> > > On Wed, Jun 06, 2012 at 05:42:35PM -0700, Linus Torvalds wrote:
> > > 
> > >  > So Al meant you to test mutex_is_locked(dentry->d_inode->i_mutex) of
> > >  > the parents.
> > > 
> > > ok, I ended up with..
> > > 
> > >         WARN_ON_ONCE(!mutex_is_locked(&target->d_parent->d_inode->i_mutex));
> > > 
> > >         if (dentry->d_parent != NULL)
> > 			       != dentry)
> > 
> > ->d_parent *never* should be NULL; when dentry is disconnected from the
> > tree (or hadn't been connected to it yet), it points to that dentry itself.
> 
> BTW, I've started to put together some documentation on ->d_parent use.
> And already have found idiocies.
> 
> 0) *all* instances of struct dentry should come from __d_alloc(); never
> embed it into another object or declare variables of type struct dentry.
> AFAICS, that one is satisfied now; any out-of-tree code violating that
> deserves to be broken and *will* be broken.
> 
> 1) For all code outside of fs/dcache.c it's read-only.  Never change it
> directly, period.  Again, satisfied in mainline, out-of-tree code can
> go play in the traffic, for all I care.
> 
> 2) Places that set ->d_parent: __d_alloc(), d_alloc(), __d_move(),
> __d_materialize_dentry().  The first two are doing that to new instance
> of struct dentry with no references to it anywhere in shared data
> structures.  The other two have had dentry passed to dentry_lock_for_move().
> 
> 3) d_alloc(NULL, name) should never happen (will instantly oops in that case).
> 
> 4) ->d_parent is *never* NULL; the only way to stumble across a dentry with
> that not being true is to crawl through the slab page directly and to run
> into an object that has been allocated by not initialized yet.  Don't do
> that (nothing in mainline does, thanks $DEITY).  "No parent" == "dentry
> has ->d_parent pointing to dentry itself".  There's a helper checking that,
> albeit with slightly misleading name - IS_ROOT(dentry).  Doesn't have to
> be fs root - anything still not attached to the tree or already detached
> from it will be that way.  Any code checking that ->d_parent is NULL is
> almost certainly a bug.  And we have a bunch of such places in fs/ceph:
> 	ceph_init_dentry()
> 	ceph_d_prune()
> 	ceph_mdsc_build_path()

The ceph_init_dentry check was added in 92cf7652 in response to getting a 
dentry with null d_parent out of d_obtain_alias().  I suspect it was 
papering over an old bug in that case.  Pushed a patch to the ceph tree to 
clean these checks out, see f3722944a8565edf434ee44bdfa37715ae0d91cd in
git://git.kernel.org/pub/scm/linux/kernel/git/sage/ceph-client.git.  Let 
me know if you want to carry it, otherwise I'll push it in the next 
window.

> a few in fs/cifs:
> 	build_path_from_dentry()
> complete idiocy in fs/debugfs (not only check for NULL ->d_parent, but
> one for ->d_parent being a negative dentry; also can't happen).
> 	debugfs_remove()
> 	debugfs_remove_recursive()
> one in fs/ocfs2:
> 	ocfs2_match_dentry()
> and one in security/inode.c:
> 	securityfs_remove() (nicked from debugfs, at a guess).
> Some of those guys can be simply dropped, but I really wonder what
> ceph_init_dentry/ceph_d_prune intended there.
> 
> Incidentall, build_path_from_dentry() ought to be lifted into fs/dcache.c;
> we have at least three copies and one of them is not an instant roothole
> only because hppfs doesn't follow underlying procfs changes.  And I'm going
> to export is_subdir() - at least gfs2 has grown itself a b0rken copy...
> 
> 5) all assignments to dentry->d_parent after initialization are under
> dentry->d_lock; dentry_lock_for_move() takes care of that.
> 
> 6) all reassignments to dentry->d_parent have ->d_lock held on old and new
> parent.  Again, dentry_lock_for_move() takes care of that and so does
> d_alloc() (that one - for new parent only; old parent is dentry itself
> here and nothing could have seen it yet).
> 
> 7) attached dentry moving away from old parent must have ->i_mutex on
> that old parent held.
> 
> 8) dentry getting attached to a new parent must have ->i_mutex held on
> that new parent.
> 
> 9) attached dentry getting moved to a new parent must have ->s_vfs_rename_mutex
> held.
> 
> Now, (7--9) are interesting; the call graph looks so:
> __d_move()
> 	<- d_move()
> 	<- __d_unalias()
> 		<- d_materialise_unique()
> __d_materialise_dentry()
> 	<- d_materialise_unique()
> 
> rename-related callers of d_move() (vfs_rename_dir/vfs_rename_other/
> nfs_rename/ocfs2_dentry_move from ocfs2_rename/ceph_rename/v9fs_vfs_rename)
> share the same locking environment; we have both parents + ->s_vfs_rename_mutex
> in case of cross-directory move held.  Pretty much by an accident we have
> one difference between vfs_rename_dir and vfs_rename_other - holding ->i_mutex
> on *victim* of overwriting rename() is not something d_move() cares about.
> We can make that more uniform, but that's really unrelated to that.
> Note that in all these cases both dentries are guaranteed to be attached
> all along.
> 
> Other callers of d_move():
> 	vfat_lookup() does d_move() of an alias; the validity of that
> strongly relies on the lack of anything resembling hardlinks on VFAT;
> the parents will be the same (and parent of target is locked).
> 	sysfs_lookup() plays insane games with d_move(); I _really_
> don't like the look of that code.  It tries to play catch-up after
> sysfs_rename() done its thing.  AFAICS, the parent *can* be changed
> by sysfs_rename().  No lock is held on said parent here; no
> ->s_vfs_rename_mutex either.  Moreover, none of the sysfs_rename() callers
> (kobject_move(), etc.) appear to give a damn about checking if it would
> create a loop.
> 	* debugfs_rename() - imitates what vfs_rename() is doing.  Same
> locking environment.  BTW,
>         trap = lock_rename(new_dir, old_dir);
>         /* Source or destination directories don't exist? */
>         if (!old_dir->d_inode || !new_dir->d_inode)
>                 goto exit;
> is bogus - lock_rename() is taking ->i_mutex on these inodes, for fsck sake!
> If this can be called with old_dir or new_dir negative, it's buggered.
> 	* ceph_fill_trace() in CEPH_MDS_OP_RENAME case.  Seems to be
> done while ceph_rename() is being executed; i.e. the usual locking
> environment holds.  No idea why it feels the need to grab references
> to old_dentry and new_dentry in ceph_rename(), though - if that thing
> can outlive ceph_rename(), we have a bad trouble.  And if it can't, there's
> no reasons to pin them down that way.

The references are owned by the request struct, which can outlive 
ceph_rename() in the case of ^C or whatever.  When that happens, the 
request is marked aborted and the ceph_fill_trace() code doesn't run 
(since the locks are no longer held).  It still needs the refs to maintain 
its own sanity, however.

> 	* d_splice_alias() - broken.  Called without any locking on the
> old directory, nevermind ->s_vfs_rename_mutex.  I really believe that
> this pair of commits needs to be reverted.  The earlier code used to
> guarantee that alias would be detached.
> 
> d_materialise_unique(dentry, inode) must be called with dentry->d_parent
> having its ->d_inode->i_mutex held.  AFAICS, that's satisfied for all
> callers - they come either from ->lookup() or from ->readdir(), both
> having the parent inode locked.  ceph is a possible exceptions; it's
> damn hard to trace back to the places triggering ceph_fill_trace() in
> general ;-/

I know, sorry about that.  :)

ceph_fill_trace() runs on the struct ceph_mds_request, which as an 
r_locked_dir member that is verified before calling splice_dentry() -> 
d_materialise_unique().  I just double-checked and it looks like all call 
sites are safe.

If ceph_rename() (or a similar method who is holding the dir i_mutex) 
aborts, ceph_mdsc_do_request() marks the aborted flag under r_fill_mutex 
to avoid races with ceph_fill_trace(), so we can rely on those locks being 
held for the duration.

Does that clear things up?
sage


> 
> __d_unalias(), in case we are asked for cross-directory move, grabs
> ->s_vfs_rename_mutex first (with trylock).  That makes ->d_parent of
> alias stable; then it grabs ->i_mutex on that parent (again with trylock)
> and we are all set.
> 
> __d_materialise_dentry(dentry, alias) is called only when alias is
> unattached.  That looks like potential race, BTW - what's to prevent it
> from becoming attached right under us?  We do have __d_materialise_dentry()
> and __d_ualias serialized by alias->d_inode->i_lock, but... there's another
> way for that unattached alias to get attached - d_splice_alias().  And
> it's done without ->i_lock there.  OTOH, it's not a problem to extend
> the protection of ->i_lock to that spot as well.
> 
> I'll continue this series in a couple of hours - there's more.  In the
> meanwhile, I'd like to hear from ceph, sysfs and nfsd folks - see above
> for the reasons.
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists