| Message ID | 20190529174636.22364-1-wen.gang.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] fs/ocfs2: fix race in ocfs2_dentry_attach_lock | expand |
Hi, This is a minor change from v3. It returns in place when racing is detected. Changwei and Daniel, I added your Reviewed-by and/or Tested-by with the patch, if you don't think it's good, pls let me know. thanks, wengang On 05/29/2019 10:46 AM, 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> > Reported-by: Daniel Sobe <daniel.sobe@nxp.com> > Tested-by: Daniel Sobe <daniel.sobe@nxp.com> > Reviewed-by: Changwei Ge <gechangwei@live.cn> > --- > v4: return in place on race detection. > > v3: add Reviewed-by, Reported-by and Tested-by only > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c > index 290373024d9d..e8ace3b54e9c 100644 > --- a/fs/ocfs2/dcache.c > +++ b/fs/ocfs2/dcache.c > @@ -310,6 +310,18 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry, > > out_attach: > spin_lock(&dentry_attach_lock); > + if (unlikely(dentry->d_fsdata && !alias)) { > + /* d_fsdata is set by a racing thread which is doing > + * the same thing as this thread is doing. Leave the racing > + * thread going ahead and we return here. > + */ > + spin_unlock(&dentry_attach_lock); > + iput(dl->dl_inode); > + ocfs2_lock_res_free(&dl->dl_lockres); > + kfree(dl); > + return 0; > + } > + > dentry->d_fsdata = dl; > dl->dl_count++; > spin_unlock(&dentry_attach_lock);
On Wed, May 29, 2019 at 10:46:36AM -0700, 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> > Reported-by: Daniel Sobe <daniel.sobe@nxp.com> > Tested-by: Daniel Sobe <daniel.sobe@nxp.com> > Reviewed-by: Changwei Ge <gechangwei@live.cn> > --- > v4: return in place on race detection. > > v3: add Reviewed-by, Reported-by and Tested-by only > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_html_latest_process_stable-2Dkernel-2Drules.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=k2_veS6gfouGEyAl_KYtO3stkXp1T-mThycjG-C3nVQ&s=qEdFSVeGJPPVIB9XpFnnu7fibsroE5YUT5oujKq10b8&e= for how to do this properly. </formletter>
On 19/5/30 01:46, 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> > Reported-by: Daniel Sobe <daniel.sobe@nxp.com> > Tested-by: Daniel Sobe <daniel.sobe@nxp.com> > Reviewed-by: Changwei Ge <gechangwei@live.cn> Looks fine. Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > --- > v4: return in place on race detection. > > v3: add Reviewed-by, Reported-by and Tested-by only > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c > index 290373024d9d..e8ace3b54e9c 100644 > --- a/fs/ocfs2/dcache.c > +++ b/fs/ocfs2/dcache.c > @@ -310,6 +310,18 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry, > > out_attach: > spin_lock(&dentry_attach_lock); > + if (unlikely(dentry->d_fsdata && !alias)) { > + /* d_fsdata is set by a racing thread which is doing > + * the same thing as this thread is doing. Leave the racing > + * thread going ahead and we return here. > + */ > + spin_unlock(&dentry_attach_lock); > + iput(dl->dl_inode); > + ocfs2_lock_res_free(&dl->dl_lockres); > + kfree(dl); > + return 0; > + } > + > dentry->d_fsdata = dl; > dl->dl_count++; > spin_unlock(&dentry_attach_lock); >
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c index 290373024d9d..e8ace3b54e9c 100644 --- a/fs/ocfs2/dcache.c +++ b/fs/ocfs2/dcache.c @@ -310,6 +310,18 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry, out_attach: spin_lock(&dentry_attach_lock); + if (unlikely(dentry->d_fsdata && !alias)) { + /* d_fsdata is set by a racing thread which is doing + * the same thing as this thread is doing. Leave the racing + * thread going ahead and we return here. + */ + spin_unlock(&dentry_attach_lock); + iput(dl->dl_inode); + ocfs2_lock_res_free(&dl->dl_lockres); + kfree(dl); + return 0; + } + dentry->d_fsdata = dl; dl->dl_count++; spin_unlock(&dentry_attach_lock);
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> Reported-by: Daniel Sobe <daniel.sobe@nxp.com> Tested-by: Daniel Sobe <daniel.sobe@nxp.com> Reviewed-by: Changwei Ge <gechangwei@live.cn> --- v4: return in place on race detection. v3: add Reviewed-by, Reported-by and Tested-by only 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)