netdev - [PATCH net v2] sch_htb: Avoid grafting on htb_destroy_class

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-next>] [day] [month] [year] [list]

Message-Id: <20230111203732.51363-1-rrameshbabu@nvidia.com>
Date:   Wed, 11 Jan 2023 12:37:33 -0800
From:   Rahul Rameshbabu <rrameshbabu@...dia.com>
To:     netdev@...r.kernel.org
Cc:     Tariq Toukan <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        David Miller <davem@...emloft.net>,
        Rahul Rameshbabu <rrameshbabu@...dia.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Maxim Mikityanskiy <maxtram95@...il.com>
Subject: [PATCH net v2] sch_htb: Avoid grafting on htb_destroy_class_offload when destroying htb

When destroying the htb, the caller may already have grafted a new qdisc
that is not part of the htb structure being destroyed.
htb_destroy_class_offload should not peek at the qdisc of the netdev queue.
Peek at old qdisc and graft only when deleting a leaf class in the htb,
rather than when deleting the htb itself.

This fix resolves two use cases.

  1. Using tc to destroy the htb.
  2. Using tc to replace the htb with another qdisc (which also leads to
     the htb being destroyed).

Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Maxim Mikityanskiy <maxtram95@...il.com>
---
 net/sched/sch_htb.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 2238edece1a4..360ce8616fd2 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1557,14 +1557,13 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
 
 	WARN_ON(!q);
 	dev_queue = htb_offload_get_queue(cl);
-	old = htb_graft_helper(dev_queue, NULL);
-	if (destroying)
-		/* Before HTB is destroyed, the kernel grafts noop_qdisc to
-		 * all queues.
+	if (!destroying) {
+		old = htb_graft_helper(dev_queue, NULL);
+		/* Last qdisc grafted should be the same as cl->leaf.q when
+		 * calling htb_destroy
 		 */
-		WARN_ON(!(old->flags & TCQ_F_BUILTIN));
-	else
 		WARN_ON(old != q);
+	}
 
 	if (cl->parent) {
 		_bstats_update(&cl->parent->bstats_bias,
@@ -1581,10 +1580,14 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
 	};
 	err = htb_offload(qdisc_dev(sch), &offload_opt);
 
-	if (!err || destroying)
-		qdisc_put(old);
-	else
-		htb_graft_helper(dev_queue, old);
+	/* htb_offload related errors when destroying cannot be handled */
+	WARN_ON(err && destroying);
+	if (!destroying) {
+		if (!err)
+			qdisc_put(old);
+		else
+			htb_graft_helper(dev_queue, old);
+	}
 
 	if (last_child)
 		return err;
-- 
2.36.2

Previous related discussions

[1] https://lore.kernel.org/netdev/20230110202003.25452-1-rrameshbabu@nvidia.com/
[2] https://lore.kernel.org/netdev/20230104174744.22280-1-rrameshbabu@nvidia.com/
[3] https://lore.kernel.org/all/CANn89iJSsFPBp5dYm3y6Jbbpuwbb9P+X3gmqk6zow0VWgx1Q-A@mail.gmail.com/