* [PATCH net v2] net/sched: serialize autonomous timers with qdisc resets
@ 2026-11-10 4:44 Kyle Zeng
2026-11-14 12:12 ` netdev-bot+sashiko
0 siblings, 1 reply; 2+ messages in thread
From: Kyle Zeng @ 2026-11-10 4:44 UTC (permalink / raw)
To: netdev; +Cc: jhs, jiri, kuba, linux-kernel
Autonomous qdisc timers can race with queue resets during graft and
destruction. A timer can read qdisc_sleeping before a graft, acquire
that root's lock after the graft, and modify queues protected by the
replacement root's lock. __qdisc_destroy() also resets detached qdiscs
without the tree lock before their destroy callbacks stop the timers.
In SFQ, either race can corrupt skb lists and flow-slot state.
Add a helper that acquires the sleeping-root lock under RCU and retries
if the root changed. Use it in the SFQ, RED, PIE, FQ-PIE and DualPI2
timers. Route default-qdisc attachment and scheduler shutdown through
dev_graft_qdisc(), so all live root replacements hold the previous
root's lock. A validated root then remains current until the timer
unlocks.
Hold the tree lock across the final qdisc reset and release it before
calling the destroy callback so synchronous timer cancellation can
complete. Detached TAPRIO children can have different sleeping roots,
so defer their final resets to taprio_destroy() and lock each subtree
separately. Keep those resets before hardware offload is disabled.
The issue was found by an LLM.
Fixes: 225d9b89c937 ("sch_sfq: rehash queues in perturb timer")
Fixes: 47e8dbb6e763 ("net/sched: do not reset queues in graft operations")
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
Changes in v2:
- Serialize the final qdisc reset with autonomous timers, addressing
the remaining destruction race identified in review.
- Reset detached TAPRIO children under their individual root locks.
- Add the two Fixes tags.
v1: https://lore.kernel.org/netdev/20261030042312.74363-1-kylebot@openai.com/
Review: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030042312.74363-1-kylebot@openai.com
I have a reproducer for this issue and can share via DM
include/net/sch_generic.h | 20 ++++++++++++++++++++
net/sched/sch_dualpi2.c | 3 +--
net/sched/sch_fq_pie.c | 3 +--
net/sched/sch_generic.c | 11 +++++++----
net/sched/sch_pie.c | 3 +--
net/sched/sch_red.c | 3 +--
net/sched/sch_sfq.c | 3 +--
net/sched/sch_taprio.c | 16 ++++++++++++++++
8 files changed, 48 insertions(+), 14 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 1acaadb3e2cf..e234256e6ae8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -608,6 +608,26 @@ static inline spinlock_t *qdisc_root_sleeping_lock(const struct Qdisc *qdisc)
return qdisc_lock(root);
}
+/* The caller must hold rcu_read_lock(). dev_graft_qdisc() changes the
+ * sleeping root under the old root's lock, so retry if it changed while
+ * we were waiting for that lock.
+ */
+static inline spinlock_t *qdisc_lock_root_sleeping(const struct Qdisc *qdisc)
+{
+ struct Qdisc *root;
+ /* Lock of the sleeping root observed under RCU. */
+ spinlock_t *lock;
+
+ for (;;) {
+ root = qdisc_root_sleeping(qdisc);
+ lock = qdisc_lock(root);
+ spin_lock(lock);
+ if (root == qdisc_root_sleeping(qdisc))
+ return lock;
+ spin_unlock(lock);
+ }
+}
+
static inline struct net_device *qdisc_dev(const struct Qdisc *qdisc)
{
return qdisc->dev_queue->dev;
diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index 4947def7c49e..8a74ceaab29d 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c
@@ -750,8 +750,7 @@ static enum hrtimer_restart dualpi2_timer(struct hrtimer *timer)
spinlock_t *root_lock; /* to lock qdisc for probability calculations */
rcu_read_lock();
- root_lock = qdisc_lock(qdisc_root_sleeping(sch));
- spin_lock(root_lock);
+ root_lock = qdisc_lock_root_sleeping(sch);
WRITE_ONCE(q->pi2_prob, calculate_probability(sch));
hrtimer_set_expires(&q->pi2_timer, next_pi2_timeout(q));
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index 5982847df8f8..a1b5126db640 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -394,8 +394,7 @@ static void fq_pie_timer(struct timer_list *t)
int max_cnt, i;
rcu_read_lock();
- root_lock = qdisc_lock(qdisc_root_sleeping(sch));
- spin_lock(root_lock);
+ root_lock = qdisc_lock_root_sleeping(sch);
/* Limit this expensive loop to 2048 flows per round. */
max_cnt = min_t(int, q->flows_cnt - q->flows_cursor, 2048);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index d2e0d20f3ba1..fbbc07700160 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,10 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
#endif
gen_kill_estimator(&qdisc->rate_est);
+ /* Autonomous timers can still run after the qdisc was detached. */
+ sch_tree_lock(qdisc);
qdisc_reset(qdisc);
-
+ sch_tree_unlock(qdisc);
if (ops->destroy)
ops->destroy(qdisc);
@@ -1213,6 +1215,7 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
struct Qdisc *oqdisc = rtnl_dereference(dev_queue->qdisc_sleeping);
spinlock_t *root_lock;
+ /* Pairs with qdisc_lock_root_sleeping() in asynchronous callbacks. */
root_lock = qdisc_lock(oqdisc);
spin_lock_bh(root_lock);
@@ -1236,8 +1239,8 @@ static void shutdown_scheduler_queue(struct net_device *dev,
struct Qdisc *qdisc_default = _qdisc_default;
if (qdisc) {
+ qdisc = dev_graft_qdisc(dev_queue, qdisc_default);
rcu_assign_pointer(dev_queue->qdisc, qdisc_default);
- rcu_assign_pointer(dev_queue->qdisc_sleeping, qdisc_default);
qdisc_put(qdisc);
}
@@ -1261,7 +1264,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
if (!netif_is_multiqueue(dev))
qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
- rcu_assign_pointer(dev_queue->qdisc_sleeping, qdisc);
+ dev_graft_qdisc(dev_queue, qdisc);
}
static void attach_default_qdiscs(struct net_device *dev)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 3b7863ffd284..9367065db4b1 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -429,8 +429,7 @@ static void pie_timer(struct timer_list *t)
spinlock_t *root_lock;
rcu_read_lock();
- root_lock = qdisc_lock(qdisc_root_sleeping(sch));
- spin_lock(root_lock);
+ root_lock = qdisc_lock_root_sleeping(sch);
pie_calculate_probability(&q->params, &q->vars, sch->qstats.backlog);
/* reset the timer to fire after 'tupdate'. tupdate is in jiffies. */
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index d7598214270b..a7601b879092 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -333,8 +333,7 @@ static inline void red_adaptative_timer(struct timer_list *t)
spinlock_t *root_lock;
rcu_read_lock();
- root_lock = qdisc_lock(qdisc_root_sleeping(sch));
- spin_lock(root_lock);
+ root_lock = qdisc_lock_root_sleeping(sch);
red_adaptative_algo(&q->parms, &q->vars);
mod_timer(&q->adapt_timer, jiffies + HZ/2);
spin_unlock(root_lock);
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8bbcfc9e85d9..2bd3fc65053a 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -611,8 +611,7 @@ static void sfq_perturbation(struct timer_list *t)
get_random_bytes(&nkey, sizeof(nkey));
rcu_read_lock();
- root_lock = qdisc_lock(qdisc_root_sleeping(sch));
- spin_lock(root_lock);
+ root_lock = qdisc_lock_root_sleeping(sch);
q->perturbation = nkey;
if (!q->filter_list && q->tail)
sfq_rehash(sch);
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 4f69ef3d847a..b7489cf4bba9 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2041,6 +2041,10 @@ static void taprio_reset(struct Qdisc *sch)
hrtimer_cancel(&q->advance_timer);
+ /* Destroy resets each detached subtree under its own root lock. */
+ if (!refcount_read(&sch->refcnt))
+ return;
+
if (q->qdiscs) {
for (i = 0; i < dev->num_tx_queues; i++)
if (q->qdiscs[i])
@@ -2063,6 +2067,18 @@ static void taprio_destroy(struct Qdisc *sch)
hrtimer_cancel(&q->advance_timer);
qdisc_synchronize(sch);
+ if (q->qdiscs) {
+ for (i = 0; i < dev->num_tx_queues; i++) {
+ struct Qdisc *child = q->qdiscs[i];
+
+ if (!child)
+ continue;
+ sch_tree_lock(child);
+ qdisc_reset(child);
+ sch_tree_unlock(child);
+ }
+ }
+
taprio_disable_offload(dev, q, NULL);
if (q->qdiscs) {
base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
--
2.55.0.openai.911.g06873a30d2f3
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH net v2] net/sched: serialize autonomous timers with qdisc resets
2026-11-10 4:44 [PATCH net v2] net/sched: serialize autonomous timers with qdisc resets Kyle Zeng
@ 2026-11-14 12:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-11-14 12:12 UTC (permalink / raw)
To: kylebot; +Cc: netdev, jhs, jiri, kuba, linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The fix does not cover HTB offload mode.
- [High] net/sched: sfq_perturbation() runs on detached child before
qdisc_reset()
- [Medium] The new qdisc_lock_root_sleeping() relies on a stated
invariant: the sleeping root only changes in dev_graft_qdisc(), under…
--
net/sched: serialize autonomous timers with qdisc resets
This adds qdisc_lock_root_sleeping(). The helper takes the sleeping
root's lock under RCU and retries if the root changed while it waited.
The SFQ, RED, PIE, FQ-PIE and DualPI2 timers now use it.
Default qdisc attach and shutdown now go through dev_graft_qdisc().
__qdisc_destroy() holds the tree lock across the final qdisc_reset().
The final resets of TAPRIO children move to taprio_destroy().
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 1acaadb3e2cf0..e234256e6ae81 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -608,6 +608,26 @@ static inline spinlock_t *qdisc_root_sleeping_lock(const struct Qdisc *qdisc)
> return qdisc_lock(root);
> }
>
> +/* The caller must hold rcu_read_lock(). dev_graft_qdisc() changes the
> + * sleeping root under the old root's lock, so retry if it changed while
> + * we were waiting for that lock.
> + */
> +static inline spinlock_t *qdisc_lock_root_sleeping(const struct Qdisc *qdisc)
> +{
> + struct Qdisc *root;
> + /* Lock of the sleeping root observed under RCU. */
> + spinlock_t *lock;
> +
> + for (;;) {
> + root = qdisc_root_sleeping(qdisc);
> + lock = qdisc_lock(root);
> + spin_lock(lock);
> + if (root == qdisc_root_sleeping(qdisc))
> + return lock;
> + spin_unlock(lock);
> + }
> +}
[Severity: Medium]
Is the invariant in this comment complete? qdisc_root_sleeping() reads
qdisc->dev_queue->qdisc_sleeping. qdisc->dev_queue can also change, and
there it changes without any root lock held:
net/sched/sch_htb.c:htb_offload_move_qdisc() {
...
if (!(cl_old->leaf.q->flags & TCQ_F_BUILTIN))
cl_old->leaf.q->dev_queue = queue_new;
cl_old->leaf.offload_queue = queue_new;
...
}
When destroying is true, both dev_graft_qdisc() calls around this store
are skipped.
One possible sequence:
1. An HTB offload root is replaced by mq. At the same time, an unlocked
filter request holds a reference on the old HTB through
__tcf_qdisc_find() -> qdisc_refcount_inc_nz().
2. mq_attach() grafts each txq to its own mq child. queue_old and
queue_new now have different sleeping roots, O and N.
3. The delayed final put calls htb_destroy() ->
htb_destroy_class_offload(..., destroying=true). The driver reports
a moved classid. htb_offload_move_qdisc() then switches the moved
leaf L from queue_old to queue_new with no lock held.
4. htb_destroy_class(moved_cl) -> qdisc_put(L) -> __qdisc_destroy(L) ->
sch_tree_lock(L) now takes N's lock and calls qdisc_reset(L).
5. Suppose L is an SFQ whose sfq_perturbation() validated O before the
switch and still holds O's lock. sfq_rehash() and sfq_reset() would
then run at the same time on L.
Can this still reach the final-reset-vs-timer corruption this patch is
meant to close?
The two plain reads of qdisc->dev_queue in qdisc_lock_root_sleeping()
also race with the plain store in htb_offload_move_qdisc().
The window looks narrow, since the timer has to hold O's lock from
before the dev_queue switch until __qdisc_destroy() takes N's lock. It
looks reachable on PREEMPT_RT or with vCPU preemption.
[ ... ]
> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> index 8bbcfc9e85d94..2bd3fc65053ac 100644
> --- a/net/sched/sch_sfq.c
> +++ b/net/sched/sch_sfq.c
> @@ -611,8 +611,7 @@ static void sfq_perturbation(struct timer_list *t)
>
> get_random_bytes(&nkey, sizeof(nkey));
> rcu_read_lock();
> - root_lock = qdisc_lock(qdisc_root_sleeping(sch));
> - spin_lock(root_lock);
> + root_lock = qdisc_lock_root_sleeping(sch);
> q->perturbation = nkey;
> if (!q->filter_list && q->tail)
> sfq_rehash(sch);
[Severity: High]
Does this close the window for a detached tree that still holds
packets?
Since 47e8dbb6e763, a root qdisc_graft() calls
dev_deactivate(dev, false). The old tree therefore keeps its packets
until this qdisc_reset() runs. Several things happen before we get
here:
- dev_graft_qdisc() has switched every txq's sleeping root.
- qdisc_graft() has done rcu_assign_pointer(dev->qdisc, new ? :
&noop_qdisc).
- __qdisc_destroy() itself has done qdisc_hash_del().
- gen_kill_estimator() has run, and it can block in
timer_shutdown_sync().
During that window, an SFQ child C of the old root still has q->tail
set and an armed perturb timer. One possible sequence, for example
deleting a root prio 1: that has an sfq child with perturb set:
CPU1 (RTNL) CPU2 (perturb timer)
qdisc_graft(dev, NULL, ...)
dev_graft_qdisc() per txq
dev->qdisc = &noop_qdisc
notify_and_destroy()
qdisc_put(old)
__qdisc_destroy(old)
qdisc_hash_del(old)
gen_kill_estimator()
sfq_perturbation(C)
qdisc_lock_root_sleeping(C)
locks new root, passes
sfq_rehash(C)
qdisc_tree_reduce_backlog(C)
sch_tree_lock(old)
qdisc_reset(old)
The timer validates the new sleeping root. That is the same lock
sch_tree_lock(old) takes later, so the rehash and the reset are
serialized.
However, the tree that C belongs to is no longer reachable.
qdisc_tree_reduce_backlog() looks up TC_H_MAJ(C->parent) with
qdisc_lookup_rcu() on the live device. The old root is gone from
dev->qdisc and from the hash, so the lookup returns NULL and hits
WARN_ON_ONCE(parentid != TC_H_ROOT).
If some live qdisc happens to match that major handle, the dropped
count and drop_len are subtracted from its q.qlen and
qstats.backlog instead. Those packets never belonged to it, so its
q.qlen can underflow.
As far as I can tell, qdisc_tree_reduce_backlog() no longer returns
early when n and len are both 0. A rehash that drops nothing
therefore still walks the parents.
This needs only CAP_NET_ADMIN in a network namespace owned by an
unprivileged user namespace. With panic_on_warn, the WARN becomes a
panic.
The timer behaviour predates this patch. But the commit message says
the validated root remains current until the timer unlocks, and that
the final reset is now serialized with autonomous timers. Holding a
current root's lock does not mean sch is still part of that root's
tree.
Should the old tree be emptied before it is unpublished, as
dev_deactivate() did before 47e8dbb6e763? Or should the autonomous
timers be stopped, or skip their work on a detached qdisc, before
the hierarchy is unlinked?
[Severity: High]
Does this cover HTB offload mode?
In offload mode, htb_init() sets TCQ_F_MQROOT on the HTB qdisc, so
sch_tree_lock(htb) takes qdisc_lock(htb) itself. Each offloaded leaf is
attached directly to its own txq. qdisc_root_sleeping(leaf) is
therefore that txq's qdisc_sleeping, never the HTB:
net/sched/sch_htb.c:htb_graft() {
...
if (q->offload) {
/* One ref for cl->leaf.q, the other for dev_queue->qdisc. */
qdisc_refcount_inc(new);
old_q = htb_graft_helper(dev_queue, new);
}
*old = qdisc_replace(sch, new, &cl->leaf.q);
...
}
htb_graft_helper() -> dev_graft_qdisc() sets dev_queue->qdisc_sleeping
to new. qdisc_replace() -> sch_tree_lock(sch) then takes the HTB's own
lock. Under that lock, qdisc_purge_queue(old) -> qdisc_reset(old) ->
sfq_reset() runs.
With an HTB offload capable device (for example mlx5) and an SFQ leaf
that has perturb set, can the following happen?
CPU1 (RTNL) CPU2 (perturb timer)
htb_graft()
htb_graft_helper(dev_queue, new)
sfq_perturbation(old)
qdisc_lock_root_sleeping(old)
locks qdisc_lock(new), passes
qdisc_replace()
sch_tree_lock(htb)
qdisc_purge_queue(old)
sfq_reset(old) sfq_rehash(old)
The reset holds the HTB's lock and the timer holds new's lock, so
nothing serializes them. Would this corrupt the slot lists, q->tail,
q->ht and the per-slot skb lists that the commit message describes?
htb_delete() in offload mode seems to follow the same pattern. First,
htb_destroy_class_offload(..., false, ...) calls
htb_graft_helper(dev_queue, NULL). The txq's sleeping root becomes
noop_qdisc, or new_q after htb_parent_to_leaf_offload() for a last
child. Then:
net/sched/sch_htb.c:htb_delete() {
...
sch_tree_lock(sch);
if (!cl->level)
qdisc_purge_queue(cl->leaf.q);
...
}
Meanwhile the leaf's timer takes noop_qdisc's lock or new_q's lock.
This race existed before this patch. However, the commit message says
the patch closes timer races "with queue resets during graft". Should
qdisc_replace() and qdisc_purge_queue() under an MQROOT parent also be
serialized against the child's sleeping-root lock?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110044450.51921-1-kylebot%40openai.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-11-14 12:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-10 4:44 [PATCH net v2] net/sched: serialize autonomous timers with qdisc resets Kyle Zeng
2026-11-14 12:12 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®