Message-ID: <20141001001615.GT7996@ZenIV.linux.org.uk>
Date:	Wed, 1 Oct 2014 01:16:15 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Mikhail Efremov <sem@...linux.org>
Subject: Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

On Mon, Sep 29, 2014 at 11:42:18AM -0700, Paul E. McKenney wrote:

> Assuming that incrementing the external name's reference count is
> atomic_add_unless, I could believe this part.  Or if you have some
> locking that makes it impossible to increment the reference count
> in any case where there is any risk that it might be decremented
> to zero, I guess.
> 
> Which might well be the pair of write_seqcount_begin() calls in __d_move(),
> now that I look more carefully.  So if the name has to be in use somewhere
> before it can be copied, then a copy can only be created if there is at
> least one copy that is not currently being removed?  If so, OK.

Huh?  copy_name() does copy a _reference_, not the name itself.  All the
copying involved is source->d_name.name = target->d_name.name.  And those
are simply unsigned char *.

write_seqcount_begin() is irrelevant here.  Look: all callers of
__d_move(x, y) are holding references both to x and y.  Contributing to
the refcount of dentries themselves, that is, not the names.

That gives exclusion between __d_move() and free_dentry() - the latter cannot
be called until dentry refcount reaches zero.  RCU is completely irrelevant
here.  In fact, no call chain leads to __d_move() under rcu_read_lock().
You must hold the target dentry hard, or it could simply be freed right
under you.

And __d_move() is taking ->d_lock on all dentries involved (in
addition to rename_lock serializing it system-wide).

What could possibly lead to refcount zero being observed on target of
__d_move()?  The history of any dentry is this:
	* it is created by __d_alloc().  Nobody can see it until __d_alloc()
returns.  Dentry refcount (not to be confused with refcount of external
name) is 1.
	* it passes through some (usually - zero) __d_move() calls.
Some - as the first argument, some - as the second one.  All those
calls are serialized by global seqlock - callers must hold rename_lock.
And all of them are done by somebody who is holding a counting reference
to dentries in question.
	* counting references to dentry might be taken and dropped;
eventually refcount reaches zero (under ->d_lock) and no further
counting references can be taken after that.  See __dentry_kill() - the
first thing it does is poisoning the refcount, so that any future
attempt to increment it would fail.  __dentry_kill() (still under ->d_lock
of dentry, ->d_lock of its parent and ->i_lock of its inode) removes
dentry from the tree, from hash and from the alias list of inode;
Then it drops the locks.  At that point the only search structure dentry
might be found in is shrink list; if it's not on such list, free_dentry()
is called immediately, otherwise it's marked so that the code processing
the shrink list in question would, as soon as it gets to that sucker,
remove it from the shrink list and call the same free_dentry().  And that's
the only thing done to such dentry by somebody finding it via a shrink list.
	* once free_dentry() has been reached, dentry can can be only seen
by RCU lookups, and after the grace period ends it gets physically freed.

free_dentry() isn't allowed to overlap __d_move(); to have that happen is
a serious dentry refcounting bug.  No __d_move() is allowed _after_
free_dentry() has been entered, either.  Again, it would take a refcounting
bug for dentries to have that happen - basically, double dput() somewhere.
If that happens, all bets are off, of course - if dentry gets unexpectedly
freed under somebody who has grabbed a reference to it and has not dropped
it yet, we are fucked.

Nothing outside of __d_move() is allowed to change ->d_name.name.  RCU-critical
code is allowed to fetch and dereference it, and such code relies upon
	a) freeing of name seen by somebody who'd done rcu_read_lock() being
delayed until after the matching rcu_read_unlock()
	b) store of terminating NUL done by __d_alloc() (and never overwritten
afterwards) being seen by RCU-critical code that has found the pointer to
that name in dentry->d_name.name

All other code accessing ->d_name.name is required to hold one of the locks
that are held by __d_move() and its callers.  Grabbing any of those leads
to smp_mb() on alpha, which serves as data dependency barrier there, so
we don't need explicit barrier there as we do in RCU-critical places - guarding
NUL will be seen.
--
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