[PATCH net] net/sched: hold the tree lock while resetting a qdisc being destroyed

[Eric Dumazet <edumazet@kernel.org>]

Since commit 47e8dbb6e763 ("net/sched: do not reset queues in graft
operations"), qdisc_graft() and mq_graft() no longer reset the qdisc
they remove, so it can still hold packets when qdisc_put() destroys it.

__qdisc_destroy() then calls qdisc_reset() without any lock, while
qdisc timers keep running until ops->destroy() stops them.
For instance sfq_perturbation() can rehash the SFQ queues under the
sleeping root lock, while sfq_reset() dequeues and frees the same
packets, corrupting skb lists.

__qdisc_destroy() has called qdisc_reset() without the lock its comment
requires since commit 4909daba3784 ("net_sched: use qdisc_reset() in
qdisc_destroy()"), relying on dev_deactivate() having already purged
the queue under the lock. Commit 47e8dbb6e763 ("net/sched: do not
reset queues in graft operations") removed that purge for graft
operations.

Take sch_tree_lock() around qdisc_reset() in __qdisc_destroy().
This is the lock these timers take, and the lock every other
qdisc_reset() caller holds. __qdisc_destroy() runs under RTNL and in
process context, since ops->destroy() can sleep.

Also update the qdisc_reset() comment, which named qdisc_lock(qdisc)
while most callers hold the root lock.

Fixes: 47e8dbb6e763 ("net/sched: do not reset queues in graft operations")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
The issue was discovered by an AI code review agent. It was confirmed
on a KASAN kernel, without kernel modifications, by running this loop
on four dummy devices in parallel ("fill" is a small sendmmsg() program
queuing 40000 UDP packets to 1024 different ports):

  tc qdisc replace dev $d root handle 1: tbf rate 100kbit burst 4k \
     limit 100m
  tc qdisc add dev $d parent 1:1 sfq perturb 1 flows 1024 divisor 1024 \
     depth 127 limit 100000
  ./fill $peer 40000
  sleep 0.$((RANDOM % 10))
  tc qdisc del dev $d root

It triggered within seconds a slab-use-after-free in sfq_dequeue()
called from sfq_reset(), and a general protection fault in sfq_rehash()
called from sfq_perturbation().

With this patch, 150 iterations on four devices completed without any
KASAN or lockdep report. The tc-testing qdisc selftests also pass on
a KASAN + lockdep kernel without any report.

 net/sched/sch_generic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index d2e0d20f3ba1cddb82987a559d49f6685a756cc1..e39745d58bfcd2c35cbc9b510b48062ba50ddda3 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1106,7 +1106,7 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
 }
 EXPORT_SYMBOL(qdisc_create_dflt);
 
-/* Under qdisc_lock(qdisc) and BH! */
+/* Under sch_tree_lock(qdisc) and BH! */
 
 void qdisc_reset(struct Qdisc *qdisc)
 {
@@ -1154,8 +1154,13 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
 #endif
 	gen_kill_estimator(&qdisc->rate_est);
 
+	/* A detached qdisc can still hold packets, and its timers
+	 * (sfq_perturbation(), ...) may run until ops->destroy() stops
+	 * them. They hold the sleeping root lock, take it here as well.
+	 */
+	sch_tree_lock(qdisc);
 	qdisc_reset(qdisc);
-
+	sch_tree_unlock(qdisc);
 
 	if (ops->destroy)
 		ops->destroy(qdisc);
-- 
2.53.0