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@xxxxxxxxxxxxxxxxxx> 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()... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
