Re: [PATCH] PKT_SCHED: Initialize list field in dummy qdiscs

[Patrick McHardy <kaber@trash.net>]

Thomas Graf wrote:

>* Patrick McHardy <418BB7D2.6060908@trash.net> 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.
>  
>
Yes, but there doesn't seem to be a path where this is true.

>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?
>  
>
__qdisc_destroy is called from a rcu-callback, not directly from
qdisc_destroy.

>>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?
>  
>
No, your patch looks fine.

Regards
Patrick

>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@suug.ch>
>
>--- 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);
>
>
>  
>