Re: [PATCH v2] ovl: fix dentry leak in ovl_get_redirect

[Liangyan <liangyan.peng@linux.alibaba.com>]

Exactly, i missed this definition of d_lock and treat it as a single 
member in dentry.
#define d_lock	d_lockref.lock

Thanks for the explanation. i will post a new patch as your suggestion.

Regards,
Liangyan

On 20/12/22 上午1:35, Al Viro wrote:
> On Tue, Dec 22, 2020 at 12:51:27AM +0800, Liangyan wrote:
>> This is the race scenario based on call trace we captured which cause the
>> dentry leak.
>>
>>
>>       CPU 0                                CPU 1
>> ovl_set_redirect                       lookup_fast
>>    ovl_get_redirect                       __d_lookup
>>      dget_dlock
>>        //no lock protection here            spin_lock(&dentry->d_lock)
>>        dentry->d_lockref.count++            dentry->d_lockref.count++
>>
>>
>> If we use dget_parent instead, we may have this race.
>>
>>
>>       CPU 0                                    CPU 1
>> ovl_set_redirect                           lookup_fast
>>    ovl_get_redirect                           __d_lookup
>>      dget_parent
>>        raw_seqcount_begin(&dentry->d_seq)      spin_lock(&dentry->d_lock)
>>        lockref_get_not_zero(&ret->d_lockref)   dentry->d_lockref.count++
> 
> And?
> 
> lockref_get_not_zero() will observe ->d_lock held and fall back to
> taking it.
> 
> The whole point of lockref is that counter and spinlock are next to each
> other.  Fastpath in lockref_get_not_zero is cmpxchg on both, and
> it is taken only if ->d_lock is *NOT* locked.  And the slow path
> there will do spin_lock() around the manipulations of ->count.
> 
> Note that ->d_lock is simply ->d_lockref.lock; ->d_seq has nothing
> to do with the whole thing.
> 
> The race in mainline is real; if you can observe anything of that
> sort with dget_parent(), we have much worse problem.  Consider
> dget() vs. lookup_fast() - no overlayfs weirdness in sight and the
> same kind of concurrent access.
> 
> Again, lockref primitives can be safely mixed with other threads
> doing operations on ->count while holding ->lock.
>