Re: [PATCH net] net/sched: Abort __tc_modify_qdisc if parent class does not exist

[Cong Wang <xiyou.wangcong@gmail.com>]

On Fri, Jul 04, 2025 at 01:34:22PM -0300, Victor Nogueira wrote:
> Lion's patch [1] revealed an ancient bug in the qdisc API.
> Whenever a user creates/modifies a qdisc specifying as a parent another
> qdisc, the qdisc API will, during grafting, detect that the user is
> not trying to attach to a class and reject. However grafting is
> performed after qdisc_create (and thus the qdiscs' init callback) is
> executed. In qdiscs that eventually call qdisc_tree_reduce_backlog
> during init or change (such as fq, hhf, choke, etc), an issue
> arises. For example, executing the following commands:
> 
> sudo tc qdisc add dev lo root handle a: htb default 2
> sudo tc qdisc add dev lo parent a: handle beef fq
> 
> Qdiscs such as fq, hhf, choke, etc unconditionally invoke
> qdisc_tree_reduce_backlog() in their control path init() or change() which
> then causes a failure to find the child class; however, that does not stop
> the unconditional invocation of the assumed child qdisc's qlen_notify with
> a null class. All these qdiscs make the assumption that class is non-null.
> 
> The solution is ensure that qdisc_leaf() which looks up the parent
> class, and is invoked prior to qdisc_create(), should return failure on
> not finding the class.
> In this patch, we leverage qdisc_leaf to return ERR_PTRs whenever the
> parentid doesn't correspond to a class, so that we can detect it
> earlier on and abort before qdisc_create is called.
> 

Thanks for your quick and excellent catch!

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Just one additional question, do we still need to safe-guard
qdisc_tree_reduce_backlog()? Below is actually what pops on my mind
immediately after reading your above description (before reading your
code):


diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c5e3673aadbe..5a32af623aa4 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -817,6 +817,8 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
                cops = sch->ops->cl_ops;
                if (notify && cops->qlen_notify) {
                        cl = cops->find(sch, parentid);
+                       if (!cl)
+                               break;
                        cops->qlen_notify(sch, cl);
                }
                sch->q.qlen -= n;