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 Cc: Jiri Pirko Signed-off-by: Eric Dumazet --- 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