[PATCH] rtnl_unlock/lock in sch_api.c TRY2

[PATCH] rtnl_unlock/lock in sch_api.c TRY2

[Catalin(ux aka Dino) BOIE]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 638 bytes --]

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.

The patch was tested.

Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>

Please apply.

The patch is a rewrite of Dave's one with a small addition (module_put).

Thanks for help goes to Dave, Thomas and Patrick.

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1879 bytes --]

--- linux-2.6.11.6/net/sched/sch_api.c.orig	2005-03-29 01:20:18.000000000 +0300
+++ linux-2.6.11.6/net/sched/sch_api.c	2005-03-29 01:40:57.000000000 +0300
@@ -406,11 +406,24 @@ qdisc_create(struct net_device *dev, u32
 
 	ops = qdisc_lookup_ops(kind);
 #ifdef CONFIG_KMOD
-	if (ops==NULL && tca[TCA_KIND-1] != NULL) {
+	if (ops==NULL && kind != NULL) {
 		char name[IFNAMSIZ];
 		if (rtattr_strlcpy(name, kind, IFNAMSIZ) < IFNAMSIZ) {
+			/* 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.
+			 */
+			rtnl_unlock();
 			request_module("sch_%s", name);
+			rtnl_lock();
 			ops = qdisc_lookup_ops(kind);
+			if (ops != NULL) {
+				module_put(ops->owner);
+				err = -EAGAIN;
+				goto err_out;
+			}
 		}
 	}
 #endif
@@ -606,14 +619,19 @@ static int tc_get_qdisc(struct sk_buff *
 
 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;
 
@@ -707,8 +725,14 @@ create_n_graft:
 		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) {

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

[Thomas Graf]

* Catalin(ux aka Dino) BOIE <Pine.LNX.4.62.0503290206500.3229@webhosting.rdsbv.ro> 2005-03-29 02:09
> +	replay:
> +	tcm = NLMSG_DATA(n);
> +	tca = arg;
> +	clid = tcm->tcm_parent;

I know this has been in dave's patch already but why is this needed?
None of these variables changed in the meantime and the netlink message
should still be the same.

> +	q = p = NULL;

There is also no way for q being != NULL here but I don't mind
if this stays as-is, both are supposed to be temporary variables.

Despite of these two comments the patch looks ok to me.

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

[Catalin(ux aka Dino) BOIE]

On Tue, 29 Mar 2005, Thomas Graf wrote:

> * Catalin(ux aka Dino) BOIE <Pine.LNX.4.62.0503290206500.3229@webhosting.rdsbv.ro> 2005-03-29 02:09
>> +	replay:
>> +	tcm = NLMSG_DATA(n);
>> +	tca = arg;
>> +	clid = tcm->tcm_parent;
>
> I know this has been in dave's patch already but why is this needed?

I checked a little and seems that this is not needed.
But I wonder why do we need to replay?
I see no reason for replay.
I follow the path and nothing is changing under us.
What I am missing?

Thank you!
---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/