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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMkkQc+ZwEyY19oZMo_zuhTR-w2gCYdjhrvReNzcuQs9CQ@mail.gmail.com>
Date: Sat, 5 Jul 2025 10:03:00 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: Victor Nogueira <victor@...atatu.com>, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, edumazet@...gle.com, jiri@...nulli.us, 
	netdev@...r.kernel.org, pctammela@...atatu.com, 
	syzbot+d8b58d7b0ad89a678a16@...kaller.appspotmail.com, 
	syzbot+5eccb463fa89309d8bdc@...kaller.appspotmail.com, 
	syzbot+1261670bbdefc5485a06@...kaller.appspotmail.com, 
	syzbot+4dadc5aecf80324d5a51@...kaller.appspotmail.com, 
	syzbot+15b96fc3aac35468fe77@...kaller.appspotmail.com
Subject: Re: [PATCH net] net/sched: Abort __tc_modify_qdisc if parent class
 does not exist

On Fri, Jul 4, 2025 at 7:54 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> 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@...il.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;
>

Sorry, meant to respond to this in last email: there's no need for
this extra piece because with the patch posted by Victor this code
will not be hit anymore.

cheers,
jamal