Skip to contentSkip to conversation

ocfs2: Fix locking for res->tracking and dlm->tracking_list

PATCH New Ashish Samant 4 participants 11 messages Jun 21, 2018 – Jun 25, 2018 OCFS2

lore
Share & tools
lore

Ashish Samant

author
#0 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c | 2 ++
 fs/ocfs2/dlm/dlmmaster.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +2 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			mlog(0, "%s: more migration to do\n", dlm->name);
 		}
 
+		spin_lock(&dlm->track_lock);
 		/* This list should be empty. If not, print remaining lockres */
 		if (!list_empty(&dlm->tracking_list)) {
 			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			list_for_each_entry(res, &dlm->tracking_list, tracking)
 				dlm_print_one_lock_resource(res);
 		}
+		spin_unlock(&dlm->track_lock);
 
 		dlm_mark_domain_leaving(dlm);
 		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +2 -2
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 
 	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
 	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
 	memset(res->lvb, 0, DLM_LVB_LEN);
 	memset(res->refmap, 0, sizeof(res->refmap));
Signature · 1 line
1.9.1
#1 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Hi Ashish,

I think we should get 'res->spinlock' before getting 'dlm->track_lock' such as in __dlm_do_purge_lockres(). But your patch reverse the locking sequence as we will get 'res->spinlock' in dlm_print_one_lock_resource() which may cause 'ABBA' deadlock.

thanks,
Jun

Quoted 50 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c | 2 ++
 fs/ocfs2/dlm/dlmmaster.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +2 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			mlog(0, "%s: more migration to do\n", dlm->name);
 		}
 
+		spin_lock(&dlm->track_lock);
 		/* This list should be empty. If not, print remaining lockres */
 		if (!list_empty(&dlm->tracking_list)) {
 			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			list_for_each_entry(res, &dlm->tracking_list, tracking)
 				dlm_print_one_lock_resource(res);
 		}
+		spin_unlock(&dlm->track_lock);
 
 		dlm_mark_domain_leaving(dlm);
 		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +2 -2
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 
 	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
 	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
 	memset(res->lvb, 0, DLM_LVB_LEN);
 	memset(res->refmap, 0, sizeof(res->refmap));
#2 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 6 lines · piaojun

Hi Ashish,

I think we should get 'res->spinlock' before getting 'dlm->track_lock' such as in __dlm_do_purge_lockres(). But your patch reverse the locking sequence as we will get 'res->spinlock' in dlm_print_one_lock_resource() which may cause 'ABBA' deadlock.

Agree. This patch could introduce dead lock issue.

Quoted 25 lines

thanks,
Jun

Quoted 21 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
  fs/ocfs2/dlm/dlmdomain.c | 2 ++
  fs/ocfs2/dlm/dlmmaster.c | 4 ++--
  2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			mlog(0, "%s: more migration to do\n", dlm->name);
  		}
Quoted 9 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 734

Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +2 -0
+		spin_lock(&dlm->track_lock);
  		/* This list should be empty. If not, print remaining lockres */
  		if (!list_empty(&dlm->tracking_list)) {
  			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			list_for_each_entry(res, &dlm->tracking_list, tracking)
  				dlm_print_one_lock_resource(res);
  		}
+		spin_unlock(&dlm->track_lock);

As Jun's comment, above fix is not proper. But the issue Ashish points to truly exists.

Quoted 9 lines
Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
  		dlm_mark_domain_leaving(dlm);
  		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
Quoted 7 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 589

Quoted 7 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
  	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
  	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);

Yet, this fix makes sense.

Thanks,
Changwei

Quoted 8 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm,

Quoted 4 lines
fs/ocfs2/dlm/dlmmaster.c +0 -0
  	memset(res->lvb, 0, DLM_LVB_LEN);
  	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#3 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

In reply to Ashish Samant
Quoted 26 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c | 2 ++
 fs/ocfs2/dlm/dlmmaster.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +1 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			mlog(0, "%s: more migration to do\n", dlm->name);
 		}
 
+		spin_lock(&dlm->track_lock);
 		/* This list should be empty. If not, print remaining lockres */
 		if (!list_empty(&dlm->tracking_list)) {
 			mlog(ML_ERROR, "Following lockres' are still on the "

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 			list_for_each_entry(res, &dlm->tracking_list, tracking)
 				dlm_print_one_lock_resource(res);
 		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Thanks,
Joseph

Quoted 19 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
 		dlm_mark_domain_leaving(dlm);
 		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +2 -2
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 
 	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
 	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
 	memset(res->lvb, 0, DLM_LVB_LEN);
 	memset(res->refmap, 0, sizeof(res->refmap));
#4 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 23 lines · Joseph Qi
Quoted 21 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
  fs/ocfs2/dlm/dlmdomain.c | 2 ++
  fs/ocfs2/dlm/dlmmaster.c | 4 ++--
  2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			mlog(0, "%s: more migration to do\n", dlm->name);
  		}
Quoted 13 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +2 -0
+		spin_lock(&dlm->track_lock);
  		/* This list should be empty. If not, print remaining lockres */
  		if (!list_empty(&dlm->tracking_list)) {
  			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			list_for_each_entry(res, &dlm->tracking_list, tracking)
  				dlm_print_one_lock_resource(res);
  		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

That won't be easy since I think the copying should also should lock resource lock.
Perhaps, we can remove lock resource from dlm->track_list only when the lock resource is released.
It brings another benefit that we can easily find which lock resource is leaked.

Thanks,
Changwei

Quoted 27 lines

Thanks,
Joseph

Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
  		dlm_mark_domain_leaving(dlm);
  		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 592

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
  	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
  	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
  	memset(res->lvb, 0, DLM_LVB_LEN);
  	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#5 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 39 lines · Changwei Ge
Quoted 23 lines · Joseph Qi
Quoted 21 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
  fs/ocfs2/dlm/dlmdomain.c | 2 ++
  fs/ocfs2/dlm/dlmmaster.c | 4 ++--
  2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			mlog(0, "%s: more migration to do\n", dlm->name);
  		}
Quoted 13 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +2 -0
+		spin_lock(&dlm->track_lock);
  		/* This list should be empty. If not, print remaining lockres */
  		if (!list_empty(&dlm->tracking_list)) {
  			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
  			list_for_each_entry(res, &dlm->tracking_list, tracking)
  				dlm_print_one_lock_resource(res);
  		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Quoted 2 lines

That won't be easy since I think the copying should also should lock resource lock.

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

Thanks,
Joseph

Quoted 36 lines

Perhaps, we can remove lock resource from dlm->track_list only when the lock resource is released.
It brings another benefit that we can easily find which lock resource is leaked.

Thanks,
Changwei

Quoted 27 lines

Thanks,
Joseph

Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
  		dlm_mark_domain_leaving(dlm);
  		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 592

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
  	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
  	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
  	memset(res->lvb, 0, DLM_LVB_LEN);
  	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#6 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 39 lines · Joseph Qi
Quoted 37 lines · Changwei Ge
Quoted 35 lines · Joseph Qi
Quoted 26 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
   fs/ocfs2/dlm/dlmdomain.c | 2 ++
   fs/ocfs2/dlm/dlmmaster.c | 4 ++--
   2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +1 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
   			mlog(0, "%s: more migration to do\n", dlm->name);
   		}
 
+		spin_lock(&dlm->track_lock);
   		/* This list should be empty. If not, print remaining lockres */
   		if (!list_empty(&dlm->tracking_list)) {
   			mlog(ML_ERROR, "Following lockres' are still on the "

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
   			list_for_each_entry(res, &dlm->tracking_list, tracking)
   				dlm_print_one_lock_resource(res);
   		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Quoted 6 lines
Quoted 2 lines

That won't be easy since I think the copying should also should lock resource lock.

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

I thought you' want to copy lock resources as well.
Um, is it possible that the copied track list points to some stale lock resources which are released after the copy.

Thanks,
Changwei

Quoted 38 lines

Thanks,
Joseph

Quoted 34 lines

Perhaps, we can remove lock resource from dlm->track_list only when the lock resource is released.
It brings another benefit that we can easily find which lock resource is leaked.

Thanks,
Changwei

Quoted 26 lines

Thanks,
Joseph

Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
   		dlm_mark_domain_leaving(dlm);
   		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 592

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
   	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
   	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
   	memset(res->lvb, 0, DLM_LVB_LEN);
   	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#7 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 44 lines · Changwei Ge
Quoted 41 lines · Joseph Qi
Quoted 39 lines · Changwei Ge
Quoted 22 lines · Joseph Qi
Quoted 21 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
   fs/ocfs2/dlm/dlmdomain.c | 2 ++
   fs/ocfs2/dlm/dlmmaster.c | 4 ++--
   2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
   			mlog(0, "%s: more migration to do\n", dlm->name);
   		}
Quoted 13 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +2 -0
+		spin_lock(&dlm->track_lock);
   		/* This list should be empty. If not, print remaining lockres */
   		if (!list_empty(&dlm->tracking_list)) {
   			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
   			list_for_each_entry(res, &dlm->tracking_list, tracking)
   				dlm_print_one_lock_resource(res);
   		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

That won't be easy since I think the copying should also should lock resource lock.

Quoted 8 lines
Quoted 4 lines

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

I thought you' want to copy lock resources as well.
Um, is it possible that the copied track list points to some stale lock resources which are released after the copy.

Oh, yes, you are right. This risk still exists.

Thanks,
Joseph

Quoted 36 lines
Quoted 35 lines
Quoted 35 lines

Perhaps, we can remove lock resource from dlm->track_list only when the lock resource is released.
It brings another benefit that we can easily find which lock resource is leaked.

Thanks,
Changwei

Quoted 26 lines

Thanks,
Joseph

Quoted 9 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
   		dlm_mark_domain_leaving(dlm);
   		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 592

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
   	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
   	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);
 
   	memset(res->lvb, 0, DLM_LVB_LEN);
   	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#8 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

In reply to Changwei Ge
Quoted 26 lines · Changwei Ge
Quoted 24 lines · Joseph Qi
Quoted 23 lines · Changwei Ge
Quoted 22 lines · Joseph Qi
Quoted 21 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify res->tracking and dlm->tracking_list without holding dlm->track_lock. This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
    fs/ocfs2/dlm/dlmdomain.c | 2 ++
    fs/ocfs2/dlm/dlmmaster.c | 4 ++--
    2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
    			mlog(0, "%s: more migration to do\n", dlm->name);
    		}
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
+		spin_lock(&dlm->track_lock);
    		/* This list should be empty. If not, print remaining lockres */
    		if (!list_empty(&dlm->tracking_list)) {
    			mlog(ML_ERROR, "Following lockres' are still on the "

@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
    			list_for_each_entry(res, &dlm->tracking_list, tracking)
    				dlm_print_one_lock_resource(res);
    		}
+		spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Right, for some reason, I was thinking the call is to __dlm_print_lock_resource() and not dlm_print_one_lock_resource(). So this could deadlock.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

That won't be easy since I think the copying should also should lock resource lock.

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

I thought you' want to copy lock resources as well.
Um, is it possible that the copied track list points to some stale lock resources which are released after the copy.

Yes dropping the track_lock can still cause the same problem. However, I am wondering , since this is during dlm unregister domain/ cluster disconnect after the dlm_thread has run, under what conditions would a concurrent access to the tracking_list occur at this point?

Thanks,
Ashish

Quoted 26 lines

Thanks,
Changwei

Quoted 22 lines

Thanks,
Joseph

Quoted 19 lines

Perhaps, we can remove lock resource from dlm->track_list only when the lock resource is released.
It brings another benefit that we can easily find which lock resource is leaked.

Thanks,
Changwei

Quoted 11 lines

Thanks,
Joseph

Quoted 8 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
    		dlm_mark_domain_leaving(dlm);
    		dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 589

Quoted 7 lines
fs/ocfs2/dlm/dlmmaster.c +2 -2
    	res->last_used = 0;
 
-	spin_lock(&dlm->spinlock);
+	spin_lock(&dlm->track_lock);
    	list_add_tail(&res->tracking, &dlm->tracking_list);
-	spin_unlock(&dlm->spinlock);
+	spin_unlock(&dlm->track_lock);

Maybe we only need this to fix the issue.

Thanks,
Ashish

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm,

Quoted 4 lines
fs/ocfs2/dlm/dlmmaster.c +0 -0
    	memset(res->lvb, 0, DLM_LVB_LEN);
    	memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#9 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 56 lines · Ashish Samant
Quoted 34 lines · Changwei Ge
Quoted 32 lines · Joseph Qi
Quoted 31 lines · Changwei Ge
Quoted 30 lines · Joseph Qi
Quoted 29 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify
res->tracking and dlm->tracking_list without holding dlm->track_lock.
This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>

---
??? fs/ocfs2/dlm/dlmdomain.c | 2 ++
??? fs/ocfs2/dlm/dlmmaster.c | 4 ++--
??? 2 files changed, 4 insertions(+), 2 deletions(-)

fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
 ??????????????? mlog(0, "%s: more migration to do\n", dlm->name);
 ??????????? }
 ??? +??????? spin_lock(&dlm->track_lock);

??????????? /* This list should be empty. If not, print remaining lockres */
??????????? if (!list_empty(&dlm->tracking_list)) { ??????????????? mlog(ML_ERROR, "Following lockres' are still on the "

fs/ocfs2/dlm/dlmdomain.c +0 -0
@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
 ??????????????? list_for_each_entry(res, &dlm->tracking_list, 
 tracking)
 ??????????????????? dlm_print_one_lock_resource(res);
 ??????????? }
+??????? spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Right, for some reason, I was thinking the call is to __dlm_print_lock_resource() and not dlm_print_one_lock_resource(). So this could deadlock.

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

That won't be easy since I think the copying should also should lock resource lock.

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

Quoted 7 lines
Quoted 3 lines

I thought you' want to copy lock resources as well.
Um, is it possible that the copied track list points to some stale lock resources which are released after the copy.

Yes dropping the track_lock can still cause the same problem. However, I am wondering , since this is during dlm unregister domain/ cluster disconnect after the dlm_thread has run, under what conditions would a concurrent access to the tracking_list occur at this point?

I think your assumption stands, we don't have to worry much about concurrent access to the ::tracking_list. DLM should make sure that after migrating all lock resources, no more lock resources should be born.

Quoted 24 lines

Thanks,
Ashish

Quoted 20 lines

Thanks,
Changwei

Quoted 16 lines

Thanks,
Joseph

Quoted 13 lines

Perhaps, we can remove lock resource from dlm->track_list only when the
lock resource is released.
It brings another benefit that we can easily find which lock resource is
leaked.

Thanks,
Changwei

Quoted 3 lines

Thanks,
Joseph

Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
 dlm_mark_domain_leaving(dlm);
 ??????????? dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt · line 589

Quoted 8 lines
fs/ocfs2/dlm/dlmmaster.c +2 -1
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt 
 *dlm,
 ??? ??????? res->last_used = 0;
 ??? -??? spin_lock(&dlm->spinlock);
+??? spin_lock(&dlm->track_lock);
 ??????? list_add_tail(&res->tracking, &dlm->tracking_list);
-??? spin_unlock(&dlm->spinlock);
+??? spin_unlock(&dlm->track_lock);

Maybe we only need this to fix the issue.

Agree. Could you resend your patch?

Thanks,
Changwei

Quoted 13 lines

Thanks,
Ashish

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt · line 592

Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0 -0
 ??? ??????? memset(res->lvb, 0, DLM_LVB_LEN);
 ??????? memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#10 lore raw

PATCH 1/1 ocfs2: Fix locking for res->tracking and dlm->tracking_list

Quoted 103 lines · Changwei Ge
Quoted 53 lines · Ashish Samant
Quoted 33 lines · Changwei Ge
Quoted 32 lines · Joseph Qi
Quoted 31 lines · Changwei Ge
Quoted 30 lines · Joseph Qi
Quoted 29 lines · Ashish Samant

In dlm_init_lockres() and dlm_unregister_domain() we access and modify
res->tracking and dlm->tracking_list without holding dlm->track_lock.
This can cause list corruptions and can end up in kernel panic.

Fix this by locking res->tracking and dlm->tracking_list with dlm->track_lock at all places.

Signed-off-by Ashish Samant <[email protected]>
---
     fs/ocfs2/dlm/dlmdomain.c | 2 ++
     fs/ocfs2/dlm/dlmmaster.c | 4 ++--
     2 files changed, 4 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +0 -0
index 2acd58b..cfb1edd 100644
@@ -723,6 +723,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
                 mlog(0, "%s: more migration to do\n", dlm->name);
             }
     +        spin_lock(&dlm->track_lock);
             /* This list should be empty. If not, print remaining

lockres */
if (!list_empty(&dlm->tracking_list)) {
mlog(ML_ERROR, "Following lockres' are still on
the "

fs/ocfs2/dlm/dlmdomain.c +0 -0
@@ -730,6 +731,7 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 735

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c +1 -0
                 list_for_each_entry(res, &dlm->tracking_list,
 tracking)
                     dlm_print_one_lock_resource(res);
             }
+        spin_unlock(&dlm->track_lock);

The locking order should be res->spinlock > dlm->track_lock. Since here just want to print error message for issue tracking, I'm wandering if we can copy tracking list to local first.

Right, for some reason, I was thinking the call is to __dlm_print_lock_resource() and not dlm_print_one_lock_resource(). So this could deadlock.

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

That won't be easy since I think the copying should also should lock resource lock.

Copy tracking list only need taking track_lock.
Then access local tracking list we don't have to take it any more and then we can call dlm_print_one_lock_resource() which will take res->spinlock.

Quoted 7 lines
Quoted 3 lines

I thought you' want to copy lock resources as well.
Um, is it possible that the copied track list points to some stale lock resources which are released after the copy.

Yes dropping the track_lock can still cause the same problem. However, I am wondering , since this is during dlm unregister domain/ cluster disconnect after the dlm_thread has run, under what conditions would a concurrent access to the tracking_list occur at this point?

I think your assumption stands, we don't have to worry much about concurrent access to the ::tracking_list. DLM should make sure that after migrating all lock resources, no more lock resources should be born.

Quoted 22 lines

Thanks,
Ashish

Quoted 19 lines

Thanks,
Changwei

Quoted 16 lines

Thanks,
Joseph

Quoted 13 lines

Perhaps, we can remove lock resource from dlm->track_list only when the
lock resource is released.
It brings another benefit that we can easily find which lock resource is
leaked.

Thanks,
Changwei

Quoted 3 lines

Thanks,
Joseph

Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/ocfs2/dlm/dlmdomain.c +0 -0
 dlm_mark_domain_leaving(dlm);
             dlm_leave_domain(dlm);
fs/ocfs2/dlm/dlmmaster.c +0 -0
index aaca094..826f056 100644
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt · line 589

Quoted 8 lines
fs/ocfs2/dlm/dlmmaster.c +2 -1
@@ -584,9 +584,9 @@ static void dlm_init_lockres(struct dlm_ctxt
 *dlm,
             res->last_used = 0;
     -    spin_lock(&dlm->spinlock);
+    spin_lock(&dlm->track_lock);
         list_add_tail(&res->tracking, &dlm->tracking_list);
-    spin_unlock(&dlm->spinlock);
+    spin_unlock(&dlm->track_lock);

Maybe we only need this to fix the issue.

Quoted 1 line

Agree. Could you resend your patch?

Sent V2.

Thanks,
Ashish

Quoted 15 lines

Thanks,
Changwei

Quoted 11 lines

Thanks,
Ashish

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt · line 592

Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0 -0
             memset(res->lvb, 0, DLM_LVB_LEN);
         memset(res->refmap, 0, sizeof(res->refmap));

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel