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

[Patrick McHardy <kaber@xxxxxxxxx>]

Thomas Graf wrote:

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