* Patrick McHardy <418BB7D2.6060908@xxxxxxxxx> 2004-11-05 18:26
> ops->put seems to be safe even without holding dev->queue_lock.
> The class refcnt is only changed from userspace, and always under
> the rtnl semaphore. get/put are always balanced, so pratically a
> class can never get destroyed by put.
You are right, this cannot be the problem. However, there is a
potential risk in qdisc_destroy if dev->queue_lock is not held.
I'm not sure but aren't all callers to qdisc_destroy holding
qdisc_lock_tree(dev) such as dev_shutdown a potential risk to
deadlocks because __qdisc_destroy tries to lock again?
> Either refcnt them or add add some kind of flag to qdiscs created
> by qdisc_create/qdisc_create_default and check for that flag.
> Initializing the lists doesn't fix all problems, directly using
> noop/noqueue doesn't increment the device refcnt, so is must not
> be dropped it __qdisc_destroy.
I was irritated by the TCQ_F_BUILTIN check in __qdisc_destroy. None
of the code in __qdisc_destroy should be applied to a builtin qdisc
or am I missing something?
The patch below prevents builtin qdiscs from being destroyed and
fixes a refcnt underflow whould lead to a bogus list unlinking
and dev_put.
Signed-off-by: Thomas Graf <tgraf@xxxxxxx>
--- linux-2.6.10-rc1-bk14.orig/net/sched/sch_generic.c 2004-11-05
18:44:49.000000000 +0100
+++ linux-2.6.10-rc1-bk14/net/sched/sch_generic.c 2004-11-05
18:43:52.000000000 +0100
@@ -479,15 +479,15 @@
module_put(ops->owner);
dev_put(qdisc->dev);
- if (!(qdisc->flags&TCQ_F_BUILTIN))
- kfree((char *) qdisc - qdisc->padded);
+ kfree((char *) qdisc - qdisc->padded);
}
/* Under dev->queue_lock and BH! */
void qdisc_destroy(struct Qdisc *qdisc)
{
- if (!atomic_dec_and_test(&qdisc->refcnt))
+ if (qdisc->flags & TCQ_F_BUILTIN ||
+ !atomic_dec_and_test(&qdisc->refcnt))
return;
list_del(&qdisc->list);
call_rcu(&qdisc->q_rcu, __qdisc_destroy);
|