Re: [PATCH] PKT_SCHED: Fix double locking in tcindex destroy path

[Thomas Graf <tgraf@xxxxxxx>]

* Patrick McHardy <41B90B81.1020102@xxxxxxxxx> 2004-12-10 03:35
> Thomas Graf wrote:
> 
> >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:

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 <tgraf@xxxxxxx>

--- linux-2.6.10-rc2-bk13.orig/net/sched/cls_tcindex.c  2004-11-30 
14:01:12.000000000 +0100
+++ linux-2.6.10-rc2-bk13/net/sched/cls_tcindex.c       2004-12-10 
13:35:03.000000000 +0100
@@ -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);
 }