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

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Sep 29, 2014 at 08:15:51AM -0700, Linus Torvalds wrote:
> On Sun, Sep 28, 2014 at 11:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Folks, care to review and test the following?
> 
> No testing, but having thought about this some more, I'm personally
> getting quite convinced that doing the RCU delaying of the external
> name freeing in the __d_free() path is entirely pointless.
> 
> So I think the *only* rcu_free() you need is for just the "free old
> name" case in copy_name().
> 
> In __d_free(), the name pointer has gone through the same grace period
> that the dentry pointer itself went through. If it's not safe to free
> the external name, then it damn well wouldn't be safe to free the
> dentry itself either.
> 
> IOW, I think your games in __d_free() are totally unnecessary.
> 
> Now you can tell me why I'm wrong.

Sure.  Three dentries.  move the first over the second, dput the second.
The name is shared, the second dentry unhashed and the last reference
dropped.  free_dentry(second) does call_rcu(__d_free, ...).

Now RCU lookup starts.  And on another CPU we move the first dentry over
the third one.  copy_name() is called, it decrements the refcount down
to 1 (__d_free() hasn't happened yet) and doesn't schedule any freeing.

RCU lookup sees first dentry, still with the old name (what used to be the
name of the second dentry).

... and __d_free() on the second dentry is called.  call_rcu() has happened
before RCU lookup has done its rcu_read_lock(), so it's not required to
wait.  We decrement the refcount on ext name, it reaches 0 and we kfree()
it.  Right under dentry_cmp().  Oops...

We really need to decrement refcount before RCU-delaying.  IOW, the decrement
should be in dentry_free(), not in __d_free()...