Wengang Wang
2019-May-29 17:46 UTC
[Ocfs2-devel] [PATCH v4] fs/ocfs2: fix race in ocfs2_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 at oracle.com>
Reported-by: Daniel Sobe <daniel.sobe at nxp.com>
Tested-by: Daniel Sobe <daniel.sobe at nxp.com>
Reviewed-by: Changwei Ge <gechangwei at 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);
--
2.13.6
Wengang
2019-May-29 17:51 UTC
[Ocfs2-devel] [PATCH v4] fs/ocfs2: fix race in ocfs2_dentry_attach_lock
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 at oracle.com> > Reported-by: Daniel Sobe <daniel.sobe at nxp.com> > Tested-by: Daniel Sobe <daniel.sobe at nxp.com> > Reviewed-by: Changwei Ge <gechangwei at 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);
Greg KH
2019-May-29 17:52 UTC
[Ocfs2-devel] [PATCH v4] fs/ocfs2: fix race in ocfs2_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 at oracle.com> > Reported-by: Daniel Sobe <daniel.sobe at nxp.com> > Tested-by: Daniel Sobe <daniel.sobe at nxp.com> > Reviewed-by: Changwei Ge <gechangwei at 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&efor how to do this properly. </formletter>
Joseph Qi
2019-May-30 01:04 UTC
[Ocfs2-devel] [PATCH v4] fs/ocfs2: fix race in ocfs2_dentry_attach_lock
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 at oracle.com> > Reported-by: Daniel Sobe <daniel.sobe at nxp.com> > Tested-by: Daniel Sobe <daniel.sobe at nxp.com> > Reviewed-by: Changwei Ge <gechangwei at live.cn>Looks fine. Reviewed-by: Joseph Qi <joseph.qi at 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); >