From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2026-09-12 18:08:38
The CoDel control law schedules the next drop one interval/sqrt(count)
after the previous drop, using the configured interval
(codel_params.interval). For very small intervals the scheduled step
rounds down to zero, so the dropping loop in codel_dequeue() never
advances and drains the entire backlog under the qdisc lock in one
call - an unprivileged user can trigger a soft lockup this way.
Fix in the shared codel code used by both codel and fq_codel:
1. Make the control-law step at least 1 tick so the dropping loop
always moves forward.
2. Cap the dropping loop at CODEL_MAX_DROPS_PER_DEQUEUE (256) drops
per codel_dequeue() call, resyncing drop_next to now when the cap
is hit: the catch-up owed to the loop grows with the idle gap and
the backlog, which no interval threshold can bound. This is a
deliberate behaviour change after long idle gaps.
The cap applies to fq_codel (4b549a2ef4be) and the mac80211 TXQ path
(fixed interval, cap only).
The target sojourn delay (codel_params.target) is not validated: it
does not feed the control law, so a sub-tick value is aggressive
rather than deadlock-prone.
Conditions to recreate the bug:
- tc qdisc add dev lo root handle 1: tbf rate 1kbit burst 2kb limit 1000000
- tc qdisc add dev lo parent 1:1 handle 10: codel interval 2us target 1ms noecn limit 1000000 (same for fq_codel)
- unpatched kernel: tc accepts it; a UDP flood under the 1kbit tbf
soft-lockups (watchdog: BUG: soft lockup) while one
codel_dequeue() call drops the backlog under the qdisc lock
- patched kernel: same setup, at most 256 drops per dequeue call,
no soft lockup
Testing: claim reproducer and interval 2us/3us variants run clean;
tdc qdisc category passes (see the selftests patch).
Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")
Reported-by: Vega <redacted>
Reviewed-by: Eric Dumazet <redacted>
Tested-by: Victor Nogueira <redacted>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/net/codel.h | 5 +++++
include/net/codel_impl.h | 16 +++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
@@ -140,6 +140,11 @@ struct codel_vars {/* needed shift to get a Q0.32 number from rec_inv_sqrt */#define REC_INV_SQRT_SHIFT (32 - REC_INV_SQRT_BITS)+/* Cap on drops per codel_dequeue() call: the loop's work depends on the+*idlegapandbacklog,bothoutsideourcontrol;resyncwhenexceeded.+*/+#define CODEL_MAX_DROPS_PER_DEQUEUE 256+/***structcodel_stats-containscodelsharedvariablesandstats*@maxpacket:largestpacketwe'veseensofar
@@ -180,6 +186,14 @@ static struct sk_buff *codel_dequeue(void *ctx,*/while(vars->dropping&&codel_time_after_eq(now,vars->drop_next)){+if(++drops>CODEL_MAX_DROPS_PER_DEQUEUE){+/* fell far behind the schedule */+WRITE_ONCE(vars->drop_next,+codel_control_law(now,+params->interval,+vars->rec_inv_sqrt));+break;+}/* dont care of possible wrap*sincethereisnomoredivide.*/
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2026-09-12 18:08:40
Add tdc cases locking the codel/fq_codel small-interval uAPI after
the dropping-loop bound (previous patch): sub-tick and two-tick
intervals are ACCEPTED (the loop bound makes them safe), the
1024us boundary is accepted, and a sub-tick target sojourn delay is
accepted (it does not participate in the control law):
codel: 6e44/a8c3/a695/9793 - interval 1us/3us/1024us and
target 1us accepted (rendered 0us/2us/1.02ms/0us by tc)
fq_codel: 1b4d/3540/49c5/3e0f - interval 1us/3us/1024us and
target 1us accepted
The positive cases match the full rendered qdisc line (tc renders
interval 1us as 0us, 3us as 2us, 1024us as 1.02ms), mirroring the
existing tests in these files.
These cases do not test the dropping-loop bound itself: tdc cannot
observe per-dequeue drop counts. c797 (fq_codel target 1 interval 1)
passes unmodified on the patched kernel, which is the uAPI evidence
for the previous patch.
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
.../tc-testing/tc-tests/qdiscs/codel.json | 72 +++++++++++++++++++
.../tc-testing/tc-tests/qdiscs/fq_codel.json | 72 +++++++++++++++++++
2 files changed, 144 insertions(+)
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 0
- [High] The new drop cap in codel_dequeue()
(include/net/codel_impl.h:189) is enforced with a function-local…
--
net/sched: codel: bound the dropping loop per dequeue call
The patch clamps the CoDel control law increment to at least one tick so
the dropping loop always advances, and caps the loop at
CODEL_MAX_DROPS_PER_DEQUEUE (256) drops per codel_dequeue() call,
resyncing drop_next to now when the cap trips.
@@ -140,6 +140,11 @@ struct codel_vars {/* needed shift to get a Q0.32 number from rec_inv_sqrt */#define REC_INV_SQRT_SHIFT (32 - REC_INV_SQRT_BITS)+/* Cap on drops per codel_dequeue() call: the loop's work depends on the+*idlegapandbacklog,bothoutsideourcontrol;resyncwhenexceeded.+*/+#define CODEL_MAX_DROPS_PER_DEQUEUE 256+/***structcodel_stats-containscodelsharedvariablesandstats*@maxpacket:largestpacketwe'veseensofar
@@ -180,6 +186,14 @@ static struct sk_buff *codel_dequeue(void *ctx, */ while (vars->dropping && codel_time_after_eq(now, vars->drop_next)) {+ if (++drops > CODEL_MAX_DROPS_PER_DEQUEUE) {+ /* fell far behind the schedule */+ WRITE_ONCE(vars->drop_next,+ codel_control_law(now,+ params->interval,+ vars->rec_inv_sqrt));+ break;+ }
[Severity: High]
Since drops is a local of codel_dequeue(), does the cap actually bound the
work done in one fq_codel ->dequeue() call, or only the work done for one
flow?
When the dropping loop empties a flow (any flow holding no more than 256
packets), dequeue_func() returns NULL, codel_should_drop(NULL, ...) clears
vars->dropping and codel_dequeue() returns NULL. __fq_codel_dequeue() in
net/sched/sch_fq_codel.c treats that NULL as "flow is empty" and restarts:
net/sched/sch_fq_codel.c:__fq_codel_dequeue() {
...
skb = codel_dequeue(sch, &sch->qstats.backlog, &q->cparams,
&flow->cvars, &q->cstats, qdisc_pkt_len,
codel_get_enqueue_time, drop_func, dequeue_func);
if (!skb) {
/* force a pass through old_flows to prevent starvation */
if ((head == &q->new_flows) && !list_empty(&q->old_flows))
list_move_tail(&flow->flowchain, &q->old_flows);
else
list_del_init(&flow->flowchain);
goto begin;
}
...
}
The goto begin re-enters codel_dequeue() for the next flow with drops back
at 0, still inside the same fq_codel_dequeue() call and the same qdisc lock
hold. With flows configurable up to 65536, doesn't that make the aggregate
per ->dequeue() drop count O(active_flows * 256) rather than 256?
fq_codel_peek() calls the same __fq_codel_dequeue() loop, so does the same
amplification apply on the peek path?
The same reproducer from the changelog, with the flood spread over many UDP
source ports so each flow holds at most 256 packets, looks like it would
still walk the whole backlog in one dequeue under the lock:
tc qdisc add dev lo root handle 1: tbf rate 1kbit burst 2kb limit 1000000
tc qdisc add dev lo parent 1:1 handle 10: fq_codel interval 2us \
target 1ms noecn limit 1000000 flows 65536
Given that, is this changelog text accurate for fq_codel?
"The cap applies to fq_codel (4b549a2ef4be) and the mac80211 TXQ path
(fixed interval, cap only)."
"patched kernel: same setup, at most 256 drops per dequeue call,
no soft lockup"
Would a budget shared across the fq outer loop work better here, either
passed in by the caller or signalled back so that __fq_codel_dequeue()
stops retrying when the budget is exhausted? Otherwise, could the
changelog state the residual multi-flow exposure?
The mac80211 TXQ path has the same shape in fq_tin_dequeue()
(include/net/fq_impl.h), which also does goto begin while fq->lock is held,
so the per-call counter resets there too; its fixed 100ms interval keeps
the per-flow loops short, but is the structural reset worth a note as well?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-1L5H.v1.20260912080102%40mojatatu.com
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 0
- [Medium] None of the eight added tdc cases exercise the code the
previous patch ("net/sched: codel: bound the dropping loop per dequeue…
- [Medium] The subject ("add codel/fq_codel interval boundary cases"),
the body ("the 1024us boundary is accepted") and the case names…
--
selftests/tc-testing: add codel/fq_codel interval boundary cases
This adds eight tdc cases (codel 6e44/a8c3/a695/9793 and fq_codel
1b4d/3540/49c5/3e0f) that create a codel or fq_codel qdisc with small
interval and target values and match the rendered qdisc dump line. The
stated purpose is to lock the small-interval uAPI after the preceding
dropping-loop bound patch.
No Link: tags are present in the commit header.
@@ -213,5 +213,77 @@"matchPattern":"qdisc codel 1: root refcnt [0-9]+ limit 1p target 5ms interval 100ms","matchCount":"1","teardown":["$TC qdisc del dev $DEV1 handle 1: root"]+},+{+"id":"6e44",+"name":"Create CODEL with 1us interval, accepted (sub-tick, uAPI locked)",+"category":[+"qdisc",+"codel"+],+"plugins":{+"requires":"nsPlugin"+},+"setup":[],+"cmdUnderTest":"$TC qdisc add dev $DUMMY handle 1: root codel interval 1us",+"expExitCode":"0",+"verifyCmd":"$TC qdisc show dev $DUMMY",+"matchPattern":"qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 0us",+"matchCount":"1",+"teardown":["$TC qdisc del dev $DUMMY handle 1: root"]+},
[Severity: Medium]
Do any of the eight added cases actually exercise the code the previous
patch changed? Each one has an empty setup, a bare tc qdisc add as
cmdUnderTest, and a plain tc qdisc show (no -s, no traffic, no dequeue) as
verifyCmd.
Neither codel_change() in net/sched/sch_codel.c nor fq_codel_change() gained
any validation in the previous patch, which touches only include/net/codel.h
and include/net/codel_impl.h. So the acceptance of interval 1us/3us/1024us
and target 1us, and the dumped values interval 0us/2us/1.02ms and target 0us,
look identical on an unpatched kernel.
Would these cases stay green if the max_t(u32, 1, ...) clamp in
codel_control_law() or the CODEL_MAX_DROPS_PER_DEQUEUE bound in
codel_dequeue() were reverted? Similarly, would 9793 and 3e0f ("target not
in control law") keep passing if target were wrongly folded into
codel_control_law()?
The changelog says:
"These cases do not test the dropping-loop bound itself: tdc cannot
observe per-dequeue drop counts."
Is that framing accurate for the harness as a whole? codel.json case deb1,
in this same file, already drives traffic via scapyPlugin:
"plugins": {
"requires": ["nsPlugin", "scapyPlugin"]
},
"scapy": [
{
"iface": "$DEV0",
"count": 10,
...
and infra/qdiscs.json case c797 configures exactly the degenerate
configuration and asserts exact counters:
"$TC qdisc add dev $DUMMY parent 1:2 handle 10: fq_codel target 1 interval 1 flows 1",
...
"verifyCmd": "$TC -s -j qdisc show dev $DUMMY",
"matchJSON": [ ... "packets": 3, "drops": 1 ... ]
Per-dequeue granularity is indeed not observable, but aggregate drop counts
under traffic appear to be. Could the changelog say that a dynamic case was
not written, rather than that tdc cannot express one?
[ ... ]
[Severity: Medium]
Which boundary is 1024us? The subject says "interval boundary cases", the
body says "the 1024us boundary is accepted", and this case (plus fq_codel
49c5) is named "1024us interval boundary accepted", but I cannot find a
1024us threshold in codel.
include/net/codel.h has:
/* CoDel uses a 1024 nsec clock, encoded in u32
* This gives a range of 2199 seconds, because of signed compares
*/
#define CODEL_SHIFT 10
The 1024 there is nanoseconds per tick, not microseconds. Is this a ns/us
mix-up? 1024us just converts to 1000 ticks and renders as 1.02ms; there is
no clamp or validation at that value in codel.h, codel_impl.h, sch_codel.c or
sch_fq_codel.c.
The granularity boundary in the uAPI conversion looks like it sits at 2us:
net/sched/sch_codel.c:codel_change() {
u32 interval = nla_get_u32(tb[TCA_CODEL_INTERVAL]);
WRITE_ONCE(q->params.interval,
((u64)interval * NSEC_PER_USEC) >> CODEL_SHIFT);
}
giving 1us -> 0 ticks, 2us -> 1 tick, 3us -> 2 ticks. The added cases cover
1us and 3us and skip 2us, which is the value used in the previous patch's own
reproducer ("codel interval 2us target 1ms"). Could a case at 2us be added
and the 1024us wording in the subject, body and case names be corrected?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2026-09-13 10:28:07
On Sat, Sep 12, 2026 at 4:36 PM [off-list ref] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 0
- [High] The new drop cap in codel_dequeue()
(include/net/codel_impl.h:189) is enforced with a function-local…
Commenting on both this and the tdc tests:
All findings are accurate, but they are arguments against the
changelog and selftest-quality nits against an already-sent; IOW, none
is a code defect requiring a revised patch.
There is one substantive item regarding the per-flow vs per-dequeue
cap comment; i have captured it is as a follow-up candidate and dont
see as a blocker.
cheers,
jamal
--
net/sched: codel: bound the dropping loop per dequeue call
The patch clamps the CoDel control law increment to at least one tick so
the dropping loop always advances, and caps the loop at
CODEL_MAX_DROPS_PER_DEQUEUE (256) drops per codel_dequeue() call,
resyncing drop_next to now when the cap trips.
@@ -140,6 +140,11 @@ struct codel_vars {/* needed shift to get a Q0.32 number from rec_inv_sqrt */#define REC_INV_SQRT_SHIFT (32 - REC_INV_SQRT_BITS)+/* Cap on drops per codel_dequeue() call: the loop's work depends on the+*idlegapandbacklog,bothoutsideourcontrol;resyncwhenexceeded.+*/+#define CODEL_MAX_DROPS_PER_DEQUEUE 256+/***structcodel_stats-containscodelsharedvariablesandstats*@maxpacket:largestpacketwe'veseensofar
@@ -180,6 +186,14 @@ static struct sk_buff *codel_dequeue(void *ctx, */ while (vars->dropping && codel_time_after_eq(now, vars->drop_next)) {+ if (++drops > CODEL_MAX_DROPS_PER_DEQUEUE) {+ /* fell far behind the schedule */+ WRITE_ONCE(vars->drop_next,+ codel_control_law(now,+ params->interval,+ vars->rec_inv_sqrt));+ break;+ }
[Severity: High]
Since drops is a local of codel_dequeue(), does the cap actually bound the
work done in one fq_codel ->dequeue() call, or only the work done for one
flow?
When the dropping loop empties a flow (any flow holding no more than 256
packets), dequeue_func() returns NULL, codel_should_drop(NULL, ...) clears
vars->dropping and codel_dequeue() returns NULL. __fq_codel_dequeue() in
net/sched/sch_fq_codel.c treats that NULL as "flow is empty" and restarts:
net/sched/sch_fq_codel.c:__fq_codel_dequeue() {
...
skb = codel_dequeue(sch, &sch->qstats.backlog, &q->cparams,
&flow->cvars, &q->cstats, qdisc_pkt_len,
codel_get_enqueue_time, drop_func, dequeue_func);
if (!skb) {
/* force a pass through old_flows to prevent starvation */
if ((head == &q->new_flows) && !list_empty(&q->old_flows))
list_move_tail(&flow->flowchain, &q->old_flows);
else
list_del_init(&flow->flowchain);
goto begin;
}
...
}
The goto begin re-enters codel_dequeue() for the next flow with drops back
at 0, still inside the same fq_codel_dequeue() call and the same qdisc lock
hold. With flows configurable up to 65536, doesn't that make the aggregate
per ->dequeue() drop count O(active_flows * 256) rather than 256?
fq_codel_peek() calls the same __fq_codel_dequeue() loop, so does the same
amplification apply on the peek path?
The same reproducer from the changelog, with the flood spread over many UDP
source ports so each flow holds at most 256 packets, looks like it would
still walk the whole backlog in one dequeue under the lock:
tc qdisc add dev lo root handle 1: tbf rate 1kbit burst 2kb limit 1000000
tc qdisc add dev lo parent 1:1 handle 10: fq_codel interval 2us \
target 1ms noecn limit 1000000 flows 65536
Given that, is this changelog text accurate for fq_codel?
"The cap applies to fq_codel (4b549a2ef4be) and the mac80211 TXQ path
(fixed interval, cap only)."
"patched kernel: same setup, at most 256 drops per dequeue call,
no soft lockup"
Would a budget shared across the fq outer loop work better here, either
passed in by the caller or signalled back so that __fq_codel_dequeue()
stops retrying when the budget is exhausted? Otherwise, could the
changelog state the residual multi-flow exposure?
The mac80211 TXQ path has the same shape in fq_tin_dequeue()
(include/net/fq_impl.h), which also does goto begin while fq->lock is held,
so the per-call counter resets there too; its fixed 100ms interval keeps
the per-flow loops short, but is the structural reset worth a note as well?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-1L5H.v1.20260912080102%40mojatatu.com
The CoDel control law schedules the next drop one interval/sqrt(count)
after the previous drop, using the configured interval
(codel_params.interval). For very small intervals the scheduled step
rounds down to zero, so the dropping loop in codel_dequeue() never
advances and drains the entire backlog under the qdisc lock in one
call - an unprivileged user can trigger a soft lockup this way.
Fix in the shared codel code used by both codel and fq_codel:
1. Make the control-law step at least 1 tick so the dropping loop
always moves forward.
2. Cap the dropping loop at CODEL_MAX_DROPS_PER_DEQUEUE (256) drops
per codel_dequeue() call, resyncing drop_next to now when the cap
is hit: the catch-up owed to the loop grows with the idle gap and
the backlog, which no interval threshold can bound. This is a
deliberate behaviour change after long idle gaps.
Both of these seem reasonable!
Reviewed-by: Toke Høiland-Jørgensen <toke@toke.dk>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski [off-list ref]:
On Sat, 12 Sep 2026 14:08:30 -0400 you wrote:
The CoDel control law schedules the next drop one interval/sqrt(count)
after the previous drop, using the configured interval
(codel_params.interval). For very small intervals the scheduled step
rounds down to zero, so the dropping loop in codel_dequeue() never
advances and drains the entire backlog under the qdisc lock in one
call - an unprivileged user can trigger a soft lockup this way.
[...]