[PATCH] rtnl_unlock/lock in sch_api.c

[PATCH] rtnl_unlock/lock in sch_api.c

[Catalin(ux aka Dino) BOIE]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 499 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.

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

Please apply.

Thanks!

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

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

--- linux/net/sched/sch_api.c.orig	2005-03-28 16:32:57.000000000 +0300
+++ linux/net/sched/sch_api.c	2005-03-28 16:38:17.000000000 +0300
@@ -404,18 +404,28 @@ qdisc_create(struct net_device *dev, u32
 	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 (ops == NULL && kind != NULL) {
 		char name[IFNAMSIZ];
 		if (rtattr_strlcpy(name, kind, IFNAMSIZ) < IFNAMSIZ) {
+			/* Must drop rtnl sem because request_module
+			 * can try to aquire rtnl sem (see teql for
+			 * example)
+			 */
+			rtnl_unlock();
 			request_module("sch_%s", name);
+			rtnl_lock();
 			ops = qdisc_lookup_ops(kind);
+			if (ops != NULL) {
+				module_put(ops->owner);
+				err = -EAGAIN;
+			}
 		}
 	}
 #endif
 
-	err = -EINVAL;
 	if (ops == NULL)
 		goto err_out;
 

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

[Thomas Graf]

Looks good, except...

> --- linux/net/sched/sch_api.c.orig	2005-03-28 16:32:57.000000000 +0300
> +++ linux/net/sched/sch_api.c	2005-03-28 16:38:17.000000000 +0300
> @@ -404,18 +404,28 @@ qdisc_create(struct net_device *dev, u32
>  	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 (ops == NULL && kind != NULL) {
>  		char name[IFNAMSIZ];
>  		if (rtattr_strlcpy(name, kind, IFNAMSIZ) < IFNAMSIZ) {
> +			/* Must drop rtnl sem because request_module
> +			 * can try to aquire rtnl sem (see teql for
> +			 * example)
> +			 */
> +			rtnl_unlock();
>  			request_module("sch_%s", name);
> +			rtnl_lock();
>  			ops = qdisc_lookup_ops(kind);
> +			if (ops != NULL) {
> +				module_put(ops->owner);
> +				err = -EAGAIN;

You're missing a goto err_out here, the ops == NULL won't catch it.

> +			}
>  		}
>  	}
>  #endif
>  
> -	err = -EINVAL;
>  	if (ops == NULL)
>  		goto err_out;

With the above goto inserted there is no need to move the err = -EINVAL.

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

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

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

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1750 bytes --]

===== 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) {

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

[Catalin(ux aka Dino) BOIE]

On Mon, 28 Mar 2005, Patrick McHardy wrote:

> 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

Patrick,
What to fix in Dave's patch? Seems ok to me.

Thanks!

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

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

[Patrick McHardy]

Catalin(ux aka Dino) BOIE wrote:
> What to fix in Dave's patch? Seems ok to me.

It didn't apply cleanly when I tried to apply it. Did
you try with latest -bk?

Regards
Patrick

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

[Catalin(ux aka Dino) BOIE]

On Tue, 29 Mar 2005, Patrick McHardy wrote:

> Catalin(ux aka Dino) BOIE wrote:
>> What to fix in Dave's patch? Seems ok to me.
>
> It didn't apply cleanly when I tried to apply it. Did
> you try with latest -bk?
>
> Regards
> Patrick

OK. I will fix it and I'll post again.
Thank you!

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

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

[Catalin(ux aka Dino) BOIE]

On Mon, 28 Mar 2005, Thomas Graf wrote:

> Looks good, except...
>> +				err = -EAGAIN;
>
> You're missing a goto err_out here, the ops == NULL won't catch it.
>
>> +			}
>>  		}
>>  	}
>>  #endif
>>
>> -	err = -EINVAL;
>>  	if (ops == NULL)
>>  		goto err_out;
>
> With the above goto inserted there is no need to move the err = -EINVAL.

Thanks, Thomas.

You are right.

I'm redoind the patch right now.

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