[PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

[PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

[Jamal Hadi Salim]

This is a followup to commit 709f34f7c28d ("net/sched: fq: add
overflow bounds to quantum and initial quantum").
The quantum_backlog_overflow series and the five siblings that followed
clamped the init-path quantum to in fq, fq_codel, fq_pie, hhf, sfq.
The change() paths with the same pattern, same writer of q->quantum,
same privilege level (CAP_NET_ADMIN in a user namespace) were not clamped.
A user can override the init clamp via tc qdisc change, restoring the
small-quantum deficit spin that the init clamp was meant to prevent.

This follow-up also covers two siblings that were missed entirely by
the original series: sch_dualpi2 and sch_pie call psched_mtu() without
any clamp at all. With a crafted size table qdisc_pkt_len reaches ~2
GiB, so quantum=1 (or a zero psched_mtu on a headerless device) makes
the deficit-refill loop spin ~2^31 times under the qdisc lock (a soft
lockup / denial of service).

Fixes based on review of 709f34f7c28d:

1. fq_pie_change() accepts quantum=1 (NLA policy fq_pie_q_range.min=1).
   Add max(256U, ...) matching fq_codel_change()
   (Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7)

2. sfq_change() accepts any non-negative quantum (only rejects
   (int)ctl->quantum < 0). Add max(256U, ...) matching fq_codel_change().
   Reject quantum > 1<<20 with -EINVAL, matching fq_codel_change() and
   the init clamp.
   (Internal review noticing same pattern)

3. hhf_change() accepts quantum=1 (only checks non_hh_quantum product).
   Add max(256U, ...) matching fq_codel_change()
   (Sashiko nipa v1 review + vega@nebusec.ai independent bug)

4. fq_change() accepts TCA_FQ_INITIAL_QUANTUM up to INT_MAX (iq_range.max
   = INT_MAX) while fq_init() now clamps to 1<<20.  Narrow iq_range.max
   to 1<<20, rejecting at parse time.  (Eric Dumazet)

5. sch_dualpi2: dualpi2_calculate_c_protection() and get_memory_limit()
   call psched_mtu() with no clamp. A huge MTU makes (s32)psched_mtu()
   overflow in the signed multiply for c_protection_init, and 2 *
   psched_mtu() wraps in get_memory_limit(). Clamp to [1, 1<<20] at
   all three call sites. (Sashiko nipa main-6-4)

6. sch_pie: pie_drop_early() calls psched_mtu() with no clamp.  With
   mtu=0x80000000 the bytemode divide silently zeroes the drop
   probability, disabling AQM. Clamp to [1, 1<<20] (Sashiko gemini)

7. sch_drr: drr_change_class() rejects explicit quantum==0 but falls
   back to psched_mtu() with no floor. Add max(256U, ...) after the
   zero reject and on the fallback path
  (vega@nebusec.ai independent bug)

8. sch_ets: ets_qdisc_change() falls back to psched_mtu() with no floor
   for bands without an explicit quantum.  Add max(256U, ...) on the
   fallback path (vega@nebusec.ai independent bug)

The init paths of fq_pie, sfq, and hhf delegate to their _change() when
opt is present, so the floor covers tc qdisc add ... quantum 1 as well
as change. The zero-quantum-from-psched_mtu case on a headerless device
(mtu==0) is also covered by the 256 floor in drr and ets; the
explicit-zero reject in drr_change_class() is preserved.

The sfq_change() silent clamp is user-visible: sfq_dump() reports the
clamped quantum, so a previously accepted quantum < 256 now reads back
as 256. Idempotent config managers that read back and compare will
see drift. fq_codel_change() made the same trade, so this is consistent.

Conditions to recreate the bug: create a fq_pie, sfq, hhf, dualpi2, or
pie qdisc (or a drr class / ets band), then tc qdisc change ... quantum 1
with a STAB size table inflating qdisc_pkt_len, e.g.:

  tc qdisc add dev dummy0 root fq_pie
  tc qdisc change dev dummy0 root fq_pie quantum 1 \
    stab data 32768 size_log 15 cell_log 0

Requires CAP_NET_ADMIN in a user namespace (unshare -Urn).

Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
Fixes: e4650d7ae425 ("net_sched: sch_sfq: handle bigger packets")
Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
Fixes: 320d031ad6e4 ("sched: Struct definition and parsing of dualpi2 qdisc")
Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")
Fixes: 13d2a1d2b032 ("pkt_sched: add DRR scheduler")
Reported-by: vega@nebusec.ai
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: stable@vger.kernel.org
---
 net/sched/sch_drr.c     |  3 ++-
 net/sched/sch_dualpi2.c | 10 +++++++---
 net/sched/sch_ets.c     |  2 +-
 net/sched/sch_fq.c      |  2 +-
 net/sched/sch_fq_pie.c  |  3 ++-
 net/sched/sch_hhf.c     |  2 +-
 net/sched/sch_pie.c     |  2 +-
 net/sched/sch_sfq.c     |  7 ++++++-
 8 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 91b1ef824afa..0ffdab27bae4 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -82,8 +82,9 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 			NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
 			return -EINVAL;
 		}
+		quantum = max(256U, quantum);
 	} else
-		quantum = psched_mtu(qdisc_dev(sch));
+		quantum = max(256U, (u32)psched_mtu(qdisc_dev(sch)));
 
 	if (cl != NULL) {
 		if (tca[TCA_RATE]) {
diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index 4f678d4ff10e..4947def7c49e 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c
@@ -208,9 +208,11 @@ static void dualpi2_reset_c_protection(struct dualpi2_sched_data *q)
 static void dualpi2_calculate_c_protection(struct Qdisc *sch,
 					   struct dualpi2_sched_data *q, u32 wc)
 {
+	u32 mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
+
 	q->c_protection_wc = wc;
 	q->c_protection_wl = MAX_WC - wc;
-	q->c_protection_init = (s32)psched_mtu(qdisc_dev(sch)) *
+	q->c_protection_init = (s32)mtu *
 		((int)q->c_protection_wc - (int)q->c_protection_wl);
 	dualpi2_reset_c_protection(q);
 }
@@ -285,8 +287,9 @@ static bool must_drop(struct Qdisc *sch, struct dualpi2_sched_data *q,
 	u64 local_l_prob;
 	bool overload;
 	u32 prob;
+	u32 mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
 
-	if (sch->qstats.backlog < 2 * psched_mtu(qdisc_dev(sch)))
+	if (sch->qstats.backlog < 2 * mtu)
 		return false;
 
 	prob = READ_ONCE(q->pi2_prob);
@@ -712,7 +715,8 @@ static u32 get_memory_limit(struct Qdisc *sch, u32 limit)
 	/* Apply rule of thumb, i.e., doubling the packet length,
 	 * to further include per packet overhead in memory_limit.
 	 */
-	u64 memlim = mul_u32_u32(limit, 2 * psched_mtu(qdisc_dev(sch)));
+	u64 memlim = mul_u32_u32(limit, 2 * clamp_t(u32, psched_mtu(qdisc_dev(sch)),
+						     1, 1 << 20));
 
 	if (upper_32_bits(memlim))
 		return U32_MAX;
diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index 25fcf4079fec..f23c8dc68f8c 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -636,7 +636,7 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 	 */
 	for (i = nstrict; i < nbands; i++) {
 		if (!quanta[i])
-			quanta[i] = psched_mtu(qdisc_dev(sch));
+			quanta[i] = max(256U, (u32)psched_mtu(qdisc_dev(sch)));
 	}
 
 	/* Before commit, make sure we can allocate all new qdiscs */
diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 6144b5686f13..ab8e7c6ae203 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -980,7 +980,7 @@ static int fq_resize(struct Qdisc *sch, u32 log)
 }
 
 static const struct netlink_range_validation iq_range = {
-	.max = INT_MAX,
+	.max = 1 << 20,
 };
 
 static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index b27d95418707..5982847df8f8 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -341,7 +341,8 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
 			   nla_get_u32(tb[TCA_FQ_PIE_BETA]));
 
 	if (tb[TCA_FQ_PIE_QUANTUM])
-		WRITE_ONCE(q->quantum, nla_get_u32(tb[TCA_FQ_PIE_QUANTUM]));
+		WRITE_ONCE(q->quantum,
+			   max(256U, nla_get_u32(tb[TCA_FQ_PIE_QUANTUM])));
 
 	if (tb[TCA_FQ_PIE_MEMORY_LIMIT])
 		WRITE_ONCE(q->memory_limit,
diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index 96acab6a8da0..bb8e8952f555 100644
--- a/net/sched/sch_hhf.c
+++ b/net/sched/sch_hhf.c
@@ -551,7 +551,7 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt,
 		return err;
 
 	if (tb[TCA_HHF_QUANTUM])
-		new_quantum = nla_get_u32(tb[TCA_HHF_QUANTUM]);
+		new_quantum = max(256U, nla_get_u32(tb[TCA_HHF_QUANTUM]));
 
 	if (tb[TCA_HHF_NON_HH_WEIGHT])
 		new_hhf_non_hh_weight = nla_get_u32(tb[TCA_HHF_NON_HH_WEIGHT]);
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index b41f2def2e2c..3b7863ffd284 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -35,7 +35,7 @@ bool pie_drop_early(struct Qdisc *sch, struct pie_params *params,
 {
 	u64 rnd;
 	u64 local_prob = vars->prob;
-	u32 mtu = psched_mtu(qdisc_dev(sch));
+	u32 mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
 
 	/* If there is still burst allowance left skip random early drop */
 	if (vars->burst_time > 0)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 187d3ed578f2..8bbcfc9e85d9 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -660,6 +660,11 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
 		return -EINVAL;
 	}
 
+	if (ctl->quantum > 1 << 20) {
+		NL_SET_ERR_MSG_MOD(extack, "quantum too large");
+		return -EINVAL;
+	}
+
 	if (ctl->perturb_period < 0 ||
 	    ctl->perturb_period > INT_MAX / HZ) {
 		NL_SET_ERR_MSG_MOD(extack, "invalid perturb period");
@@ -688,7 +693,7 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
 
 	/* update and validate configuration */
 	if (ctl->quantum)
-		quantum = ctl->quantum;
+		quantum = max(256U, ctl->quantum);
 	if (ctl->flows)
 		maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);
 	if (ctl->divisor) {
-- 
2.43.0

Re: [PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

[Toke Høiland-Jørgensen]

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

> This is a followup to commit 709f34f7c28d ("net/sched: fq: add
> overflow bounds to quantum and initial quantum").
> The quantum_backlog_overflow series and the five siblings that followed
> clamped the init-path quantum to in fq, fq_codel, fq_pie, hhf, sfq.
> The change() paths with the same pattern, same writer of q->quantum,
> same privilege level (CAP_NET_ADMIN in a user namespace) were not clamped.
> A user can override the init clamp via tc qdisc change, restoring the
> small-quantum deficit spin that the init clamp was meant to prevent.
>
> This follow-up also covers two siblings that were missed entirely by
> the original series: sch_dualpi2 and sch_pie call psched_mtu() without
> any clamp at all. With a crafted size table qdisc_pkt_len reaches ~2
> GiB, so quantum=1 (or a zero psched_mtu on a headerless device) makes
> the deficit-refill loop spin ~2^31 times under the qdisc lock (a soft
> lockup / denial of service).
>
> Fixes based on review of 709f34f7c28d:
>
> 1. fq_pie_change() accepts quantum=1 (NLA policy fq_pie_q_range.min=1).
>    Add max(256U, ...) matching fq_codel_change()
>    (Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7)
>
> 2. sfq_change() accepts any non-negative quantum (only rejects
>    (int)ctl->quantum < 0). Add max(256U, ...) matching fq_codel_change().
>    Reject quantum > 1<<20 with -EINVAL, matching fq_codel_change() and
>    the init clamp.
>    (Internal review noticing same pattern)
>
> 3. hhf_change() accepts quantum=1 (only checks non_hh_quantum product).
>    Add max(256U, ...) matching fq_codel_change()
>    (Sashiko nipa v1 review + vega@nebusec.ai independent bug)
>
> 4. fq_change() accepts TCA_FQ_INITIAL_QUANTUM up to INT_MAX (iq_range.max
>    = INT_MAX) while fq_init() now clamps to 1<<20.  Narrow iq_range.max
>    to 1<<20, rejecting at parse time.  (Eric Dumazet)
>
> 5. sch_dualpi2: dualpi2_calculate_c_protection() and get_memory_limit()
>    call psched_mtu() with no clamp. A huge MTU makes (s32)psched_mtu()
>    overflow in the signed multiply for c_protection_init, and 2 *
>    psched_mtu() wraps in get_memory_limit(). Clamp to [1, 1<<20] at
>    all three call sites. (Sashiko nipa main-6-4)
>
> 6. sch_pie: pie_drop_early() calls psched_mtu() with no clamp.  With
>    mtu=0x80000000 the bytemode divide silently zeroes the drop
>    probability, disabling AQM. Clamp to [1, 1<<20] (Sashiko gemini)
>
> 7. sch_drr: drr_change_class() rejects explicit quantum==0 but falls
>    back to psched_mtu() with no floor. Add max(256U, ...) after the
>    zero reject and on the fallback path
>   (vega@nebusec.ai independent bug)
>
> 8. sch_ets: ets_qdisc_change() falls back to psched_mtu() with no floor
>    for bands without an explicit quantum.  Add max(256U, ...) on the
>    fallback path (vega@nebusec.ai independent bug)
>
> The init paths of fq_pie, sfq, and hhf delegate to their _change() when
> opt is present, so the floor covers tc qdisc add ... quantum 1 as well
> as change. The zero-quantum-from-psched_mtu case on a headerless device
> (mtu==0) is also covered by the 256 floor in drr and ets; the
> explicit-zero reject in drr_change_class() is preserved.
>
> The sfq_change() silent clamp is user-visible: sfq_dump() reports the
> clamped quantum, so a previously accepted quantum < 256 now reads back
> as 256. Idempotent config managers that read back and compare will
> see drift. fq_codel_change() made the same trade, so this is consistent.
>
> Conditions to recreate the bug: create a fq_pie, sfq, hhf, dualpi2, or
> pie qdisc (or a drr class / ets band), then tc qdisc change ... quantum 1
> with a STAB size table inflating qdisc_pkt_len, e.g.:
>
>   tc qdisc add dev dummy0 root fq_pie
>   tc qdisc change dev dummy0 root fq_pie quantum 1 \
>     stab data 32768 size_log 15 cell_log 0
>
> Requires CAP_NET_ADMIN in a user namespace (unshare -Urn).
>
> Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Fixes: e4650d7ae425 ("net_sched: sch_sfq: handle bigger packets")
> Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
> Fixes: 320d031ad6e4 ("sched: Struct definition and parsing of dualpi2 qdisc")
> Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")
> Fixes: 13d2a1d2b032 ("pkt_sched: add DRR scheduler")
> Reported-by: vega@nebusec.ai
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: clamp quantum and psched_mtu in change paths and missed siblings

This follow-up to 709f34f7c28d adds a 256 byte floor to the quantum written
by the change() paths of fq_pie, hhf, sfq and drr, floors the psched_mtu()
fallback in drr and ets, narrows the sch_fq TCA_FQ_INITIAL_QUANTUM policy
range to 1<<20, and clamps psched_mtu() to [1, 1<<20] in sch_dualpi2 and
sch_pie. The stated goal is to stop a small quantum combined with a
STAB-inflated qdisc_pkt_len from spinning the deficit-refill loops under the
qdisc lock.

A few questions on the changelog first.

>     8. sch_ets: ets_qdisc_change() falls back to psched_mtu() with no floor
>        for bands without an explicit quantum.  Add max(256U, ...) on the
>        fallback path (vega@nebusec.ai independent bug)

[Severity: Low]

The ETS item is described as fixing a real denial-of-service condition, but
none of the seven Fixes: tags names an ETS commit. git blame on the
psched_mtu() fallback in ets_qdisc_change() at the baseline points at
dcc68b4d8084 ("net: sch_ets: Add a new Qdisc"), which is not in the tag
list. Should the following be added so stable tooling can map the ETS part?

  Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")

>     1. fq_pie_change() accepts quantum=1 (NLA policy fq_pie_q_range.min=1).
>        Add max(256U, ...) matching fq_codel_change()
>        (Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7)
[ ... ]
>     5. sch_dualpi2: dualpi2_calculate_c_protection() and get_memory_limit()
>        call psched_mtu() with no clamp. A huge MTU makes (s32)psched_mtu()
>        overflow in the signed multiply for c_protection_init, and 2 *
>        psched_mtu() wraps in get_memory_limit(). Clamp to [1, 1<<20] at
>        all three call sites. (Sashiko nipa main-6-4)

[Severity: Low]

This patch bundles eight independent fixes over seven qdiscs with seven
Fixes: tags and Cc: stable, covering different bug classes: deficit-spin
quantum floors, the signed multiply overflow in dualpi2 c_protection_init,
the bytemode divide in pie_drop_early(), and a netlink policy narrowing.
The tags span very different stable ranges (320d031ad6e4 for dualpi2 is
recent, 13d2a1d2b032 for drr is from 2008). Can this be split so each bug
can be backported or reverted on its own?

The changelog also carries what look like internal review-tool
identifiers - "Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7",
"Sashiko nipa main-6-4", "Sashiko gemini", "Internal review noticing same
pattern". Are those meant to stay in the upstream commit message?

> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index 91b1ef824afaf..0ffdab27bae42 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -82,8 +82,9 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  			NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
>  			return -EINVAL;
>  		}
> +		quantum = max(256U, quantum);
>  	} else
> -		quantum = psched_mtu(qdisc_dev(sch));
> +		quantum = max(256U, (u32)psched_mtu(qdisc_dev(sch)));

[Severity: High]

Only the low side is bounded here. drr_change_class() still accepts any
non-zero u32, and cl->quantum and cl->deficit are both u32. What happens
with a quantum near U32_MAX?

  tc class add ... drr quantum 4294967293

net/sched/sch_drr.c:drr_dequeue() {
	...
	WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum));
	list_move_tail(&cl->alist, &q->active);
	...
}

With quantum = 0xfffffffd that refill wraps modulo 2^32 and is the same as
deficit -= 3, so with a STAB-inflated qdisc_pkt_len of 0x40000000 the loop
needs roughly 3.6e8 passes before deficit wraps past zero, all with the
qdisc root lock held. ets_qdisc_dequeue() uses the identical u32 arithmetic
and ets_quantum_parse() rejects only zero.

Should drr and ets also reject quantum > 1<<20, the way sfq_change() now
does in this same patch?

> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 25fcf4079fece..f23c8dc68f8cb 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -636,7 +636,7 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  	 */
>  	for (i = nstrict; i < nbands; i++) {
>  		if (!quanta[i])
> -			quanta[i] = psched_mtu(qdisc_dev(sch));
> +			quanta[i] = max(256U, (u32)psched_mtu(qdisc_dev(sch)));

[Severity: High]

This assignment is guarded by !quanta[i], so it only applies to bands that
carry no explicit quantum. Every explicit value still goes through
ets_quantum_parse(), which rejects zero only:

net/sched/sch_ets.c:ets_quantum_parse() {
	*quantum = nla_get_u32(attr);
	if (!*quantum) {
		NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
		return -EINVAL;
	}
	return 0;
}

So does the reproducer in the changelog still work for ETS?

  tc qdisc add dev dummy0 root ets bands 2 quanta 1 1

ets_class_change() has the same gap, it writes the parsed value straight
through with WRITE_ONCE(cl->quantum, quantum), and then
ets_qdisc_dequeue() refills one byte per pass:

net/sched/sch_ets.c:ets_qdisc_dequeue() {
	...
	cl->deficit += READ_ONCE(cl->quantum);
	list_move_tail(&cl->alist, &q->active);
	...
}

Would putting the floor in ets_quantum_parse() cover the explicit path as
well? If so, test 41f5 in
tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json expects
"quanta 4294967294 1 1" to be accepted and dumped unchanged, so it would
need updating.

[Severity: Low]

The comment just above this loop still says the fallback is the MTU:

	/* If there are more bands than strict + quanta provided, the remaining
	 * ones are ETS with quantum of MTU. Initialize the missing values here.
	 */

On a headerless or tiny-MTU device, which is the case this hunk exists for,
the band now gets 256 rather than the MTU. Could the comment be updated to
say max(MTU, 256)?

> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 6144b5686f13f..ab8e7c6ae2039 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -980,7 +980,7 @@ static int fq_resize(struct Qdisc *sch, u32 log)
>  }
>  
>  static const struct netlink_range_validation iq_range = {
> -	.max = INT_MAX,
> +	.max = 1 << 20,
>  };

[Severity: High]

This narrows TCA_FQ_INITIAL_QUANTUM, which seeds the credit of a new flow.
The refill amount in the dequeue loop comes from q->quantum instead, and
fq_change() still accepts 1:

net/sched/sch_fq.c:fq_change() {
	...
	if (tb[TCA_FQ_QUANTUM]) {
		u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);

		if (quantum > 0 && quantum <= (1 << 20)) {
			WRITE_ONCE(q->quantum, quantum);
	...
}

net/sched/sch_fq.c:fq_dequeue() {
	...
	if (f->credit <= 0) {
		f->credit += q->quantum;
		head->first = f->next;
		fq_flow_add_tail(q, f, OLD_FLOW);
		goto begin;
	}
	...
}

The lead paragraph says "The change() paths with the same pattern, same
writer of q->quantum, same privilege level ... were not clamped", and
sch_fq is the qdisc of the primary Fixes: commit. With

  tc qdisc replace dev dummy0 root fq quantum 1 stab data 32767 \
    size_log 16 cell_log 0

does fq_dequeue() still need ~2^31 passes of f->credit += 1 after
f->credit -= plen? Should sch_fq get the same 256 floor as its siblings?

> diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
> index 96acab6a8da03..bb8e8952f5553 100644
> --- a/net/sched/sch_hhf.c
> +++ b/net/sched/sch_hhf.c
> @@ -551,7 +551,7 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt,
>  		return err;
>  
>  	if (tb[TCA_HHF_QUANTUM])
> -		new_quantum = nla_get_u32(tb[TCA_HHF_QUANTUM]);
> +		new_quantum = max(256U, nla_get_u32(tb[TCA_HHF_QUANTUM]));

[Severity: High]

The floor is applied only when the attribute is present, so a small quantum
coming from hhf_init() survives:

net/sched/sch_hhf.c:hhf_init() {
	...
	q->quantum = psched_mtu(qdisc_dev(sch));
	...
	if ((int)q->quantum <= 0 ||
	    (u64)q->quantum * q->hhf_non_hh_weight > INT_MAX)
		q->quantum = 256;
	...
}

Values in 1..255 pass untouched here, unlike sfq_init() and fq_pie_init()
which use clamp_t(u32, psched_mtu(...), 256, 1 << 20). With
ip link set dummy0 mtu 0 psched_mtu is 14, and on a veth with mtu 68 it is
82.

The changelog states "The init paths of fq_pie, sfq, and hhf delegate to
their _change() when opt is present, so the floor covers tc qdisc add ...
quantum 1 as well as change". Is that true for
tc qdisc add dev dummy0 root hhf with no options, where hhf_change() is
never called? And for tc qdisc change ... hhf non_hh_weight 1, where
new_quantum defaults to the unfloored q->quantum and is re-committed by
WRITE_ONCE(q->quantum, new_quantum)?

With q->quantum = 14 and a STAB-inflated qdisc_pkt_len, does hhf_dequeue()
still need ~7.7e7 passes of bucket->deficit += weight * q->quantum? Would
clamping in hhf_init() to [256, 1<<20] like the siblings be more robust?
The member comment also still reads

	u32		   quantum;        /* psched_mtu(qdisc_dev(sch)); */

which no longer describes either writer.

> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> index 187d3ed578f26..8bbcfc9e85d94 100644
> --- a/net/sched/sch_sfq.c
> +++ b/net/sched/sch_sfq.c
> @@ -660,6 +660,11 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
>  		return -EINVAL;
>  	}
>  
> +	if (ctl->quantum > 1 << 20) {
> +		NL_SET_ERR_MSG_MOD(extack, "quantum too large");
> +		return -EINVAL;
> +	}
> +

[Severity: Low]

With this reject in place, is the documented field contract still
reachable?

net/sched/sch_sfq.c:sfq_sched_data {
	...
	unsigned int	quantum;	/* Allotment per round: MUST BE >= MTU */
	...
}

sfq_init() already caps the default with
clamp_t(u32, psched_mtu(qdisc_dev(sch)), 256, 1 << 20), and after this
change userspace can no longer raise quantum to the MTU on a device whose
psched_mtu exceeds 1 MiB (dummy clears max_mtu and accepts an MTU of
2147483634, as noted in 709f34f7c28d). Could the comment be updated to
state the enforced 256..1<<20 range?

Also, tc_sfq_qopt configurations with quantum > 1 MiB that used to be
accepted now return -EINVAL, while the peer attributes
(TCA_FQ_PIE_QUANTUM, TCA_HHF_QUANTUM) clamp instead of rejecting. Was
rejecting rather than clamping intended for sfq here?

> @@ -688,7 +693,7 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
>  
>  	/* update and validate configuration */
>  	if (ctl->quantum)
> -		quantum = ctl->quantum;
> +		quantum = max(256U, ctl->quantum);

[Severity: Medium]

Does a constant floor bound the spin, or only divide it by a constant? The
numerator stays user-controlled:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
	...
	pkt_len <<= stab->szopts.size_log;
out:
	if (unlikely(pkt_len < 1))
		pkt_len = 1;
	...
}

STAB_SIZE_LOG_MAX is 30 and qdisc_get_stab() only checks size_log and
cell_log against it, so qdisc_pkt_len still reaches ~2^31. With quantum
exactly at the new floor:

net/sched/sch_sfq.c:sfq_dequeue() {
	...
	if (slot->allot <= 0) {
		q->tail = slot;
		WRITE_ONCE(slot->allot, slot->allot + q->quantum);
		goto next_slot;
	}
	...
}

that is still ~8.4e6 iterations per dequeued packet with the qdisc root
lock held and BH disabled, and fq_pie_dequeue(), hhf_dequeue(),
drr_dequeue() and ets_qdisc_dequeue() have the same shape. Would bounding
the STAB-derived pkt_len, or rounding the deficit up to cover the packet in
one step instead of looping, remove the class rather than attenuate it?

Re: [PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

[Jamal Hadi Salim]

On Thu, Aug 27, 2026 at 3:34 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.


Thanks for forwarding - I hadnt looked yet. Some of the findings are
valid and expose patterns. I'll send a v2 with:

- ETS: move the 256 floor into ets_quantum_parse() so explicit quanta
are covered, not just the fallback path. Add upper bound (1<<20).
- DRR: add upper bound clamp_t(u32, quantum, 256, 1<<20) matching sfq.
- sch_fq: add 256 floor in fq_change() for TCA_FQ_QUANTUM and in
fq_init() for tiny-MTU devices.
- hhf: clamp hhf_init() to [256, 1<<20] matching the siblings.
- Add Fixes: dcc68b4d8084 for ETS.

The Medium finding (STAB size_log=30 → 2^31) is addressed by the
merged qdisc_pkt_len cap (8f735d64382d) — together with the 256 floor
they bound the loop; neither alone is sufficient.

The rest are opinionated, so i will ignore them. There is nothing to
followup for net-next..

cheers,
jamal

> ---
> net/sched: clamp quantum and psched_mtu in change paths and missed siblings
>
> This follow-Thanks for forwarding the Sashiko review. The findings are valid and
expose same-pattern gaps. I'll send a v2 folding them in:

- ETS: move the 256 floor into ets_quantum_parse() so explicit quanta
  are covered, not just the fallback path. Add upper bound (1<<20).
- DRR: add upper bound clamp_t(u32, quantum, 256, 1<<20) matching sfq.
- sch_fq: add 256 floor in fq_change() for TCA_FQ_QUANTUM and in
  fq_init() for tiny-MTU devices.
- hhf: clamp hhf_init() to [256, 1<<20] matching the siblings.
- Add Fixes: dcc68b4d8084 for ETS.

The Medium finding (STAB size_log=30 → 2^31) is addressed by the
merged qdisc_pkt_len cap (8f735d64382d) — together with the 256 floor
they bound the loop; neither alone is sufficient.

cheers,
jamal
up to 709f34f7c28d adds a 256 byte floor to the quantum written
> by the change() paths of fq_pie, hhf, sfq and drr, floors the psched_mtu()
> fallback in drr and ets, narrows the sch_fq TCA_FQ_INITIAL_QUANTUM policy
> range to 1<<20, and clamps psched_mtu() to [1, 1<<20] in sch_dualpi2 and
> sch_pie. The stated goal is to stop a small quantum combined with a
> STAB-inflated qdisc_pkt_len from spinning the deficit-refill loops under the
> qdisc lock.
>
> A few questions on the changelog first.
>
> >     8. sch_ets: ets_qdisc_change() falls back to psched_mtu() with no floor
> >        for bands without an explicit quantum.  Add max(256U, ...) on the
> >        fallback path (vega@nebusec.ai independent bug)
>
> [Severity: Low]
>
> The ETS item is described as fixing a real denial-of-service condition, but
> none of the seven Fixes: tags names an ETS commit. git blame on the
> psched_mtu() fallback in ets_qdisc_change() at the baseline points at
> dcc68b4d8084 ("net: sch_ets: Add a new Qdisc"), which is not in the tag
> list. Should the following be added so stable tooling can map the ETS part?
>
>   Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
>
> >     1. fq_pie_change() accepts quantum=1 (NLA policy fq_pie_q_range.min=1).
> >        Add max(256U, ...) matching fq_codel_change()
> >        (Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7)
> [ ... ]
> >     5. sch_dualpi2: dualpi2_calculate_c_protection() and get_memory_limit()
> >        call psched_mtu() with no clamp. A huge MTU makes (s32)psched_mtu()
> >        overflow in the signed multiply for c_protection_init, and 2 *
> >        psched_mtu() wraps in get_memory_limit(). Clamp to [1, 1<<20] at
> >        all three call sites. (Sashiko nipa main-6-4)
>
> [Severity: Low]
>
> This patch bundles eight independent fixes over seven qdiscs with seven
> Fixes: tags and Cc: stable, covering different bug classes: deficit-spin
> quantum floors, the signed multiply overflow in dualpi2 c_protection_init,
> the bytemode divide in pie_drop_early(), and a netlink policy narrowing.
> The tags span very different stable ranges (320d031ad6e4 for dualpi2 is
> recent, 13d2a1d2b032 for drr is from 2008). Can this be split so each bug
> can be backported or reverted on its own?
>
> The changelog also carries what look like internal review-tool
> identifiers - "Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7",
> "Sashiko nipa main-6-4", "Sashiko gemini", "Internal review noticing same
> pattern". Are those meant to stay in the upstream commit message?
>
> > diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> > index 91b1ef824afaf..0ffdab27bae42 100644
> > --- a/net/sched/sch_drr.c
> > +++ b/net/sched/sch_drr.c
> > @@ -82,8 +82,9 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >                       NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
> >                       return -EINVAL;
> >               }
> > +             quantum = max(256U, quantum);
> >       } else
> > -             quantum = psched_mtu(qdisc_dev(sch));
> > +             quantum = max(256U, (u32)psched_mtu(qdisc_dev(sch)));
>
> [Severity: High]
>
> Only the low side is bounded here. drr_change_class() still accepts any
> non-zero u32, and cl->quantum and cl->deficit are both u32. What happens
> with a quantum near U32_MAX?
>
>   tc class add ... drr quantum 4294967293
>
> net/sched/sch_drr.c:drr_dequeue() {
>         ...
>         WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum));
>         list_move_tail(&cl->alist, &q->active);
>         ...
> }
>
> With quantum = 0xfffffffd that refill wraps modulo 2^32 and is the same as
> deficit -= 3, so with a STAB-inflated qdisc_pkt_len of 0x40000000 the loop
> needs roughly 3.6e8 passes before deficit wraps past zero, all with the
> qdisc root lock held. ets_qdisc_dequeue() uses the identical u32 arithmetic
> and ets_quantum_parse() rejects only zero.
>
> Should drr and ets also reject quantum > 1<<20, the way sfq_change() now
> does in this same patch?
>
> > diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > index 25fcf4079fece..f23c8dc68f8cb 100644
> > --- a/net/sched/sch_ets.c
> > +++ b/net/sched/sch_ets.c
> > @@ -636,7 +636,7 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
> >        */
> >       for (i = nstrict; i < nbands; i++) {
> >               if (!quanta[i])
> > -                     quanta[i] = psched_mtu(qdisc_dev(sch));
> > +                     quanta[i] = max(256U, (u32)psched_mtu(qdisc_dev(sch)));
>
> [Severity: High]
>
> This assignment is guarded by !quanta[i], so it only applies to bands that
> carry no explicit quantum. Every explicit value still goes through
> ets_quantum_parse(), which rejects zero only:
>
> net/sched/sch_ets.c:ets_quantum_parse() {
>         *quantum = nla_get_u32(attr);
>         if (!*quantum) {
>                 NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
>                 return -EINVAL;
>         }
>         return 0;
> }
>
> So does the reproducer in the changelog still work for ETS?
>
>   tc qdisc add dev dummy0 root ets bands 2 quanta 1 1
>
> ets_class_change() has the same gap, it writes the parsed value straight
> through with WRITE_ONCE(cl->quantum, quantum), and then
> ets_qdisc_dequeue() refills one byte per pass:
>
> net/sched/sch_ets.c:ets_qdisc_dequeue() {
>         ...
>         cl->deficit += READ_ONCE(cl->quantum);
>         list_move_tail(&cl->alist, &q->active);
>         ...
> }
>
> Would putting the floor in ets_quantum_parse() cover the explicit path as
> well? If so, test 41f5 in
> tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json expects
> "quanta 4294967294 1 1" to be accepted and dumped unchanged, so it would
> need updating.
>
> [Severity: Low]
>
> The comment just above this loop still says the fallback is the MTU:
>
>         /* If there are more bands than strict + quanta provided, the remaining
>          * ones are ETS with quantum of MTU. Initialize the missing values here.
>          */
>
> On a headerless or tiny-MTU device, which is the case this hunk exists for,
> the band now gets 256 rather than the MTU. Could the comment be updated to
> say max(MTU, 256)?
>
> > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> > index 6144b5686f13f..ab8e7c6ae2039 100644
> > --- a/net/sched/sch_fq.c
> > +++ b/net/sched/sch_fq.c
> > @@ -980,7 +980,7 @@ static int fq_resize(struct Qdisc *sch, u32 log)
> >  }
> >
> >  static const struct netlink_range_validation iq_range = {
> > -     .max = INT_MAX,
> > +     .max = 1 << 20,
> >  };
>
> [Severity: High]
>
> This narrows TCA_FQ_INITIAL_QUANTUM, which seeds the credit of a new flow.
> The refill amount in the dequeue loop comes from q->quantum instead, and
> fq_change() still accepts 1:
>
> net/sched/sch_fq.c:fq_change() {
>         ...
>         if (tb[TCA_FQ_QUANTUM]) {
>                 u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
>
>                 if (quantum > 0 && quantum <= (1 << 20)) {
>                         WRITE_ONCE(q->quantum, quantum);
>         ...
> }
>
> net/sched/sch_fq.c:fq_dequeue() {
>         ...
>         if (f->credit <= 0) {
>                 f->credit += q->quantum;
>                 head->first = f->next;
>                 fq_flow_add_tail(q, f, OLD_FLOW);
>                 goto begin;
>         }
>         ...
> }
>
> The lead paragraph says "The change() paths with the same pattern, same
> writer of q->quantum, same privilege level ... were not clamped", and
> sch_fq is the qdisc of the primary Fixes: commit. With
>
>   tc qdisc replace dev dummy0 root fq quantum 1 stab data 32767 \
>     size_log 16 cell_log 0
>
> does fq_dequeue() still need ~2^31 passes of f->credit += 1 after
> f->credit -= plen? Should sch_fq get the same 256 floor as its siblings?
>
> > diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
> > index 96acab6a8da03..bb8e8952f5553 100644
> > --- a/net/sched/sch_hhf.c
> > +++ b/net/sched/sch_hhf.c
> > @@ -551,7 +551,7 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt,
> >               return err;
> >
> >       if (tb[TCA_HHF_QUANTUM])
> > -             new_quantum = nla_get_u32(tb[TCA_HHF_QUANTUM]);
> > +             new_quantum = max(256U, nla_get_u32(tb[TCA_HHF_QUANTUM]));
>
> [Severity: High]
>
> The floor is applied only when the attribute is present, so a small quantum
> coming from hhf_init() survives:
>
> net/sched/sch_hhf.c:hhf_init() {
>         ...
>         q->quantum = psched_mtu(qdisc_dev(sch));
>         ...
>         if ((int)q->quantum <= 0 ||
>             (u64)q->quantum * q->hhf_non_hh_weight > INT_MAX)
>                 q->quantum = 256;
>         ...
> }
>
> Values in 1..255 pass untouched here, unlike sfq_init() and fq_pie_init()
> which use clamp_t(u32, psched_mtu(...), 256, 1 << 20). With
> ip link set dummy0 mtu 0 psched_mtu is 14, and on a veth with mtu 68 it is
> 82.
>
> The changelog states "The init paths of fq_pie, sfq, and hhf delegate to
> their _change() when opt is present, so the floor covers tc qdisc add ...
> quantum 1 as well as change". Is that true for
> tc qdisc add dev dummy0 root hhf with no options, where hhf_change() is
> never called? And for tc qdisc change ... hhf non_hh_weight 1, where
> new_quantum defaults to the unfloored q->quantum and is re-committed by
> WRITE_ONCE(q->quantum, new_quantum)?
>
> With q->quantum = 14 and a STAB-inflated qdisc_pkt_len, does hhf_dequeue()
> still need ~7.7e7 passes of bucket->deficit += weight * q->quantum? Would
> clamping in hhf_init() to [256, 1<<20] like the siblings be more robust?
> The member comment also still reads
>
>         u32                quantum;        /* psched_mtu(qdisc_dev(sch)); */
>
> which no longer describes either writer.
>
> > diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> > index 187d3ed578f26..8bbcfc9e85d94 100644
> > --- a/net/sched/sch_sfq.c
> > +++ b/net/sched/sch_sfq.c
> > @@ -660,6 +660,11 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
> >               return -EINVAL;
> >       }
> >
> > +     if (ctl->quantum > 1 << 20) {
> > +             NL_SET_ERR_MSG_MOD(extack, "quantum too large");
> > +             return -EINVAL;
> > +     }
> > +
>
> [Severity: Low]
>
> With this reject in place, is the documented field contract still
> reachable?
>
> net/sched/sch_sfq.c:sfq_sched_data {
>         ...
>         unsigned int    quantum;        /* Allotment per round: MUST BE >= MTU */
>         ...
> }
>
> sfq_init() already caps the default with
> clamp_t(u32, psched_mtu(qdisc_dev(sch)), 256, 1 << 20), and after this
> change userspace can no longer raise quantum to the MTU on a device whose
> psched_mtu exceeds 1 MiB (dummy clears max_mtu and accepts an MTU of
> 2147483634, as noted in 709f34f7c28d). Could the comment be updated to
> state the enforced 256..1<<20 range?
>
> Also, tc_sfq_qopt configurations with quantum > 1 MiB that used to be
> accepted now return -EINVAL, while the peer attributes
> (TCA_FQ_PIE_QUANTUM, TCA_HHF_QUANTUM) clamp instead of rejecting. Was
> rejecting rather than clamping intended for sfq here?
>
> > @@ -688,7 +693,7 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
> >
> >       /* update and validate configuration */
> >       if (ctl->quantum)
> > -             quantum = ctl->quantum;
> > +             quantum = max(256U, ctl->quantum);
>
> [Severity: Medium]
>
> Does a constant floor bound the spin, or only divide it by a constant? The
> numerator stays user-controlled:
>
> net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
>         ...
>         pkt_len <<= stab->szopts.size_log;
> out:
>         if (unlikely(pkt_len < 1))
>                 pkt_len = 1;
>         ...
> }
>
> STAB_SIZE_LOG_MAX is 30 and qdisc_get_stab() only checks size_log and
> cell_log against it, so qdisc_pkt_len still reaches ~2^31. With quantum
> exactly at the new floor:
>
> net/sched/sch_sfq.c:sfq_dequeue() {
>         ...
>         if (slot->allot <= 0) {
>                 q->tail = slot;
>                 WRITE_ONCE(slot->allot, slot->allot + q->quantum);
>                 goto next_slot;
>         }
>         ...
> }
>
> that is still ~8.4e6 iterations per dequeued packet with the qdisc root
> lock held and BH disabled, and fq_pie_dequeue(), hhf_dequeue(),
> drr_dequeue() and ets_qdisc_dequeue() have the same shape. Would bounding
> the STAB-derived pkt_len, or rounding the deficit up to cover the packet in
> one step instead of looping, remove the class rather than attenuate it?