Skip to contentSkip to conversation

PKT_SCHED: Fix double locking in tcindex destroy path

PATCH In review Thomas Graf 3 participants 4 messages Dec 10, 2004 – Dec 20, 2004

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Thomas Graf

author
#0 lore raw

PATCH 1/1 PKT_SCHED: Fix double locking in tcindex destroy path

tcindex's destroy uses its own delete functions to destroy its configuration. The delete function (correctly) takes the qdisc_tree_lock to prevent list walkings from happening while removing from the list. The qdisc_tree_lock is already held if we're comming via the destroy path and thus a double locking takes place.

Patch not needed for 2.4 since both destroy paths are unlocked but will be needed if we add them.

Signed-off-by Thomas Graf <[email protected]>
net/sched/cls_tcindex.c +11 -4
@@ -160,7 +160,8 @@
 }
 
 
-static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
+static int
+__tcindex_delete(struct tcf_proto *tp, unsigned long arg, int already_locked)
 {
 	struct tcindex_data *p = PRIV(tp);
 	struct tcindex_filter_result *r = (struct tcindex_filter_result *) arg;

@@ -182,9 +183,11 @@
 
 found:
 		f = *walk;
-		tcf_tree_lock(tp); 
+		if (!already_locked)
+			tcf_tree_lock(tp);
 		*walk = f->next;
-		tcf_tree_unlock(tp);
+		if (!already_locked)
+			tcf_tree_unlock(tp);
 	}
 	tcf_unbind_filter(tp, &r->res);
 #ifdef CONFIG_NET_CLS_POLICE

@@ -195,6 +198,10 @@
 	return 0;
 }
 
+static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
+{
+	return __tcindex_delete(tp, arg, 0);
+}
 
 /*
  * There are no parameters for tcindex_init, so we overload tcindex_change

@@ -384,7 +391,7 @@
 static int tcindex_destroy_element(struct tcf_proto *tp,
     unsigned long arg, struct tcf_walker *walker)
 {
-	return tcindex_delete(tp,arg);
+	return __tcindex_delete(tp,arg, 1);
 }
#1 lore raw
Quoted 10 lines · Thomas Graf

tcindex's destroy uses its own delete functions to destroy its configuration. The delete function (correctly) takes the qdisc_tree_lock to prevent list walkings from happening while removing from the list. The qdisc_tree_lock is already held if we're comming via the destroy path and thus a double locking takes place.

Patch not needed for 2.4 since both destroy paths are unlocked but will be needed if we add them.

Looks correct, but 2.4 does need this. qdisc_destroy in 2.4 always happens under dev->queue_lock. For example dev_shutdown from 2.4:

       write_lock(&qdisc_tree_lock);
       spin_lock_bh(&dev->queue_lock);
...
       qdisc_destroy(qdisc);

But please rename "already_locked" to "lock" to make it look less like a hack to avoid deadlock.

net/sched/cls_tcindex.c line 394

Quoted 3 lines
net/sched/cls_tcindex.c +1 -0
+	return __tcindex_delete(tp,arg, 1);

And a space is missing :)

Regards
Patrick

author
#2 lore raw

* Patrick McHardy <[email protected]> 2004-12-10 03:35

Quoted 8 lines
Quoted 6 lines · Thomas Graf

tcindex's destroy uses its own delete functions to destroy its configuration. The delete function (correctly) takes the qdisc_tree_lock to prevent list walkings from happening while removing from the list. The qdisc_tree_lock is already held if we're comming via the destroy path and thus a double locking takes place.

Quoted 6 lines
Quoted 4 lines

Patch not needed for 2.4 since both destroy paths are unlocked but will be needed if we add them.

Looks correct, but 2.4 does need this. qdisc_destroy in 2.4 always happens under dev->queue_lock. For example dev_shutdown from 2.4:

Not 100% correct since cls_api.c drops the lock before calling tcf_destroy but the patch is indeed needed and it's not a problem if dev->queue_lock is not taken since it is already unlinked as you correctly stated in your previous mail. Thanks Patrick.

Patch also applies to 2.4 with some fuzz.

Signed-off-by Thomas Graf <[email protected]>
net/sched/cls_tcindex.c +11 -4
@@ -160,7 +160,8 @@
 }
 
 
-static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
+static int
+__tcindex_delete(struct tcf_proto *tp, unsigned long arg, int lock)
 {
 	struct tcindex_data *p = PRIV(tp);
 	struct tcindex_filter_result *r = (struct tcindex_filter_result *) arg;

@@ -182,9 +183,11 @@
 
 found:
 		f = *walk;
-		tcf_tree_lock(tp); 
+		if (lock)
+			tcf_tree_lock(tp);
 		*walk = f->next;
-		tcf_tree_unlock(tp);
+		if (lock)
+			tcf_tree_unlock(tp);
 	}
 	tcf_unbind_filter(tp, &r->res);
 #ifdef CONFIG_NET_CLS_POLICE

@@ -195,6 +198,10 @@
 	return 0;
 }
 
+static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
+{
+	return __tcindex_delete(tp, arg, 1);
+}
 
 /*
  * There are no parameters for tcindex_init, so we overload tcindex_change

@@ -384,7 +391,7 @@
 static int tcindex_destroy_element(struct tcf_proto *tp,
     unsigned long arg, struct tcf_walker *walker)
 {
-	return tcindex_delete(tp,arg);
+	return __tcindex_delete(tp, arg, 0);
 }
maintainer
#3 lore raw
Quoted 16 lines · On Fri, 10 Dec 2004 13:44:45 +0100 Thomas Graf <[email protected]> wrote:

* Patrick McHardy <[email protected]> 2004-12-10 03:35

Quoted 8 lines
Quoted 6 lines · Thomas Graf

tcindex's destroy uses its own delete functions to destroy its configuration. The delete function (correctly) takes the qdisc_tree_lock to prevent list walkings from happening while removing from the list. The qdisc_tree_lock is already held if we're comming via the destroy path and thus a double locking takes place.

Quoted 6 lines
Quoted 4 lines

Patch not needed for 2.4 since both destroy paths are unlocked but will be needed if we add them.

Looks correct, but 2.4 does need this. qdisc_destroy in 2.4 always happens under dev->queue_lock. For example dev_shutdown from 2.4:

Quoted 8 lines

Not 100% correct since cls_api.c drops the lock before calling tcf_destroy but the patch is indeed needed and it's not a problem if dev->queue_lock is not taken since it is already unlinked as you correctly stated in your previous mail. Thanks Patrick.

Patch also applies to 2.4 with some fuzz.

Signed-off-by Thomas Graf <[email protected]>

I think the conditional locking is quite ugly, but I can't suggest something better at this time.

Patch applied to both 2.4.x and 2.6.x, thanks Patrick and Thomas.