Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

From: Paul E. McKenney
Date: Thu Oct 02 2014 - 01:38:58 EST


On Wed, Oct 01, 2014 at 01:16:15AM +0100, Al Viro wrote:
> 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.

Please accept my apologies for the noise!

Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/