Changwei Ge
2019-May-10 01:47 UTC
[Ocfs2-devel] [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock
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 at 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 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
Daniel Sobe
2019-May-10 07:17 UTC
[Ocfs2-devel] [EXT] Re: [PATCH v2] fs/ocfs2: fix race in ocfs2_dentry_attach_lock
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 at oss.oracle.com> On Behalf Of Changwei Ge Sent: Freitag, 10. Mai 2019 03:47 To: Wengang Wang <wen.gang.wang at 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 at 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 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_______________________________________________ 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=