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

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

[Changwei Ge]

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@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.

So another code path might use a freed lock resource. This might be the 
cause of the problem Daniel reported last night(in China).


> +		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. :(


Thanks,

Changwei

> +	}
>   	/*
>   	 * This actually gets us our PRMODE level lock. From now on,
>   	 * we'll have a notification if one of these names is

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

[Changwei Ge]

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@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

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

[Changwei Ge]

On 2019/5/10 1:08 ??, Wengang Wang wrote:
> Hi Changwei,
>
> On 2019/5/8 18:52, Changwei Ge wrote:
>> Hi Wengang?
>>
>>
>> After checking out your further elaboration,? all my concerns are 
>> cleared.
>>
>> I truly had a overlook on your patch then ...
>
> Thank you for your active review! Let's wait for Daniel's test result.


OK. You can also add my

Reviewed-by:? Changwei Ge <gechangwei@live.cn>

together with Daniel's positive feedback(if any) :)


Thank you for your effort into this issue. I suppose Daniel has been 
reporting it since a long time ago.


Thanks,

Changwei


>
> thanks,
> wengang
>
>>
>>
>> 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@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

[Ocfs2-devel] [EXT] Re: [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

[Daniel Sobe]

Hi,

I'm running version 2 of the patch with the optional debug print enabled. Judging by that, the issue did not occur yet (no print so far). I will report any failures, or, after having seen the debug print for several times without any issues, success.

Thank you already for helping me out by finding the cause for this issue!

Regards,

Daniel

-----Original Message-----
From: ocfs2-devel-bounces@oss.oracle.com <ocfs2-devel-bounces@oss.oracle.com> On Behalf Of Changwei Ge
Sent: Freitag, 10. Mai 2019 03:47
To: Wengang Wang <wen.gang.wang@oracle.com>; ocfs2-devel at oss.oracle.com
Subject: [EXT] Re: [Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

Caution: EXT Email

On 2019/5/10 1:08 ??, Wengang Wang wrote:
> Hi Changwei,
>
> On 2019/5/8 18:52, Changwei Ge wrote:
>> Hi Wengang?
>>
>>
>> After checking out your further elaboration,  all my concerns are 
>> cleared.
>>
>> I truly had a overlook on your patch then ...
>
> Thank you for your active review! Let's wait for Daniel's test result.


OK. You can also add my

Reviewed-by:  Changwei Ge <gechangwei@live.cn>

together with Daniel's positive feedback(if any) :)


Thank you for your effort into this issue. I suppose Daniel has been reporting it since a long time ago.


Thanks,

Changwei


>
> thanks,
> wengang
>
>>
>>
>> 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@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
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://urldefense.proofpoint.com/v2/url?u=https-3A__eur01.safelinks.protection.outlook.com_-3Furl-3Dhttps-253A-252F-252Foss.oracle.com-252Fmailman-252Flistinfo-252Focfs2-2Ddevel-26amp-3Bdata-3D02-257C01-257Cdaniel.sobe-2540nxp.com-257Cbae8d5e6d6514212a5d008d6d4e9a37d-257C686ea1d3bc2b4c6fa92cd99c5c301635-257C0-257C0-257C636930497271173645-26amp-3Bsdata-3D-252F884d7KcV04xtrTR7mrMuB2M2mHo8ejUhDMnyL6jVOQ-253D-26amp-3Breserved-3D0&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=d-RKw7knw4QWlkAUlYmbZfjA3W-G-GbbfkpgBZFJSIY&s=lbtYvshqsTIlv0NdAlExkDmsGvAxeEAWK2Vk21M3c_g&e=

[Ocfs2-devel] [EXT] Re: [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

[Daniel Sobe]

Hi,

with several days of uptime and several occurrences of the debug print, now I am convinced that the issue is resolved. Thank you again! 

Regards,

Daniel

-----Original Message-----
From: Wengang Wang <wen.gang.wang@oracle.com> 
Sent: Montag, 13. Mai 2019 19:02
To: Daniel Sobe <daniel.sobe@nxp.com>
Cc: Changwei Ge <gechangwei@live.cn>; ocfs2-devel at oss.oracle.com
Subject: Re: [EXT] Re: [Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

Caution: EXT Email

Thank you Daniel for your effort on testing this!

Regards,
Wengang

On 2019/5/10 0:17, Daniel Sobe wrote:
> Hi,
>
> I'm running version 2 of the patch with the optional debug print enabled. Judging by that, the issue did not occur yet (no print so far). I will report any failures, or, after having seen the debug print for several times without any issues, success.
>
> Thank you already for helping me out by finding the cause for this issue!
>
> Regards,
>
> Daniel
>
> -----Original Message-----
> From: ocfs2-devel-bounces at oss.oracle.com 
> <ocfs2-devel-bounces@oss.oracle.com> On Behalf Of Changwei Ge
> Sent: Freitag, 10. Mai 2019 03:47
> To: Wengang Wang <wen.gang.wang@oracle.com>; 
> ocfs2-devel at oss.oracle.com
> Subject: [EXT] Re: [Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in 
> ocfs2_dentry_attach_lock
>
> Caution: EXT Email
>
> On 2019/5/10 1:08 ??, Wengang Wang wrote:
>> Hi Changwei,
>>
>> On 2019/5/8 18:52, Changwei Ge wrote:
>>> Hi Wengang?
>>>
>>>
>>> After checking out your further elaboration,  all my concerns are 
>>> cleared.
>>>
>>> I truly had a overlook on your patch then ...
>> Thank you for your active review! Let's wait for Daniel's test result.
>
> OK. You can also add my
>
> Reviewed-by:  Changwei Ge <gechangwei@live.cn>
>
> together with Daniel's positive feedback(if any) :)
>
>
> Thank you for your effort into this issue. I suppose Daniel has been reporting it since a long time ago.
>
>
> Thanks,
>
> Changwei
>
>
>> thanks,
>> wengang
>>
>>>
>>> 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@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
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://urldefense.proofpoint.com/v2/url?u=https-3A__eur01.safelinks.protection.outlook.com_-3Furl-3Dhttps-253A-252F-252Foss&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=Gz7_NR-FntEMsX_dGTWM6WAmtQzCzraf8N8rgZ5XxWI&s=3Aukp4l74jZb8W-KGtCtmXO9qC1azKFkWAXc_4wWc4w&e=.
> oracle.com%2Fmailman%2Flistinfo%2Focfs2-devel&amp;data=02%7C01%7Cdanie
> l.sobe%40nxp.com%7C1b23960ebdec48a5dd7808d6d7c4bc59%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C636933637309761294&amp;sdata=JTi6hQGsYS%2Fy
> rI16neiae8Q055Rg%2BLVaOLnG4BqxQyo%3D&amp;reserved=0

[Ocfs2-devel] [EXT] Re: [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock

[Wengang Wang]

Hi Daniel,

That's cool!? I going to resend the patch with Changwei's Reviewed-by 
and your Reported-by and Tested-by.

Thanks,
Wengang

On 2019/5/17 7:21, Daniel Sobe wrote:
> Hi,
>
> with several days of uptime and several occurrences of the debug print, now I am convinced that the issue is resolved. Thank you again!
>
> Regards,
>
> Daniel
>
> -----Original Message-----
> From: Wengang Wang <wen.gang.wang@oracle.com>
> Sent: Montag, 13. Mai 2019 19:02
> To: Daniel Sobe <daniel.sobe@nxp.com>
> Cc: Changwei Ge <gechangwei@live.cn>; ocfs2-devel at oss.oracle.com
> Subject: Re: [EXT] Re: [Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock
>
> Caution: EXT Email
>
> Thank you Daniel for your effort on testing this!
>
> Regards,
> Wengang
>
> On 2019/5/10 0:17, Daniel Sobe wrote:
>> Hi,
>>
>> I'm running version 2 of the patch with the optional debug print enabled. Judging by that, the issue did not occur yet (no print so far). I will report any failures, or, after having seen the debug print for several times without any issues, success.
>>
>> Thank you already for helping me out by finding the cause for this issue!
>>
>> Regards,
>>
>> Daniel
>>
>> -----Original Message-----
>> From: ocfs2-devel-bounces at oss.oracle.com
>> <ocfs2-devel-bounces@oss.oracle.com> On Behalf Of Changwei Ge
>> Sent: Freitag, 10. Mai 2019 03:47
>> To: Wengang Wang <wen.gang.wang@oracle.com>;
>> ocfs2-devel at oss.oracle.com
>> Subject: [EXT] Re: [Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in
>> ocfs2_dentry_attach_lock
>>
>> Caution: EXT Email
>>
>> On 2019/5/10 1:08 ??, Wengang Wang wrote:
>>> Hi Changwei,
>>>
>>> On 2019/5/8 18:52, Changwei Ge wrote:
>>>> Hi Wengang?
>>>>
>>>>
>>>> After checking out your further elaboration,  all my concerns are
>>>> cleared.
>>>>
>>>> I truly had a overlook on your patch then ...
>>> Thank you for your active review! Let's wait for Daniel's test result.
>> OK. You can also add my
>>
>> Reviewed-by:  Changwei Ge <gechangwei@live.cn>
>>
>> together with Daniel's positive feedback(if any) :)
>>
>>
>> Thank you for your effort into this issue. I suppose Daniel has been reporting it since a long time ago.
>>
>>
>> Thanks,
>>
>> Changwei
>>
>>
>>> thanks,
>>> wengang
>>>
>>>> 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@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
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel at oss.oracle.com
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Foss.
>> oracle.com%2Fmailman%2Flistinfo%2Focfs2-devel&amp;data=02%7C01%7Cdanie
>> l.sobe%40nxp.com%7C1b23960ebdec48a5dd7808d6d7c4bc59%7C686ea1d3bc2b4c6f
>> a92cd99c5c301635%7C0%7C0%7C636933637309761294&amp;sdata=JTi6hQGsYS%2Fy
>> rI16neiae8Q055Rg%2BLVaOLnG4BqxQyo%3D&amp;reserved=0