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

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

[Victor Nogueira]

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.

[1] https://lore.kernel.org/netdev/d912cbd7-193b-4269-9857-525bee8bbb6a@gmail.com/

Fixes: 5e50da01d0ce ("[NET_SCHED]: Fix endless loops (part 2): "simple" qdiscs")
Reported-by: syzbot+d8b58d7b0ad89a678a16@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/68663c93.a70a0220.5d25f.0857.GAE@google.com/
Reported-by: syzbot+5eccb463fa89309d8bdc@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/68663c94.a70a0220.5d25f.0858.GAE@google.com/
Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/686764a5.a00a0220.c7b3.0013.GAE@google.com/
Reported-by: syzbot+15b96fc3aac35468fe77@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/686764a5.a00a0220.c7b3.0014.GAE@google.com/
Reported-by: syzbot+4dadc5aecf80324d5a51@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/68679e81.a70a0220.29cf51.0016.GAE@google.com/
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_api.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index d8a33486c511..659ab1c38518 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -336,17 +336,22 @@ struct Qdisc *qdisc_lookup_rcu(struct net_device *dev, u32 handle)
 	return q;
 }
 
-static struct Qdisc *qdisc_leaf(struct Qdisc *p, u32 classid)
+static struct Qdisc *qdisc_leaf(struct Qdisc *p, u32 classid,
+				struct netlink_ext_ack *extack)
 {
 	unsigned long cl;
 	const struct Qdisc_class_ops *cops = p->ops->cl_ops;
 
-	if (cops == NULL)
-		return NULL;
+	if (cops == NULL) {
+		NL_SET_ERR_MSG(extack, "Parent qdisc is not classful");
+		return ERR_PTR(-EOPNOTSUPP);
+	}
 	cl = cops->find(p, classid);
 
-	if (cl == 0)
-		return NULL;
+	if (cl == 0) {
+		NL_SET_ERR_MSG(extack, "Specified class not found");
+		return ERR_PTR(-ENOENT);
+	}
 	return cops->leaf(p, cl);
 }
 
@@ -1490,16 +1495,20 @@ static int __tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 					NL_SET_ERR_MSG(extack, "Failed to find qdisc with specified classid");
 					return -ENOENT;
 				}
-				q = qdisc_leaf(p, clid);
+				q = qdisc_leaf(p, clid, extack);
 			} else if (dev_ingress_queue(dev)) {
 				q = rtnl_dereference(dev_ingress_queue(dev)->qdisc_sleeping);
 			}
 		} else {
 			q = rtnl_dereference(dev->qdisc);
 		}
-		if (!q) {
-			NL_SET_ERR_MSG(extack, "Cannot find specified qdisc on specified device");
-			return -ENOENT;
+		if (IS_ERR_OR_NULL(q)) {
+			if (!q) {
+				NL_SET_ERR_MSG(extack,
+					       "Cannot find specified qdisc on specified device");
+				return -ENOENT;
+			}
+			return PTR_ERR(q);
 		}
 
 		if (tcm->tcm_handle && q->handle != tcm->tcm_handle) {
@@ -1602,7 +1611,9 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 					NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
 					return -ENOENT;
 				}
-				q = qdisc_leaf(p, clid);
+				q = qdisc_leaf(p, clid, extack);
+				if (IS_ERR(q))
+					return PTR_ERR(q);
 			} else if (dev_ingress_queue_create(dev)) {
 				q = rtnl_dereference(dev_ingress_queue(dev)->qdisc_sleeping);
 			}
-- 
2.34.1

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

[Cong Wang]

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;

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

[Jamal Hadi Salim]

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

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

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

[Jakub Kicinski]

On Fri,  4 Jul 2025 13:34:22 -0300 Victor Nogueira wrote:
> -		if (!q) {
> -			NL_SET_ERR_MSG(extack, "Cannot find specified qdisc on specified device");
> -			return -ENOENT;
> +		if (IS_ERR_OR_NULL(q)) {
> +			if (!q) {
> +				NL_SET_ERR_MSG(extack,
> +					       "Cannot find specified qdisc on specified device");
> +				return -ENOENT;
> +			}
> +			return PTR_ERR(q);
>  		}

nit: could you leave the existing handling as is?

	if (!q) {
		NL_SET_ERR_MSG(extack, "Cannot find specified qdisc on specified device");
		return -ENOENT;
	}
	if (IS_ERR(q))
		return PTR_ERR(q);

the double conditions increase indentation and confuse static analyzers
-- 
pw-bot: cr