request_module while holding rtnl semaphore

There are several instances of request_module beeing called while holding the rtnl semaphore in net/sched. A pratical problem with this is the teql scheduler which deadlocks when calling register_netdev from its init function. A more far-fetched problem would be some crazy person with their modules in a nfs-mounted directory on a server reachable over a dial-on-demand link. I couldn't come up with a solution except for refusing to autoload teql, maybe someone else has an idea.

1) Abuse of the rtnl. It's being used for too many things. It's basically the networking system's BKL. If the locking were more granular then this shouldn't occur.

2) Hooking random net/sched requests into rtnetlink. By being an rtnetlink user you pay the price of taking the rtnl. Most of the net/sched stuff has nothing to do with rtnetlink.

Quoted 5 lines · On Sat, 06 Nov 2004 10:35:45 +1100 Herbert Xu <[email protected]>…

2) Hooking random net/sched requests into rtnetlink. By being an rtnetlink user you pay the price of taking the rtnl. Most of the net/sched stuff has nothing to do with rtnetlink.

I believe this is a false statement. Every single networking config change depends upon device existence and state in some way. By virtue of that, they really depend upon the RTNL being held during the duration of their execution.

Only the actual implementations know when and where such a dependency on device state et al. does not exist, and therefore where RTNL holding is not necessary.

Therefore I suggest we just implement the fix for this inside of the packet scheduler layer itself. Simply by dropping the RTNL semaphore during the module request, and then regrabbing the RTNL semaphore and replaying the request from the beginning.

The net/sched/sch_api.c version of the fix would look like the following. The act_api.c case would require a bit more surgery, but with the right restructuring it can be done too.

===== net/sched/sch_api.c 1.41 vs edited =====

net/sched/sch_api.c +31 -7

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

Quoted 3 lines · On Sat, 06 Nov 2004 10:35:45 +1100 Herbert Xu <[email protected]>…

2) Hooking random net/sched requests into rtnetlink. By being an rtnetlink user you pay the price of taking the rtnl. Most of the net/sched stuff has nothing to do with rtnetlink.

I believe this is a false statement. Every single networking config change depends upon device existence and state in some way. By virtue of that, they really depend upon the RTNL being held during the duration of their execution.

Only the actual implementations know when and where such a dependency on device state et al. does not exist, and therefore where RTNL holding is not necessary.

Therefore I suggest we just implement the fix for this inside of the packet scheduler layer itself. Simply by dropping the RTNL semaphore during the module request, and then regrabbing the RTNL semaphore and replaying the request from the beginning.

Sounds reasonable for the sch_api.c and cls_api.c case but will get very nasty for act_api as you noted and any further sub layers that might be added in the future. (The classifier I'm working on is such a case.)

Quoted 5 lines · Thomas Graf

Sounds reasonable for the sch_api.c and cls_api.c case but will get very nasty for act_api as you noted and any further sub layers that might be added in the future. (The classifier I'm working on is such a case.)

Can you please show an example of a case where this is needed ? Just to help me understand.

* Patrick McHardy <[email protected]> 2004-11-10 02:10

Quoted 4 lines

Can you please show an example of a case where this is needed ? Just to help me understand.

The action code might load modules in the middle of a classifier configuration and it will be very hard to reverse those changes. Right now we could move it to the top of all configurations and it would probably be possible to get back where we fetch the device but it will get impossible once a classifier requires module loading which is not unlikely.

Well, it's not impossible but it would mean to have the action code parse the TLV and just try to load the module, then report to the classifier so it can try to load its own modules and then do the actual action and classifier configuration. I don't even want to think of how nasty it gets once a action module requests modules itself ;->

Quoted 8 lines · Thomas Graf

The action code might load modules in the middle of a classifier configuration and it will be very hard to reverse those changes. Right now we could move it to the top of all configurations and it would probably be possible to get back where we fetch the device but it will get impossible once a classifier requires module loading which is not unlikely.

Assuming all error-paths do proper cleanup, returning -EAGAIN should always result in the same configuration state as before.

that once this is true it would work perfectly fine, however I think it would be inefficient to parse the whole TLV tree multiple times.

* Patrick McHardy <[email protected]> 2004-11-10 02:29

Quoted 4 lines

Assuming all error-paths do proper cleanup, returning -EAGAIN should always result in the same configuration state as before.

I agree but this assumption is wrong, at least for u32.

I agree
that once this is true it would work perfectly fine, however I think it would be inefficient to parse the whole TLV tree multiple times.

Quoted 3 lines

that once this is true it would work perfectly fine, however I think it would be inefficient to parse the whole TLV tree multiple times.

Well it's only going to happen once for each module so that's no big deal.

It will only happen once for every module, so I don't think it's a big deal.

Agreed. I'm used to huge filter configurations up to 100 MiB per netlink message and therefore I do care whether such as message is

Quoted 3 lines

Agreed. I'm used to huge filter configurations up to 100 MiB per netlink message and therefore I do care whether such as message is

100MiB!? How did you manage to stuff that into one linear skb?

Quoted 4 lines

Quoted 2 lines

Assuming all error-paths do proper cleanup, returning -EAGAIN should always result in the same configuration state as before.

I agree but this assumption is wrong, at least for u32.

It will be true soon :) Anything else is a bug, and a nice side-effect of this change is that all those dusty error-paths actually get used.

I started working on a patchset to clean up all the error paths, allow changing all pararmeters for those not supporting it yet and to add the action bits for tcindex, route, and rsvp. Finishing it and writing all the test cases to do proper testing will take some time but I hope to have it ready for early 2.6.11 inclusion.

Just to let you know so we don't do redundant work.

The handling of a failure in tcf_bind_filter is inconsistent.

u32: ignore
fw: ignore
route: ignore
rsvp: ignore
tcindex: error

It might be a good idea to make this consistent. So in order to validate the classid before making any changes we could simply lock it via get (see patch below), return an error if it fails and put it back in case of an error further in the path or after binding the filter.

Bindings not only locks the class from removal while a filter is pointing to it. It speeds up classyfing by saving a lookup for every tc_classify call. It's not really a problem if the class is not locked, the qdisc will look it up and falls back to a default class if it doesn't exists so it's rather a cosmetic/policy thing.

Quoted 13 lines · Thomas Graf

The handling of a failure in tcf_bind_filter is inconsistent.

u32: ignore
fw: ignore
route: ignore
rsvp: ignore
tcindex: error

It might be a good idea to make this consistent. So in order to validate the classid before making any changes we could simply lock it via get (see patch below), return an error if it fails and put it back in case of an error further in the path or after binding the filter.

Quoted 6 lines

Bindings not only locks the class from removal while a filter is pointing to it. It speeds up classyfing by saving a lookup for every tc_classify call. It's not really a problem if the class is not locked, the qdisc will look it up and falls back to a default class if it doesn't exists so it's rather a cosmetic/policy thing.

You should just fix tcindex not to care about errors in tcf_bind_filter. bind_tcf already locks the class. Some qdiscs (like prio) map bind_filter to get, but others (HTB, HFSC, CBQ) use a seperate counter because it is legal to end up with a refcnt > 0 after delete. When a class with filters pointing to it is tried to destroy they return -EBUSY, which can't be done by looking at the refcnt.

* Patrick McHardy <[email protected]> 2004-12-13 19:11

Quoted 8 lines

You should just fix tcindex not to care about errors in tcf_bind_filter. bind_tcf already locks the class. Some qdiscs (like prio) map bind_filter to get, but others (HTB, HFSC, CBQ) use a seperate counter because it is legal to end up with a refcnt > 0 after delete. When a class with filters pointing to it is tried to destroy they return -EBUSY, which can't be done by looking at the refcnt.

Little misunderstanding here. I'm not aiming at replacing tcf_bind_filter with get. My question is rather whether to regard tcf_bind_filter not setting tcf_result->class as an error or ignore it.

I'm all for ignoring it in tcindex, it requires some changes because it checks tcf_result.class field to see if hash bucket is non-empty if perfect hash is used but is not a problem at all.

The tcf_class_get/put would be required to ensure proper locking during validation of parameters if validating the classid being last before changing things doesn't make sense due to the need to undo expensive operations required before binding.

I will fix tcindex, since you also agree on simply ignoring it and regard the binding as an ptional locking and performance increase possibility given to userspace.

* Patrick McHardy <[email protected]> 2004-12-13 19:11

Quoted 21 lines

Quoted 13 lines · Thomas Graf

The handling of a failure in tcf_bind_filter is inconsistent.

u32: ignore
fw: ignore
route: ignore
rsvp: ignore
tcindex: error

It might be a good idea to make this consistent. So in order to validate the classid before making any changes we could simply lock it via get (see patch below), return an error if it fails and put it back in case of an error further in the path or after binding the filter.

Quoted 6 lines

Bindings not only locks the class from removal while a filter is pointing to it. It speeds up classyfing by saving a lookup for every tc_classify call. It's not really a problem if the class is not locked, the qdisc will look it up and falls back to a default class if it doesn't exists so it's rather a cosmetic/policy thing.

Quoted 6 lines

You should just fix tcindex not to care about errors in tcf_bind_filter. bind_tcf already locks the class. Some qdiscs (like prio) map bind_filter to get, but others (HTB, HFSC, CBQ) use a seperate counter because it is legal to end up with a refcnt > 0 after delete. When a class with filters pointing to it is tried to destroy they return -EBUSY, which can't be done by looking at the refcnt.

Little misunderstanding here. I'm not aiming at replacing tcf_bind_filter with get. My question is rather whether to regard tcf_bind_filter not setting tcf_result->class as an error or ignore it.

I'm all for ignoring it in tcindex, it requires some changes because it checks tcf_result.class field to see if hash bucket is non-empty if perfect hash is used but is not a problem at all.

The tcf_class_get/put would be required to ensure proper locking during validation of parameters if validating the classid being last before changing things doesn't make sense due to the need to undo expensive operations required before binding.

I will fix tcindex, since you also agree on simply ignoring it and regard the binding as an ptional locking and performance increase possibility given to userspace.

When/why would binding fail? tcindex is an exception.
I dont see binding as having any contribution to the error path. Additional locking is not advisable. The binding could happen while traffic is running.

I totally disagree. The use of rtnl for all networking configuration changes is a virtue of our current setup.

Only the actual implementations know when and where such a dependency on device state et al. does not exist, and therefore where RTNL holding is not necessary.

Therefore I suggest we just implement the fix for this inside of the packet scheduler layer itself. Simply by dropping the RTNL semaphore during the module request, and then regrabbing the RTNL semaphore and replaying the request from the beginning.

Therefore I suggest we just implement the fix for this inside of the packet scheduler layer itself. Simply by dropping the RTNL semaphore during the module request, and then regrabbing the RTNL semaphore and replaying the request from the beginning.

The net/sched/sch_api.c version of the fix would look like the following. The act_api.c case would require a bit more surgery, but with the right restructuring it can be done too.

===== net/sched/sch_api.c 1.41 vs edited =====

net/sched/sch_api.c +31 -7

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

Quoted 10 lines · David S. Miller

Therefore I suggest we just implement the fix for this inside of the packet scheduler layer itself. Simply by dropping the RTNL semaphore during the module request, and then regrabbing the RTNL semaphore and replaying the request from the beginning.

The net/sched/sch_api.c version of the fix would look like the following. The act_api.c case would require a bit more surgery, but with the right restructuring it can be done too.

This patch got lost somehow. The act_api.c changes are actually even more complicated because besides the action init path, changes can also be made from classifiers in a deep call-chain. I hope Thomas's recent changes make it easier to fix this, but I think this patch should go in now anyway.

* Patrick McHardy <[email protected]> 2005-01-11 04:04

Quoted 6 lines

This patch got lost somehow. The act_api.c changes are actually even more complicated because besides the action init path, changes can also be made from classifiers in a deep call-chain. I hope Thomas's recent changes make it easier to fix this, but I think this patch should go in now anyway.

The action initialization is now done first and no classifier data is changed except for tp->root modifications which must not be undone so you can safely return EBUSY. rsvp might be an exception, I haven't looked too closely into it yet.

tcindex returns EBUSY when a filter is changed which does not fit into the hashtable, so this must be changed.

Quoted 3 lines · Thomas Graf

tcindex returns EBUSY when a filter is changed which does not fit into the hashtable, so this must be changed.

It's -EAGAIN, so no problem, nothing returns this currently. I'm going to send a patch later.

* Patrick McHardy <[email protected]> 2005-01-11 22:05

Quoted 6 lines

Quoted 3 lines · Thomas Graf

tcindex returns EBUSY when a filter is changed which does not fit into the hashtable, so this must be changed.

It's -EAGAIN, so no problem, nothing returns this currently. I'm going to send a patch later.

Of course, then there is no problem. Will you fix the request_module in cls_api.c as well?

Quoted 3 lines

Of course, then there is no problem. Will you fix the request_module in cls_api.c as well?

Just saw your patch, never mind.