[PATCH net repost 1/2] net/sched: codel: bound the dropping loop per dequeue call

[PATCH net repost 1/2] net/sched: codel: bound the dropping loop per dequeue call

[Jamal Hadi Salim]

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 <vega@nebusec.ai>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
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(-)

diff --git a/include/net/codel.h b/include/net/codel.h
index aa80f744826c..183d43c2bd43 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -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
+ * idle gap and backlog, both outside our control; resync when exceeded.
+ */
+#define CODEL_MAX_DROPS_PER_DEQUEUE 256
+
 /**
  * struct codel_stats - contains codel shared variables and stats
  * @maxpacket:	largest packet we've seen so far
diff --git a/include/net/codel_impl.h b/include/net/codel_impl.h
index 2c1f0ec309e9..8f26132d45b7 100644
--- a/include/net/codel_impl.h
+++ b/include/net/codel_impl.h
@@ -93,12 +93,17 @@ static void codel_Newton_step(struct codel_vars *vars)
  * CoDel control_law is t + interval/sqrt(count)
  * We maintain in rec_inv_sqrt the reciprocal value of sqrt(count) to avoid
  * both sqrt() and divide operation.
+ *
+ * Clamp the increment to at least 1 tick: a very small interval (or a
+ * large count) can truncate it to zero, stalling the dropping loop.
  */
 static codel_time_t codel_control_law(codel_time_t t,
 				      codel_time_t interval,
 				      u32 rec_inv_sqrt)
 {
-	return t + reciprocal_scale(interval, rec_inv_sqrt << REC_INV_SQRT_SHIFT);
+	return t + max_t(u32, 1,
+			 reciprocal_scale(interval,
+					  rec_inv_sqrt << REC_INV_SQRT_SHIFT));
 }
 
 static bool codel_should_drop(const struct sk_buff *skb,
@@ -154,6 +159,7 @@ static struct sk_buff *codel_dequeue(void *ctx,
 				     codel_skb_dequeue_t dequeue_func)
 {
 	struct sk_buff *skb = dequeue_func(vars, ctx);
+	unsigned int drops = 0;
 	codel_time_t now;
 	bool drop;
 
@@ -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
 				 * since there is no more divide.
 				 */
-- 
2.43.0

[PATCH net repost 2/2] selftests/tc-testing: add codel/fq_codel interval boundary cases

[Jamal Hadi Salim]

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(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
index 6d515d0e5ed6..a894e6f0e267 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
@@ -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"]
+    },
+    {
+        "id": "a8c3",
+        "name": "Create CODEL with 3us interval, accepted (two ticks)",
+        "category": [
+            "qdisc",
+            "codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel interval 3us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 2us",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
+    },
+    {
+        "id": "a695",
+        "name": "Create CODEL with 1024us interval boundary accepted",
+        "category": [
+            "qdisc",
+            "codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel interval 1024us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 1.02ms",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
+    },
+    {
+        "id": "9793",
+        "name": "Create CODEL with 1us target, accepted (target not in control law)",
+        "category": [
+            "qdisc",
+            "codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel target 1us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 0us interval 100ms",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
     }
 ]
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
index 4ce62b857fd7..de6a1b8d954a 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
@@ -316,5 +316,77 @@
         "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 1p flows 1024 quantum.*target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64",
         "matchCount": "1",
         "teardown": ["$TC qdisc del dev $DEV1 handle 1: root"]
+    },
+    {
+        "id": "1b4d",
+        "name": "Create FQ_CODEL with 1us interval, accepted (sub-tick, uAPI locked)",
+        "category": [
+            "qdisc",
+            "fq_codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel interval 1us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 5ms interval 0us memory_limit 32Mb ecn drop_batch 64",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
+    },
+    {
+        "id": "3540",
+        "name": "Create FQ_CODEL with 3us interval, accepted (two ticks)",
+        "category": [
+            "qdisc",
+            "fq_codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel interval 3us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 5ms interval 2us memory_limit 32Mb ecn drop_batch 64",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
+    },
+    {
+        "id": "49c5",
+        "name": "Create FQ_CODEL with 1024us interval boundary accepted",
+        "category": [
+            "qdisc",
+            "fq_codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel interval 1024us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 5ms interval 1.02ms memory_limit 32Mb ecn drop_batch 64",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
+    },
+    {
+        "id": "3e0f",
+        "name": "Create FQ_CODEL with 1us target, accepted (target not in control law)",
+        "category": [
+            "qdisc",
+            "fq_codel"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel target 1us",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 0us interval 100ms memory_limit 32Mb ecn drop_batch 64",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
     }
 ]
-- 
2.43.0

Re: [PATCH net repost 1/2] net/sched: codel: bound the dropping loop per dequeue call

[netdev-bot+sashiko]

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.

> diff --git a/include/net/codel.h b/include/net/codel.h
> index aa80f744826cd..183d43c2bd434 100644
> --- a/include/net/codel.h
> +++ b/include/net/codel.h
> @@ -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
> + * idle gap and backlog, both outside our control; resync when exceeded.
> + */
> +#define CODEL_MAX_DROPS_PER_DEQUEUE 256
> +
>  /**
>   * struct codel_stats - contains codel shared variables and stats
>   * @maxpacket:	largest packet we've seen so far

[ ... ]

> diff --git a/include/net/codel_impl.h b/include/net/codel_impl.h
> index 2c1f0ec309e9f..8f26132d45b7f 100644
> --- a/include/net/codel_impl.h
> +++ b/include/net/codel_impl.h

[ ... ]

> @@ -154,6 +159,7 @@ static struct sk_buff *codel_dequeue(void *ctx,
>  				     codel_skb_dequeue_t dequeue_func)
>  {
>  	struct sk_buff *skb = dequeue_func(vars, ctx);
> +	unsigned int drops = 0;
>  	codel_time_t now;
>  	bool drop;
>  
> @@ -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

Re: [PATCH net repost 2/2] selftests/tc-testing: add codel/fq_codel interval boundary cases

[netdev-bot+sashiko]

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.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> index 6d515d0e5ed69..a894e6f0e2677 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> @@ -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?

[ ... ]

> +    {
> +        "id": "a695",
> +        "name": "Create CODEL with 1024us interval boundary accepted",
> +        "category": [
> +            "qdisc",
> +            "codel"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel interval 1024us",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 1.02ms",
> +        "matchCount": "1",
> +        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
> +    },

[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?

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> index 4ce62b857fd7a..de6a1b8d954ae 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> @@ -316,5 +316,77 @@

[ ... ]

> +    {
> +        "id": "49c5",
> +        "name": "Create FQ_CODEL with 1024us interval boundary accepted",
> +        "category": [
> +            "qdisc",
> +            "fq_codel"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel interval 1024us",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 5ms interval 1.02ms memory_limit 32Mb ecn drop_batch 64",
> +        "matchCount": "1",
> +        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
> +    },

The same two questions apply to the fq_codel cases 1b4d/3540/49c5/3e0f: they
are config-only adds plus a show, and 49c5 carries the same 1024us boundary
naming.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-1L5H.v1.20260912080102%40mojatatu.com

Re: [PATCH net repost 1/2] net/sched: codel: bound the dropping loop per dequeue call

[Jamal Hadi Salim]

On Sat, Sep 12, 2026 at 4:36 PM <netdev-bot+sashiko@kernel.org> 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.
>
> > diff --git a/include/net/codel.h b/include/net/codel.h
> > index aa80f744826cd..183d43c2bd434 100644
> > --- a/include/net/codel.h
> > +++ b/include/net/codel.h
> > @@ -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
> > + * idle gap and backlog, both outside our control; resync when exceeded.
> > + */
> > +#define CODEL_MAX_DROPS_PER_DEQUEUE 256
> > +
> >  /**
> >   * struct codel_stats - contains codel shared variables and stats
> >   * @maxpacket:       largest packet we've seen so far
>
> [ ... ]
>
> > diff --git a/include/net/codel_impl.h b/include/net/codel_impl.h
> > index 2c1f0ec309e9f..8f26132d45b7f 100644
> > --- a/include/net/codel_impl.h
> > +++ b/include/net/codel_impl.h
>
> [ ... ]
>
> > @@ -154,6 +159,7 @@ static struct sk_buff *codel_dequeue(void *ctx,
> >                                    codel_skb_dequeue_t dequeue_func)
> >  {
> >       struct sk_buff *skb = dequeue_func(vars, ctx);
> > +     unsigned int drops = 0;
> >       codel_time_t now;
> >       bool drop;
> >
> > @@ -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

Re: [PATCH net repost 1/2] net/sched: codel: bound the dropping loop per dequeue call

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> 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>