Re: Q: spin_unlock(dentry) after lock_parent(dentry)

["J. R. Okajima" <hooanon05g@gmail.com>]

Al Viro,

I think I could find a clearer problematic scenario.
Can it happen in real world?

- /.../dirA/dirB/fileC exists.
- shrink_dentry_list() gets a list containing fileC and dirB, but not
  dirA
  + Round 1: fileC
    - shrink_dentry_list() gets d_parent which is dirB
    - dirB is DCACHE_SHRINK_LIST set, so __dentry_kill() is called but
      not dentry_free()
    - shrink_dentry_list() gets higher d_parent which is dirA
    - dirA is not DCACHE_SHRINK_LIST set, so both of __dentry_kill() and
      dentry_free() are is called
  + Round 2: dirB
    - shrink_dentry_list() tries getting d_parent which is dirA
    - dirA is freed by RCU. if RCU already activated __d_free(), BANG!


> In the case of concurrent dentry_kill() and shrink_dentry_list(),
> dentry_kill() kills the dentry and calls list_del(&dentry->d_u.d_child).
> lock_parent() (called by shrink_dentry_list()) doesn't check d_child
> nor DCACHE_DENTRY_KILLED. So shrink_dentry_list() may get a parent of a
> dead dentry and tries traversing via d_parent.
> Is it correct to track d_parent after list_del(&dentry->d_u.d_child)?
> Should lock_parent() consider DCACHE_DENTRY_KILLED too?
>
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -533,7 +533,8 @@ failed:
>  static inline struct dentry *lock_parent(struct dentry *dentry)
>  {
>  	struct dentry *parent = dentry->d_parent;
> -	if (IS_ROOT(dentry))
> +	if (IS_ROOT(dentry)
> +	    || (dentry->d_flags & DCACHE_DENTRY_KILLED))
>  		return NULL;
>  	if (likely(spin_trylock(&parent->d_lock)))
>  		return parent;


J. R. Okajima