Ocfs2 devel - May 2019 - [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

Hi Wengang?


After checking out your further elaboration,? all my concerns are cleared.

I truly had a overlook on your patch then ...


Thanks?

Changwei


On 2019/5/9 12:32 ??, Wengang Wang wrote:
>
> On 2019/5/7 19:06, Changwei Ge wrote:
>> Hi Wengang,
>>
>>
>> I think this version might need be improved.
>>
>>
>> On 2019/5/8 2:52 ??, Wengang Wang wrote:
>>> ocfs2_dentry_attach_lock() can be executed in parallel threads 
>>> against the
>>> same dentry. Make that race safe.
>>> The race is like this:
>>>
>>> ????????????? thread A?????????????????????????????? thread B
>>>
>>> (A1) enter ocfs2_dentry_attach_lock,
>>> seeing dentry->d_fsdata is NULL,
>>> and no alias found by
>>> ocfs2_find_local_alias, so kmalloc
>>> a new ocfs2_dentry_lock structure
>>> to local variable "dl", dl1
>>>
>>> ???????????????? .....
>>>
>>> ????????????????????????????????????? (B1) enter 
>>> ocfs2_dentry_attach_lock,
>>> ????????????????????????????????????? seeing dentry->d_fsdata is
NULL,
>>> ????????????????????????????????????? and no alias found by
>>> ????????????????????????????????????? ocfs2_find_local_alias so
kmalloc
>>> ????????????????????????????????????? a new ocfs2_dentry_lock
structure
>>> ????????????????????????????????????? to local variable
"dl", dl2.
>>>
>>> ???????????????????????????????????????????????????? ......
>>>
>>> (A2) set dentry->d_fsdata with dl1,
>>> call ocfs2_dentry_lock() and increase
>>> dl1->dl_lockres.l_ro_holders to 1 on
>>> success.
>>> ??????????????? ......
>>>
>>> ????????????????????????????????????? (B2) set dentry->d_fsdata
with
>>> dl2
>>> ????????????????????????????????????? call ocfs2_dentry_lock() and 
>>> increase
>>> ??????????????????? dl2->dl_lockres.l_ro_holders to 1 on
>>> ??????????????????? success.
>>>
>>> ??????????????????????????????????????????????????? ......
>>>
>>> (A3) call ocfs2_dentry_unlock()
>>> and decrease
>>> dl2->dl_lockres.l_ro_holders to 0
>>> on success.
>>> ?????????????? ....
>>>
>>> ????????????????????????????????????? (B3) call
ocfs2_dentry_unlock(),
>>> ????????????????????????????????????? decreasing
>>> ??????????????????? dl2->dl_lockres.l_ro_holders, but
>>> ??????????????????? see it's zero now, panic
>>>
>>> Signed-off-by: Wengang Wang <wen.gang.wang at oracle.com>
>>> ---
>>> v2: 1) removed lock on dentry_attach_lock at the first access of
>>> ???????? dentry->d_fsdata since it helps very little.
>>> ????? 2) do cleanups before freeing the duplicated dl
>>> ????? 3) return after freeing the duplicated dl found.
>>> ---
>>> ?? fs/ocfs2/dcache.c | 16 ++++++++++++++--
>>> ?? 1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>>> index 290373024d9d..0d220a66297e 100644
>>> --- a/fs/ocfs2/dcache.c
>>> +++ b/fs/ocfs2/dcache.c
>>> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry
*dentry,
>>> ?????? int ret;
>>> ?????? struct dentry *alias;
>>> ?????? struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>> +??? struct ocfs2_dentry_lock *dl_free_on_race = NULL;
>>> trace_ocfs2_dentry_attach_lock(dentry->d_name.len,
dentry->d_name.name,
>>> ????????????????????????? (unsigned long long)parent_blkno, dl);
>>> @@ -310,10 +311,21 @@ int ocfs2_dentry_attach_lock(struct dentry 
>>> *dentry,
>>> ?? ?? out_attach:
>>> ?????? spin_lock(&dentry_attach_lock);
>>> -??? dentry->d_fsdata = dl;
>>> -??? dl->dl_count++;
>>> +??? /* d_fsdata could be set by parallel thread */
>>> +??? if (unlikely(dentry->d_fsdata && !alias)) {
>>> +??????? dl_free_on_race = dl;
>>> +??? } else {
>>> +??????? dentry->d_fsdata = dl;
>>> +??????? dl->dl_count++;
>>> +??? }
>>> ?????? spin_unlock(&dentry_attach_lock);
>>> ?? +??? if (unlikely(dl_free_on_race)) {
>>> +??????? iput(dl_free_on_race->dl_inode);
>>> + ocfs2_lock_res_free(&dl_free_on_race->dl_lockres);
>>
>> I am afraid we are meeting a great risk here as we don't know if
the
>> another dentry lock code path is still using the being freed lock 
>> resource.
>
> Any access to this lock resource via dlm_debug->d_lockres_tracking 
> should be with ocfs2_dlm_tracking_lock locked. That lock protects the 
> lock resources well.? This is the design. If any access is without the 
> lock protection, it should take a reference of that lock resource thus 
> there should be a reference count field in ocfs2_lock_res structure. 
> But there is no such reference count there. It's not designed to 
> access the lock resource "offlock".
>
> The dl I am freeing is still local (though it's in d_lockres_tracking 
> list, I explained it's safe above). I didn't set it do
dentry->fs_data
> yet, so another dentry lock path can't see it at all so can't have
any
> access to it.
>
>
>> So another code path might use a freed lock resource.
> There is no such possibility.
>> This might be the
>> cause of the problem Daniel reported last night(in China).
> Nope, the problem that Daniel reported is just because I didn't remove 
> this lock resource from that list.
>
> I freed that dl (including the lock resource) leaving that lock 
> resource (freed memory) in d_lockres_tracking list. That memory then 
> is reused for other purpose causing the original "next" and
"prev"
> changed, leading the corruption on that list.
>
>
>>
>>
>>> +??????? kfree(dl_free_on_race);
>>> +??????? return 0;
>> Moreover, I think we can't directly return from here especially we
are
>> returning 0 here since we even don't increase dentry lock resource
>> holders counter. :(
> This hit the race case, means another path is doing the same thing. 
> That racing path would do the lock and then unlock stuff. This path 
> doesn't need to do the duplicated work.
>
> Returning 0 is the same behavior as the original code does at the very 
> beginning of ocfs2_dentry_attach_lock on seeing dentry->fs_data is not 
> NULL.
>
> Actually this function doesn't return with increased holder counter, 
> it lock (increase ro_holders by 1) and unlock immediately (decrease 
> ro_holders by 1), so the holder counter remains unchanged. The purpose 
> is not to lock but to get a chance (via callback 
> ocfs2_dentry_convert_worker) to get notified when the file is removed 
> by remote node.
>
> thanks,
> wengang
>
>>
>>
>> Thanks,
>>
>> Changwei
>>
>>> +??? }
>>> ?????? /*
>>> ??????? * This actually gets us our PRMODE level lock. From now on,
>>> ??????? * we'll have a notification if one of these names is