netdev
[Top] [All Lists]

Re: [PATCH] rtnl_unlock/lock in sch_api.c

To: "Catalin(ux aka Dino) BOIE" <util@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] rtnl_unlock/lock in sch_api.c
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Mon, 28 Mar 2005 16:50:32 +0200
Cc: netdev@xxxxxxxxxxx, davem@xxxxxxxxxxxxx
In-reply-to: <Pine.LNX.4.62.0503281720430.20453@xxxxxxxxxxxxxxxxxxx>
References: <Pine.LNX.4.62.0503281720430.20453@xxxxxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.6) Gecko/20050324 Debian/1.7.6-1
Catalin(ux aka Dino) BOIE wrote:
Hello!

Trying to load a custom module (same for teql but I didn't tried it)
whan the qdisc module is not loaded, makes tc hang.
This is because qdisc_create aquires rtnl_sem and then tries to load a module that tries to register_netdev (that tries to aquire the same rtnl_sem).
Applying this patch makes the problem go away.

You open a race by dropping the lock and not replaying the request
after acquiring it again. This is Dave's original patch, please
simply fix this one up so it applies again.

Regards
Patrick
===== net/sched/sch_api.c 1.41 vs edited =====
--- 1.41/net/sched/sch_api.c    2004-11-05 16:34:45 -08:00
+++ edited/net/sched/sch_api.c  2004-11-09 15:57:19 -08:00
@@ -396,17 +396,30 @@
        struct Qdisc_ops *ops;
        int size;
 
+       err = -EINVAL;
        ops = qdisc_lookup_ops(kind);
 #ifdef CONFIG_KMOD
        if (ops==NULL && tca[TCA_KIND-1] != NULL) {
                if (RTA_PAYLOAD(kind) <= IFNAMSIZ) {
+                       rtnl_unlock();
                        request_module("sch_%s", (char*)RTA_DATA(kind));
+                       rtnl_lock();
+
                        ops = qdisc_lookup_ops(kind);
+
+                       /* We dropped the RTNL semaphore in order to
+                        * perform the module load.  So, even if we
+                        * succeeded in loading the module we have to
+                        * tell the caller to replay the request.  We
+                        * indicate this using -EAGAIN.
+                        */
+                       if (ops != NULL)
+                               err = -EAGAIN;
+                       goto err_out;
                }
        }
 #endif
 
-       err = -EINVAL;
        if (ops == NULL)
                goto err_out;
        err = -EBUSY;
@@ -600,14 +613,19 @@
 
 static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
 {
-       struct tcmsg *tcm = NLMSG_DATA(n);
-       struct rtattr **tca = arg;
+       struct tcmsg *tcm;
+       struct rtattr **tca;
        struct net_device *dev;
-       u32 clid = tcm->tcm_parent;
-       struct Qdisc *q = NULL;
-       struct Qdisc *p = NULL;
+       u32 clid;
+       struct Qdisc *q, *p;
        int err;
 
+replay:
+       tcm = NLMSG_DATA(n);
+       tca = arg;
+       clid = tcm->tcm_parent;
+       q = p = NULL;
+
        if ((dev = __dev_get_by_index(tcm->tcm_ifindex)) == NULL)
                return -ENODEV;
 
@@ -701,8 +719,14 @@
                q = qdisc_create(dev, tcm->tcm_parent, tca, &err);
         else
                q = qdisc_create(dev, tcm->tcm_handle, tca, &err);
-       if (q == NULL)
+       if (q == NULL) {
+               if (err == -EAGAIN) {
+                       /* Replay the request. */
+                       dev_put(dev);
+                       goto replay;
+               }
                return err;
+       }
 
 graft:
        if (1) {

<Prev in Thread] Current Thread [Next in Thread>