[PATCH net v3 0/9] net/sched: clamp quantum/psched_mtu in change paths

[PATCH net v3 0/9] net/sched: clamp quantum/psched_mtu in change paths

[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 in fq, fq_codel, fq_pie, hhf, sfq. The
change() paths were not clamped but it is the same pattern, same writer
of q->quantum, same privilege level (CAP_NET_ADMIN in a user namespace).
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 series 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).

Each patch fixes one qdisc with its own Fixes: tag so they can be
backported independently - the commits they fix shift differently in
the git tree.

Patch 1: fq - clamp TCA_FQ_QUANTUM and TCA_FQ_INITIAL_QUANTUM in change
Patch 2: fq_pie - clamp quantum in change path
Patch 3: sfq - clamp quantum and reject > 1<<20 in change path
Patch 4: hhf - clamp quantum in change and init paths
Patch 5: dualpi2 - clamp psched_mtu at all 3 call sites
Patch 6: pie - clamp psched_mtu in pie_drop_early
Patch 7: drr - clamp quantum in change class
Patch 8: ets - clamp quantum in parse and fallback paths
Patch 9: selftests - update ETS test 41f5 for clamped quanta

Conditions to recreate (applies to all): create the qdisc, then
tc qdisc change ... quantum 1 with a STAB size table inflating
qdisc_pkt_len. Requires CAP_NET_ADMIN in a user namespace (unshare -Urn).

Sashiko links:
- v1: https://sashiko.dev/#/patchset/20260826074056.7873-1-jhs@mojatatu.com
- v2: https://sashiko.dev/#/patchset/20260829081229.81708-1-jhs@mojatatu.com

v2 -> v3:
- Add patch 9/9: update tdc case 41f5 (ETS offload quanta wrap test) -
  the [256, 1<<20] clamp in ets_quantum_parse() rejects/normalises the
  wrapping quanta 41f5 asserts verbatim, so the tc executor run failed
  (Jakub). 41f5 now matches the clamped values (quanta 1048576 256 256)
  and its name reflects the new behavior.
- No kernel-code changes; patches 1-8 are identical to v2.

v1 -> v2:
- Split the single monolithic patch into 8 per-qdisc patches, each with
  its own Fixes: tag, so stable backports can cherry-pick individually.
- Add cover letter.
- ETS: move 256 floor into ets_quantum_parse() so explicit quanta are
  clamped, not just the fallback path (Sashiko gemini + nipa gpt-5-6-sol-1-7).
- DRR: add upper bound clamp_t(u32, quantum, 256, 1<<20) matching sfq
  (Sashiko nipa gpt-5-6-sol-3-19).
- ETS: add upper bound clamp in ets_quantum_parse() and fallback path.
- sch_fq: add 256 floor in fq_change() for TCA_FQ_QUANTUM and clamp
  fq_init() quantum to [256, 1<<20] for tiny-MTU devices
  (Sashiko gemini + nipa main-1-1).
- hhf: clamp hhf_init() to [256, 1<<20] matching siblings
  (Sashiko gemini + nipa gpt-5-6-sol-6-24).
- Add Fixes: dcc68b4d8084 for ETS (Sashiko nipa gpt-5-6-sol-1-8).
- Re-add Toke's Reviewed-by from v1 (kept since v2 only splits the patch,
  the code Toke reviewed is unchanged in approach).

Jamal Hadi Salim (9):
  net/sched: fq: clamp quantum and initial_quantum in change path
  net/sched: fq_pie: clamp quantum in change path
  net/sched: sfq: clamp quantum in change path
  net/sched: hhf: clamp quantum in change and init paths
  net/sched: dualpi2: clamp psched_mtu at all call sites
  net/sched: pie: clamp psched_mtu in pie_drop_early
  net/sched: drr: clamp quantum in change class
  net/sched: ets: clamp quantum in parse and fallback paths
  selftests: tc-testing: update ETS test 41f5 for clamped quanta

[PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Jamal Hadi Salim]

The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
[1, 1<<20]. A user can override the init clamp via tc qdisc change,
restoring the small-quantum deficit spin that the init clamp prevents.

Narrow iq_range.max to 1<<20 so TCA_FQ_INITIAL_QUANTUM is rejected at
parse time. Clamp TCA_FQ_QUANTUM to [256, 1<<20] in fq_change() and
fq_init() quantum to [256, 1<<20] for tiny-MTU devices.

Conditions to recreate the bug:
  CONFIG_NET_SCH_FQ=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

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

Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_fq.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 6144b5686f13..35f940b2205d 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] = {
@@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
 			   nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
 
 	if (tb[TCA_FQ_QUANTUM]) {
-		u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
+		u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
+				     256, 1 << 20);
 
-		if (quantum > 0 && quantum <= (1 << 20)) {
-			WRITE_ONCE(q->quantum, quantum);
-		} else {
-			NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
-			err = -EINVAL;
-		}
+		WRITE_ONCE(q->quantum, quantum);
 	}
 
 	if (tb[TCA_FQ_INITIAL_QUANTUM])
@@ -1232,7 +1228,7 @@ static int fq_init(struct Qdisc *sch, struct nlattr *opt,
 	sch->limit		= 10000;
 	q->flow_plimit		= 100;
 	mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
-	q->quantum		= min_t(u32, 2 * mtu, 1 << 20);
+	q->quantum		= clamp_t(u32, 2 * mtu, 256, 1 << 20);
 	q->initial_quantum	= min_t(u32, 10 * mtu, 1 << 20);
 	q->flow_refill_delay	= msecs_to_jiffies(40);
 	q->flow_max_rate	= ~0UL;
-- 
2.43.0

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Eric Dumazet]

On Tue, Sep 1, 2026 at 11:39 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> restoring the small-quantum deficit spin that the init clamp prevents.
>
> Narrow iq_range.max to 1<<20 so TCA_FQ_INITIAL_QUANTUM is rejected at
> parse time. Clamp TCA_FQ_QUANTUM to [256, 1<<20] in fq_change() and
> fq_init() quantum to [256, 1<<20] for tiny-MTU devices.
>
> Conditions to recreate the bug:
>   CONFIG_NET_SCH_FQ=y. Requires CAP_NET_ADMIN (namespace-local via
>   unshare -Urn suffices).
>
>   tc qdisc add dev dummy0 root fq
>   tc qdisc change dev dummy0 root fq quantum 1 stab data 32768 size_log 15 cell_log 0
>
> Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
> Reported-by: Vega <vega@nebusec.ai>
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  net/sched/sch_fq.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 6144b5686f13..35f940b2205d 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] = {
> @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
>                            nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
>
>         if (tb[TCA_FQ_QUANTUM]) {
> -               u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> +               u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> +                                    256, 1 << 20);
>
> -               if (quantum > 0 && quantum <= (1 << 20)) {
> -                       WRITE_ONCE(q->quantum, quantum);
> -               } else {
> -                       NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> -                       err = -EINVAL;
> -               }
> +               WRITE_ONCE(q->quantum, quantum);
>         }
>
>         if (tb[TCA_FQ_INITIAL_QUANTUM])
> @@ -1232,7 +1228,7 @@ static int fq_init(struct Qdisc *sch, struct nlattr *opt,
>         sch->limit              = 10000;
>         q->flow_plimit          = 100;
>         mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
> -       q->quantum              = min_t(u32, 2 * mtu, 1 << 20);
> +       q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
>         q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);
>         q->flow_refill_delay    = msecs_to_jiffies(40);
>         q->flow_max_rate        = ~0UL;
> --
> 2.43.0
>

If we consider dev->mtu admissible values, which are in [0, INT_MAX],
we have to guard against u32 overflows
for 2*mtu and 10*mtu expressions.

Commit 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum
and initial quantum") missed this?

Please use for q->quantum:  clamp_t(u64, 2ULL * mtu, 1ULL << 20)
and for q->initial_quantum: clamp_t(u64, 10ULL * mtu, 1ULL << 20)

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Eric Dumazet]

On Wed, Sep 2, 2026 at 1:52 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 1, 2026 at 11:39 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> > TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> > [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> > restoring the small-quantum deficit spin that the init clamp prevents.
> >
> > Narrow iq_range.max to 1<<20 so TCA_FQ_INITIAL_QUANTUM is rejected at
> > parse time. Clamp TCA_FQ_QUANTUM to [256, 1<<20] in fq_change() and
> > fq_init() quantum to [256, 1<<20] for tiny-MTU devices.
> >
> > Conditions to recreate the bug:
> >   CONFIG_NET_SCH_FQ=y. Requires CAP_NET_ADMIN (namespace-local via
> >   unshare -Urn suffices).
> >
> >   tc qdisc add dev dummy0 root fq
> >   tc qdisc change dev dummy0 root fq quantum 1 stab data 32768 size_log 15 cell_log 0
> >
> > Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
> > Reported-by: Vega <vega@nebusec.ai>
> > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > Tested-by: Victor Nogueira <victor@mojatatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > ---
> >  net/sched/sch_fq.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> > index 6144b5686f13..35f940b2205d 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] = {
> > @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
> >                            nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
> >
> >         if (tb[TCA_FQ_QUANTUM]) {
> > -               u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> > +               u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> > +                                    256, 1 << 20);
> >
> > -               if (quantum > 0 && quantum <= (1 << 20)) {
> > -                       WRITE_ONCE(q->quantum, quantum);
> > -               } else {
> > -                       NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> > -                       err = -EINVAL;
> > -               }
> > +               WRITE_ONCE(q->quantum, quantum);
> >         }
> >
> >         if (tb[TCA_FQ_INITIAL_QUANTUM])
> > @@ -1232,7 +1228,7 @@ static int fq_init(struct Qdisc *sch, struct nlattr *opt,
> >         sch->limit              = 10000;
> >         q->flow_plimit          = 100;
> >         mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
> > -       q->quantum              = min_t(u32, 2 * mtu, 1 << 20);
> > +       q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
> >         q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);
> >         q->flow_refill_delay    = msecs_to_jiffies(40);
> >         q->flow_max_rate        = ~0UL;
> > --
> > 2.43.0
> >
>
> If we consider dev->mtu admissible values, which are in [0, INT_MAX],
> we have to guard against u32 overflows
> for 2*mtu and 10*mtu expressions.
>
> Commit 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum
> and initial quantum") missed this?
>
> Please use for q->quantum:  clamp_t(u64, 2ULL * mtu, 1ULL << 20)
> and for q->initial_quantum: clamp_t(u64, 10ULL * mtu, 1ULL << 20)

Wait, I missed the
mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);

Not clear why we have so many clamp_t(), this is quite confusing.

We could instead:

mtu = psched_mtu(qdisc_dev(sch));
q->quantum = clamp_t(u64, 2ULL * mtu, 256, 1ULL << 20);
q->initial_quantum = min_t(u64, 10ULL * mtu, 1ULL << 20);

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Jamal Hadi Salim]

On Tue, Sep 1, 2026 at 8:08 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Sep 2, 2026 at 1:52 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Sep 1, 2026 at 11:39 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> > > TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> > > [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> > > restoring the small-quantum deficit spin that the init clamp prevents.
> > >
> > > Narrow iq_range.max to 1<<20 so TCA_FQ_INITIAL_QUANTUM is rejected at
> > > parse time. Clamp TCA_FQ_QUANTUM to [256, 1<<20] in fq_change() and
> > > fq_init() quantum to [256, 1<<20] for tiny-MTU devices.
> > >
> > > Conditions to recreate the bug:
> > >   CONFIG_NET_SCH_FQ=y. Requires CAP_NET_ADMIN (namespace-local via
> > >   unshare -Urn suffices).
> > >
> > >   tc qdisc add dev dummy0 root fq
> > >   tc qdisc change dev dummy0 root fq quantum 1 stab data 32768 size_log 15 cell_log 0
> > >
> > > Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
> > > Reported-by: Vega <vega@nebusec.ai>
> > > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > > Tested-by: Victor Nogueira <victor@mojatatu.com>
> > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > ---
> > >  net/sched/sch_fq.c | 14 +++++---------
> > >  1 file changed, 5 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> > > index 6144b5686f13..35f940b2205d 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] = {
> > > @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
> > >                            nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
> > >
> > >         if (tb[TCA_FQ_QUANTUM]) {
> > > -               u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> > > +               u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> > > +                                    256, 1 << 20);
> > >
> > > -               if (quantum > 0 && quantum <= (1 << 20)) {
> > > -                       WRITE_ONCE(q->quantum, quantum);
> > > -               } else {
> > > -                       NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> > > -                       err = -EINVAL;
> > > -               }
> > > +               WRITE_ONCE(q->quantum, quantum);
> > >         }
> > >
> > >         if (tb[TCA_FQ_INITIAL_QUANTUM])
> > > @@ -1232,7 +1228,7 @@ static int fq_init(struct Qdisc *sch, struct nlattr *opt,
> > >         sch->limit              = 10000;
> > >         q->flow_plimit          = 100;
> > >         mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
> > > -       q->quantum              = min_t(u32, 2 * mtu, 1 << 20);
> > > +       q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
> > >         q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);
> > >         q->flow_refill_delay    = msecs_to_jiffies(40);
> > >         q->flow_max_rate        = ~0UL;
> > > --
> > > 2.43.0
> > >
> >
> > If we consider dev->mtu admissible values, which are in [0, INT_MAX],
> > we have to guard against u32 overflows
> > for 2*mtu and 10*mtu expressions.
> >
> > Commit 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum
> > and initial quantum") missed this?
> >
> > Please use for q->quantum:  clamp_t(u64, 2ULL * mtu, 1ULL << 20)
> > and for q->initial_quantum: clamp_t(u64, 10ULL * mtu, 1ULL << 20)
>
> Wait, I missed the
> mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
>
> Not clear why we have so many clamp_t(), this is quite confusing.
>
> We could instead:
>
> mtu = psched_mtu(qdisc_dev(sch));
> q->quantum = clamp_t(u64, 2ULL * mtu, 256, 1ULL << 20);
> q->initial_quantum = min_t(u64, 10ULL * mtu, 1ULL << 20);

Trying to understand your concern: Are you arguing for
stylistic/readability improvement or robustness?
The only place i can see where the multiplies you describe could
happen is what the 709f34f7c28d patch closed. i.e this part:

-       q->quantum              = 2 * psched_mtu(qdisc_dev(sch));
-       q->initial_quantum      = 10 * psched_mtu(qdisc_dev(sch));

IIRC, you are suggesting restoring the original logic (x2 and x10) and
i am likely missing why that is a must do? Does it break anything?

cheers,
jamal

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Eric Dumazet]

On Wed, Sep 2, 2026 at 2:40 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
>
> Trying to understand your concern: Are you arguing for
> stylistic/readability improvement or robustness?
> The only place i can see where the multiplies you describe could
> happen is what the 709f34f7c28d patch closed. i.e this part:
>
> -       q->quantum              = 2 * psched_mtu(qdisc_dev(sch));
> -       q->initial_quantum      = 10 * psched_mtu(qdisc_dev(sch));
>
> IIRC, you are suggesting restoring the original logic (x2 and x10) and
> i am likely missing why that is a must do? Does it break anything?

It does not break anything; it just causes a cascade of various clamps.

After your patch we will have:

       mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
       q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
       q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);

So @mtu is no longer the psched mtu anymore.

This is fine.

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Jamal Hadi Salim]

On Wed, Sep 2, 2026 at 9:04 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Sep 2, 2026 at 2:40 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> >
> > Trying to understand your concern: Are you arguing for
> > stylistic/readability improvement or robustness?
> > The only place i can see where the multiplies you describe could
> > happen is what the 709f34f7c28d patch closed. i.e this part:
> >
> > -       q->quantum              = 2 * psched_mtu(qdisc_dev(sch));
> > -       q->initial_quantum      = 10 * psched_mtu(qdisc_dev(sch));
> >
> > IIRC, you are suggesting restoring the original logic (x2 and x10) and
> > i am likely missing why that is a must do? Does it break anything?
>
> It does not break anything; it just causes a cascade of various clamps.
>
> After your patch we will have:
>
>        mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
>        q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
>        q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);
>
> So @mtu is no longer the psched mtu anymore.
>
> This is fine.

The u64 variant would read cleaner for sure.  If i didnt misread you
though you are saying the current the mtu-no-longer-psched_mtu
aliasing is fine. So would it be ok to see if any other comments
spring up from says sashiko that are worth reviewing the patch before
I proceed with this? Or do you want a v4 or a followup?

cheers,
jamal

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Eric Dumazet]

On Wed, Sep 2, 2026 at 3:55 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Sep 2, 2026 at 9:04 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Sep 2, 2026 at 2:40 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > >
> > > Trying to understand your concern: Are you arguing for
> > > stylistic/readability improvement or robustness?
> > > The only place i can see where the multiplies you describe could
> > > happen is what the 709f34f7c28d patch closed. i.e this part:
> > >
> > > -       q->quantum              = 2 * psched_mtu(qdisc_dev(sch));
> > > -       q->initial_quantum      = 10 * psched_mtu(qdisc_dev(sch));
> > >
> > > IIRC, you are suggesting restoring the original logic (x2 and x10) and
> > > i am likely missing why that is a must do? Does it break anything?
> >
> > It does not break anything; it just causes a cascade of various clamps.
> >
> > After your patch we will have:
> >
> >        mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
> >        q->quantum              = clamp_t(u32, 2 * mtu, 256, 1 << 20);
> >        q->initial_quantum      = min_t(u32, 10 * mtu, 1 << 20);
> >
> > So @mtu is no longer the psched mtu anymore.
> >
> > This is fine.
>
> The u64 variant would read cleaner for sure.  If i didnt misread you
> though you are saying the current the mtu-no-longer-psched_mtu
> aliasing is fine. So would it be ok to see if any other comments
> spring up from says sashiko that are worth reviewing the patch before
> I proceed with this? Or do you want a v4 or a followup?

I think v3 is fine, thanks.

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 4 · Low: 1

- [Medium] Commit message misdescribes the pre-patch behaviour and the
  severity of the bug being fixed.
- [Medium] fq_change() no longer rejects an out-of-range TCA_FQ_QUANTUM.
- [Medium] The `Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds
  to quantum and initial quantum")` tag does not identify the commit…
- [Medium] Narrowing `iq_range.max` from INT_MAX to 1<<20 makes any
  configuration that sets initial_quantum above 1 MiB fail with -ERANGE…
- [Low] The tc-testing case 10f7 "Create FQ with invalid initial_quantum
  setting" uses `initial_quantum 0x80000000` (2147483648), which was…

--

net/sched: fq: clamp quantum and initial_quantum in change path

This narrows the netlink policy range for TCA_FQ_INITIAL_QUANTUM to 1<<20
and replaces the range check on TCA_FQ_QUANTUM in fq_change() with an
unconditional clamp to [256, 1<<20].  fq_init() now also clamps its
default quantum to the same range.

A few questions on the changelog and on the behaviour changes.

> The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> restoring the small-quantum deficit spin that the init clamp prevents.

[Severity: Medium]
Is this description of the pre-patch behaviour accurate?  The code this
patch removes from fq_change() is:

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);
		} else {
			NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
			err = -EINVAL;
		}
	}

so the accepted range on the change path was [1, 1<<20], not
[1, INT_MAX].

Also, 709f34f7c28d only touched fq_init(), where the clamp prevents
quantum == 0 arising from 2 * psched_mtu() wrapping.  quantum == 0 was
already refused by fq_change() with -EINVAL, so what exactly could be
overridden through tc qdisc change?

On the "spin" itself, __qdisc_calculate_pkt_len() in net/sched/sch_api.c
already bounds the stab-inflated length:

	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);

With QDISC_PKT_LEN_MAX == (1 << 20), the fq_dequeue() refill loop

	if (f->credit <= 0) {
		f->credit += q->quantum;
		...
		goto begin;
	}

terminates after at most ~2^20 passes for any quantum >= 1.

Would it be more accurate to state that the residual gap closed on the
change path is quantum in [1, 255]?

> Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")

[Severity: Medium]
Does this Fixes tag point at the right commit?  709f34f7c28d changes only
fq_init().  Blame at the parent of this commit attributes the fq_change()
quantum handling to 3725a269815ba6 (2015) with the 1<<20 upper bound from
d9e15a2733067c (2020), and iq_range.max = INT_MAX to 7041101ff6c307
(2023).

The reproducer in the changelog

	tc qdisc change dev dummy0 root fq quantum 1 stab ...

goes only through fq_change(), so it would behave the same on kernels
predating 709f34f7c28d.  Since stable selection keys off the Fixes tag,
should the tag name one of the older commits instead?

> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 6144b5686f13f..35f940b2205df 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: Medium]
Is this an intentional uAPI restriction?  With
NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range), validate_nla() now returns
-ERANGE during nla_parse_nested_deprecated(), so

	tc qdisc add dev dummy0 root fq initial_quantum 2000000

fails outright and fq_change() never runs.  Values above 1 MiB have been
accepted since 7041101ff6c307, and unvalidated before that.

What bug does a large initial_quantum cause?  In sch_fq.c the only uses of
q->initial_quantum are the two assignments in fq_classify()

	f->credit = q->initial_quantum;

plus the dump, while the refill loop in fq_dequeue() adds q->quantum.  So
initial_quantum looks like a per-new-flow burst knob that cannot drive the
refill loop.

Should the changelog mention the compatibility break for existing tc
scripts?

>  
>  static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
> @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
>  			   nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
>  
>  	if (tb[TCA_FQ_QUANTUM]) {
> -		u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> +		u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> +				     256, 1 << 20);
>  
> -		if (quantum > 0 && quantum <= (1 << 20)) {
> -			WRITE_ONCE(q->quantum, quantum);
> -		} else {
> -			NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> -			err = -EINVAL;
> -		}
> +		WRITE_ONCE(q->quantum, quantum);
>  	}

[Severity: Medium]
Is the loss of the -EINVAL here intended?  After this change,

	tc qdisc change dev dummy0 root fq quantum 0

returns success and installs 256, where it previously returned -EINVAL
with the "invalid quantum" extack (that rejection dates back to
3725a269815ba6, with the upper bound from d9e15a2733067c).  Values in
[1, 255] that were previously honoured are now silently rewritten to 256.

There is a second, less obvious effect: this branch can no longer set err,
so the later blocks in fq_change()

	if (!err && tb[TCA_FQ_PRIOMAP])
		err = fq_load_priomap(...);

	if (!err && tb[TCA_FQ_WEIGHTS])
		err = fq_load_weights(...);

	if (!err) {
		sch_tree_unlock(sch);
		err = fq_resize(sch, fq_log);
		sch_tree_lock(sch);
	}

now all run for a request that carries an invalid quantum together with
priomap, weights or a new buckets_log.  Previously such a request was
refused as a whole.  Was that change in behaviour considered?

The same file still rejects out-of-range band quanta in fq_load_weights()
with

	NL_SET_ERR_MSG_MOD(extack, "invalid weights");

and fq_codel_change() keeps the split convention:

net/sched/sch_fq_codel.c:fq_codel_change() {
		quantum = max(256U, nla_get_u32(tb[TCA_FQ_CODEL_QUANTUM]));
		if (quantum > FQ_CODEL_QUANTUM_MAX) {
			NL_SET_ERR_MSG(extack, "Invalid quantum");
			return -EINVAL;
		}
}

Within this patch, TCA_FQ_INITIAL_QUANTUM is hard-rejected at parse time
while its neighbour TCA_FQ_QUANTUM is silently clamped.  Could the two
adjacent attributes signal out-of-range input the same way, i.e. keep the
-EINVAL for quantum above 1<<20 and only clamp the low end?

>  
>  	if (tb[TCA_FQ_INITIAL_QUANTUM])

[ ... ]

[Severity: Low]
This isn't a bug, but is there test coverage for the new bounds?  The
tdc case 10f7 "Create FQ with invalid initial_quantum setting" in
tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json uses

	initial_quantum 0x80000000

which is 2147483648 and was already refused by the old INT_MAX bound, so
it passes identically before and after this patch and would keep passing
if the 1<<20 bound were reverted.  Case 845b uses initial_quantum 900000,
below the new bound, and the only quantum case 2390 uses quantum 9000,
inside the new clamp.

Nothing in fq.json exercises 0x100000 accepted versus 0x100001 rejected,
and nothing exercises quantum 0, quantum 1 (the reproducer named in the
changelog) or quantum above 1<<20.  The series does update tc-testing for
the ETS clamp but adds nothing for fq.  Could fq.json be extended along
with this change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>

Man, this is going to be more time consuming than I anticipated. I may
not going to be able to respond to all today.
I am going to summarize my responses here.
Summary: I dont see need to make any updates, but I will if something
else serious shows up on the other patches.


Here are my conclusions on your findings:

1. [Medium] "pre-patch behaviour misdescribed". This is a valid
complaint. The changelog was misleading.
It's a freaking changelog, a human wouldnt label it as "medium"
priority - but you do. I will fix if i have to resend.

2. [Medium] "fq_change() no longer rejects out-of-range quantum"
IMO, Valid as a behaviour description (0 now installs 256 instead of -EINVAL).
Intentional: the clamp is the fix; the d9e15a273306 reject was
protecting against the same small-quantum hang this floor now prevents
by construction. The err-flow note (later blocks now run for an
otherwise-invalid request) is real but harmless: no error path was
reachable for out-of-range quantum alone, and the attributes are
independent.

3. [Medium] "Fixes: 709f34f7c28d wrong"

Partially valid: The iq_range INT_MAX that this patch narrows came
from 7041101ff6c3; the change-path floor itself is new hardening. If I
have to resend will point Fixes: to 7041101ff6c3.

4. [Medium] "initial_quantum -ERANGE is a uAPI break"
initial_quantum > 1 MiB has no functional use (burst-credit only), and
the flood-amplification risk is what the bound is for.
The changelog could name the compatibility break; noted for a future resend.

5. [Low] "tdc coverage for the new bounds"
Valid.  Will add fq.json cases (quantum 0x100000 vs 0x100001, quantum
0/1) if the series is ever resent.

Overall conclusion: no code change requested for this patch as
accepted on-list; documentation/attribution notes only.
So only if i have to resend..

cheers,
jamal

> Critical: 0 · High: 0 · Medium: 4 · Low: 1
>
> - [Medium] Commit message misdescribes the pre-patch behaviour and the
>   severity of the bug being fixed.
> - [Medium] fq_change() no longer rejects an out-of-range TCA_FQ_QUANTUM.
> - [Medium] The `Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds
>   to quantum and initial quantum")` tag does not identify the commit…
> - [Medium] Narrowing `iq_range.max` from INT_MAX to 1<<20 makes any
>   configuration that sets initial_quantum above 1 MiB fail with -ERANGE…
> - [Low] The tc-testing case 10f7 "Create FQ with invalid initial_quantum
>   setting" uses `initial_quantum 0x80000000` (2147483648), which was…
>
> --
>
> net/sched: fq: clamp quantum and initial_quantum in change path
>
> This narrows the netlink policy range for TCA_FQ_INITIAL_QUANTUM to 1<<20
> and replaces the range check on TCA_FQ_QUANTUM in fq_change() with an
> unconditional clamp to [256, 1<<20].  fq_init() now also clamps its
> default quantum to the same range.
>
> A few questions on the changelog and on the behaviour changes.
>
> > The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> > TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> > [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> > restoring the small-quantum deficit spin that the init clamp prevents.
>
> [Severity: Medium]
> Is this description of the pre-patch behaviour accurate?  The code this
> patch removes from fq_change() is:
>
> 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);
>                 } else {
>                         NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
>                         err = -EINVAL;
>                 }
>         }
>
> so the accepted range on the change path was [1, 1<<20], not
> [1, INT_MAX].
>
> Also, 709f34f7c28d only touched fq_init(), where the clamp prevents
> quantum == 0 arising from 2 * psched_mtu() wrapping.  quantum == 0 was
> already refused by fq_change() with -EINVAL, so what exactly could be
> overridden through tc qdisc change?
>
> On the "spin" itself, __qdisc_calculate_pkt_len() in net/sched/sch_api.c
> already bounds the stab-inflated length:
>
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>
> With QDISC_PKT_LEN_MAX == (1 << 20), the fq_dequeue() refill loop
>
>         if (f->credit <= 0) {
>                 f->credit += q->quantum;
>                 ...
>                 goto begin;
>         }
>
> terminates after at most ~2^20 passes for any quantum >= 1.
>
> Would it be more accurate to state that the residual gap closed on the
> change path is quantum in [1, 255]?
>
> > Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
>
> [Severity: Medium]
> Does this Fixes tag point at the right commit?  709f34f7c28d changes only
> fq_init().  Blame at the parent of this commit attributes the fq_change()
> quantum handling to 3725a269815ba6 (2015) with the 1<<20 upper bound from
> d9e15a2733067c (2020), and iq_range.max = INT_MAX to 7041101ff6c307
> (2023).
>
> The reproducer in the changelog
>
>         tc qdisc change dev dummy0 root fq quantum 1 stab ...
>
> goes only through fq_change(), so it would behave the same on kernels
> predating 709f34f7c28d.  Since stable selection keys off the Fixes tag,
> should the tag name one of the older commits instead?
>
> > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> > index 6144b5686f13f..35f940b2205df 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: Medium]
> Is this an intentional uAPI restriction?  With
> NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range), validate_nla() now returns
> -ERANGE during nla_parse_nested_deprecated(), so
>
>         tc qdisc add dev dummy0 root fq initial_quantum 2000000
>
> fails outright and fq_change() never runs.  Values above 1 MiB have been
> accepted since 7041101ff6c307, and unvalidated before that.
>
> What bug does a large initial_quantum cause?  In sch_fq.c the only uses of
> q->initial_quantum are the two assignments in fq_classify()
>
>         f->credit = q->initial_quantum;
>
> plus the dump, while the refill loop in fq_dequeue() adds q->quantum.  So
> initial_quantum looks like a per-new-flow burst knob that cannot drive the
> refill loop.
>
> Should the changelog mention the compatibility break for existing tc
> scripts?
>
> >
> >  static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
> > @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
> >                          nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
> >
> >       if (tb[TCA_FQ_QUANTUM]) {
> > -             u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> > +             u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> > +                                  256, 1 << 20);
> >
> > -             if (quantum > 0 && quantum <= (1 << 20)) {
> > -                     WRITE_ONCE(q->quantum, quantum);
> > -             } else {
> > -                     NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> > -                     err = -EINVAL;
> > -             }
> > +             WRITE_ONCE(q->quantum, quantum);
> >       }
>
> [Severity: Medium]
> Is the loss of the -EINVAL here intended?  After this change,
>
>         tc qdisc change dev dummy0 root fq quantum 0
>
> returns success and installs 256, where it previously returned -EINVAL
> with the "invalid quantum" extack (that rejection dates back to
> 3725a269815ba6, with the upper bound from d9e15a2733067c).  Values in
> [1, 255] that were previously honoured are now silently rewritten to 256.
>
> There is a second, less obvious effect: this branch can no longer set err,
> so the later blocks in fq_change()
>
>         if (!err && tb[TCA_FQ_PRIOMAP])
>                 err = fq_load_priomap(...);
>
>         if (!err && tb[TCA_FQ_WEIGHTS])
>                 err = fq_load_weights(...);
>
>         if (!err) {
>                 sch_tree_unlock(sch);
>                 err = fq_resize(sch, fq_log);
>                 sch_tree_lock(sch);
>         }
>
> now all run for a request that carries an invalid quantum together with
> priomap, weights or a new buckets_log.  Previously such a request was
> refused as a whole.  Was that change in behaviour considered?
>
> The same file still rejects out-of-range band quanta in fq_load_weights()
> with
>
>         NL_SET_ERR_MSG_MOD(extack, "invalid weights");
>
> and fq_codel_change() keeps the split convention:
>
> net/sched/sch_fq_codel.c:fq_codel_change() {
>                 quantum = max(256U, nla_get_u32(tb[TCA_FQ_CODEL_QUANTUM]));
>                 if (quantum > FQ_CODEL_QUANTUM_MAX) {
>                         NL_SET_ERR_MSG(extack, "Invalid quantum");
>                         return -EINVAL;
>                 }
> }
>
> Within this patch, TCA_FQ_INITIAL_QUANTUM is hard-rejected at parse time
> while its neighbour TCA_FQ_QUANTUM is silently clamped.  Could the two
> adjacent attributes signal out-of-range input the same way, i.e. keep the
> -EINVAL for quantum above 1<<20 and only clamp the low end?
>
> >
> >       if (tb[TCA_FQ_INITIAL_QUANTUM])
>
> [ ... ]
>
> [Severity: Low]
> This isn't a bug, but is there test coverage for the new bounds?  The
> tdc case 10f7 "Create FQ with invalid initial_quantum setting" in
> tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json uses
>
>         initial_quantum 0x80000000
>
> which is 2147483648 and was already refused by the old INT_MAX bound, so
> it passes identically before and after this patch and would keep passing
> if the 1<<20 bound were reverted.  Case 845b uses initial_quantum 900000,
> below the new bound, and the only quantum case 2390 uses quantum 9000,
> inside the new clamp.
>
> Nothing in fq.json exercises 0x100000 accepted versus 0x100001 rejected,
> and nothing exercises quantum 0, quantum 1 (the reproducer named in the
> changelog) or quantum above 1<<20.  The series does update tc-testing for
> the ETS clamp but adds nothing for fq.  Could fq.json be extended along
> with this change?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 2/9] net/sched: fq_pie: clamp quantum in change path

[Jamal Hadi Salim]

fq_pie_change() accepts any quantum value from userspace, including 1.
With a crafted size table qdisc_pkt_len reaches ~2 GiB, so quantum=1
makes the deficit-refill loop spin ~2^31 times under the qdisc lock
(a soft lockup / denial of service).

Add max(256U, ...) matching fq_codel_change().

Conditions to recreate the bug:
  CONFIG_NET_SCH_FQ_PIE=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

  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

Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_fq_pie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

[PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path

[Jamal Hadi Salim]

sfq_change() accepts any non-negative quantum (only rejects
(int)ctl->quantum < 0). With a crafted size table qdisc_pkt_len reaches
~2 GiB, so quantum=1 makes the deficit-refill loop spin ~2^31 times
under the qdisc lock (a soft lockup / denial of service).

Add max(256U, ...) matching fq_codel_change(). Reject quantum > 1<<20
with -EINVAL, matching fq_codel_change() and the init clamp.

Conditions to recreate the bug:
  CONFIG_NET_SCH_SFQ=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

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

Fixes: e4650d7ae425 ("net_sched: sch_sfq: handle bigger packets")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_sfq.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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 v3 3/9] net/sched: sfq: clamp quantum in change path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 0 (1)

- [High] Incomplete fix: raising the quantum floor to 256 in sfq_change()
  does not bound the deficit-refill loop in sfq_dequeue()…
- [Medium] The reproducer in the commit message cannot reach
  sfq_change().
- [Medium] The commit message's justification contradicts the tree the
  patch is applied to.

Pre-existing issues:
- [Low] Code/comment mismatch: struct sfq_sched_data documents `unsigned
  int quantum; /* Allotment per round: MUST BE >= MTU */`…

--

net/sched: sfq: clamp quantum in change path

This adds two new checks to sfq_change(): quantum values above 1<<20 are
rejected with -EINVAL, and accepted quantum values are raised to a floor
of 256 bytes.  The stated goal is to stop the deficit-refill loop in
sfq_dequeue() from spinning for a very long time under the qdisc lock.

A few questions on the changelog and on how much of the loop is actually
bounded afterwards.

>     sfq_change() accepts any non-negative quantum (only rejects
>     (int)ctl->quantum < 0). With a crafted size table qdisc_pkt_len reaches
>     ~2 GiB, so quantum=1 makes the deficit-refill loop spin ~2^31 times
>     under the qdisc lock (a soft lockup / denial of service).

[Severity: Medium]
Is the "~2 GiB" figure still accurate for this tree?  The tail of
__qdisc_calculate_pkt_len() in net/sched/sch_api.c already ends with:

	/* A size table can inflate qdisc_pkt_len() beyond any real packet
	 * (via overhead, the data table, or size_log); cap it so deficit
	 * schedulers such as DRR/ETS terminate their refill loops.
	 */
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);

with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
git blame puts that clamp at commit 8f735d64382dcf, which is already in
the baseline this patch applies to, so qdisc_pkt_len() appears to top out
at 1 MiB rather than ~2 GiB, making the "~2^31 times" number roughly
2048x too large.

Given the Fixes: tag will drive stable triage, would it be worth saying
that the generic pkt_len clamp already bounds the loop in current trees,
and that the 2^31 figure only applies to kernels predating that clamp?

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

[Severity: Medium]
Can this second command actually reach sfq_change()?  sfq_qdisc_ops sets:

	.change		=	NULL,

and qdisc_change() in net/sched/sch_api.c rejects the request before
either the options or the size table are applied:

	if (tca[TCA_OPTIONS]) {
		if (!sch->ops->change) {
			NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc");
			return -EINVAL;
		}

For "tc qdisc change dev dummy0 root sfq quantum 1 ..." iproute2 always
emits TCA_OPTIONS, and __tc_modify_qdisc() falls through to
qdisc_change() for an existing root qdisc of matching kind, so the
command should return -EINVAL.

The only live caller of sfq_change() looks to be sfq_init(), i.e. tc
qdisc add / replace.  Should the subject ("clamp quantum in change
path"), the narrative and the reproducer be reworded around
add/replace instead?

> 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;
> +	}
> +
>  	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);

[Severity: High]
Does a 256-byte quantum floor actually bound the refill loop, or only
divide its trip count?  The loop in sfq_dequeue() credits one quantum to
one slot per iteration and stops at the first visited slot with a
positive allot:

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

q->tail is not advanced past the slot that was just served; the served
slot is only debited:

	} else {
		WRITE_ONCE(slot->allot, slot->allot - qdisc_pkt_len(skb));
	}

So with a size table pushing qdisc_pkt_len() to QDISC_PKT_LEN_MAX, each
slot served in one round-robin sweep is left at about -1 MiB, and a slot
needs (1 << 20) / 256 = 4096 visits to become positive again.  New flows
start positive in sfq_enqueue():

		/* We could use a bigger initial quantum for new flows */
		WRITE_ONCE(slot->allot, q->quantum);

so N flows can each be driven deeply negative by one dequeue before the
ring wraps.  With flows 65408 (SFQ_MAX_FLOWS) that is on the order of
4096 * 65408 iterations inside a single sfq_dequeue() call, with the root
qdisc spinlock held and BH disabled.

The same arithmetic seems to hold without any small quantum at all: a
default quantum derived from a 1500-byte MTU still gives roughly
(1 << 20) / 1500 * 65408 iterations under the same setup.

Would bounding the deficit itself work better here, for example clamping
or resetting slot->allot when it goes below -q->quantum, or computing the
number of quanta arithmetically rather than looping one quantum at a
time?

[Severity: Low]
This is a pre-existing mismatch and not introduced by this patch, but
while this validation is being touched: struct sfq_sched_data documents

	unsigned int	quantum;	/* Allotment per round: MUST BE >= MTU */

and sfq_init() derives its default from the device MTU:

	q->quantum = clamp_t(u32, psched_mtu(qdisc_dev(sch)),
			     256, 1 << 20);

whereas sfq_change() now stores max(256U, ctl->quantum) without looking
at psched_mtu(), so on a 1500-MTU device a requested quantum of 300 is
stored verbatim.  Should the clamp use psched_mtu() as its lower bound,
or should the "MUST BE >= MTU" comment be updated?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 2 · Low: 0 (1)
>
> - [High] Incomplete fix: raising the quantum floor to 256 in sfq_change()
>   does not bound the deficit-refill loop in sfq_dequeue()…
> - [Medium] The reproducer in the commit message cannot reach
>   sfq_change().
> - [Medium] The commit message's justification contradicts the tree the
>   patch is applied to.
>
> Pre-existing issues:
> - [Low] Code/comment mismatch: struct sfq_sched_data documents `unsigned
>   int quantum; /* Allotment per round: MUST BE >= MTU */`…
>
>


Ok, so 4 issues:
1. [High] "256 floor does not bound the deficit-refill walk"
This is valid but NOT introduced by this patch.
This is a pre-existing residual of the same family (the guard belongs
at the refill site in sfq_dequeue(), exactly like the fq_codel case);
Again, it is NOT introduced by this patch and does not invalidate the clamp.
Filed as a separate follow-up patch; will reference whatever this
commit is when it gets merged on that followup.

2. [Medium] "reproducer cannot reach sfq_change()"
Valid. sfq_change() is only wired to sfq_init()/add-replace, not
change. Changelog wording issue only. Only if i have to repost.

3. [Medium] "justification contradicts the tree"
QDISC_PKT_LEN_MAX caps pkt_len at 1 MiB on the tree. So i wont do
anything with this.

4. [Low] "comment says MUST BE >= MTU"
noted; the 256 floor supersedes the comment's assumption; comment
refresh belongs with any future sfq patch.

Summary:  i dont see anything here either that requires a resend.

cheers,
jamal
>
> net/sched: sfq: clamp quantum in change path
>
> This adds two new checks to sfq_change(): quantum values above 1<<20 are
> rejected with -EINVAL, and accepted quantum values are raised to a floor
> of 256 bytes.  The stated goal is to stop the deficit-refill loop in
> sfq_dequeue() from spinning for a very long time under the qdisc lock.
>
> A few questions on the changelog and on how much of the loop is actually
> bounded afterwards.
>
> >     sfq_change() accepts any non-negative quantum (only rejects
> >     (int)ctl->quantum < 0). With a crafted size table qdisc_pkt_len reaches
> >     ~2 GiB, so quantum=1 makes the deficit-refill loop spin ~2^31 times
> >     under the qdisc lock (a soft lockup / denial of service).
>
> [Severity: Medium]
> Is the "~2 GiB" figure still accurate for this tree?  The tail of
> __qdisc_calculate_pkt_len() in net/sched/sch_api.c already ends with:
>
>         /* A size table can inflate qdisc_pkt_len() beyond any real packet
>          * (via overhead, the data table, or size_log); cap it so deficit
>          * schedulers such as DRR/ETS terminate their refill loops.
>          */
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>
> with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
> git blame puts that clamp at commit 8f735d64382dcf, which is already in
> the baseline this patch applies to, so qdisc_pkt_len() appears to top out
> at 1 MiB rather than ~2 GiB, making the "~2^31 times" number roughly
> 2048x too large.
>
> Given the Fixes: tag will drive stable triage, would it be worth saying
> that the generic pkt_len clamp already bounds the loop in current trees,
> and that the 2^31 figure only applies to kernels predating that clamp?
>
> >       tc qdisc add dev dummy0 root sfq
> >       tc qdisc change dev dummy0 root sfq quantum 1 stab data 32768 size_log 15 cell_log 0
>
> [Severity: Medium]
> Can this second command actually reach sfq_change()?  sfq_qdisc_ops sets:
>
>         .change         =       NULL,
>
> and qdisc_change() in net/sched/sch_api.c rejects the request before
> either the options or the size table are applied:
>
>         if (tca[TCA_OPTIONS]) {
>                 if (!sch->ops->change) {
>                         NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc");
>                         return -EINVAL;
>                 }
>
> For "tc qdisc change dev dummy0 root sfq quantum 1 ..." iproute2 always
> emits TCA_OPTIONS, and __tc_modify_qdisc() falls through to
> qdisc_change() for an existing root qdisc of matching kind, so the
> command should return -EINVAL.
>
> The only live caller of sfq_change() looks to be sfq_init(), i.e. tc
> qdisc add / replace.  Should the subject ("clamp quantum in change
> path"), the narrative and the reproducer be reworded around
> add/replace instead?
>
> > 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;
> > +     }
> > +
> >       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);
>
> [Severity: High]
> Does a 256-byte quantum floor actually bound the refill loop, or only
> divide its trip count?  The loop in sfq_dequeue() credits one quantum to
> one slot per iteration and stops at the first visited slot with a
> positive allot:
>
> net/sched/sch_sfq.c:sfq_dequeue() {
>         ...
> next_slot:
>         a = q->tail->next;
>         slot = &q->slots[a];
>         if (slot->allot <= 0) {
>                 q->tail = slot;
>                 WRITE_ONCE(slot->allot, slot->allot + q->quantum);
>                 goto next_slot;
>         }
>         ...
> }
>
> q->tail is not advanced past the slot that was just served; the served
> slot is only debited:
>
>         } else {
>                 WRITE_ONCE(slot->allot, slot->allot - qdisc_pkt_len(skb));
>         }
>
> So with a size table pushing qdisc_pkt_len() to QDISC_PKT_LEN_MAX, each
> slot served in one round-robin sweep is left at about -1 MiB, and a slot
> needs (1 << 20) / 256 = 4096 visits to become positive again.  New flows
> start positive in sfq_enqueue():
>
>                 /* We could use a bigger initial quantum for new flows */
>                 WRITE_ONCE(slot->allot, q->quantum);
>
> so N flows can each be driven deeply negative by one dequeue before the
> ring wraps.  With flows 65408 (SFQ_MAX_FLOWS) that is on the order of
> 4096 * 65408 iterations inside a single sfq_dequeue() call, with the root
> qdisc spinlock held and BH disabled.
>
> The same arithmetic seems to hold without any small quantum at all: a
> default quantum derived from a 1500-byte MTU still gives roughly
> (1 << 20) / 1500 * 65408 iterations under the same setup.
>
> Would bounding the deficit itself work better here, for example clamping
> or resetting slot->allot when it goes below -q->quantum, or computing the
> number of quanta arithmetically rather than looping one quantum at a
> time?
>
> [Severity: Low]
> This is a pre-existing mismatch and not introduced by this patch, but
> while this validation is being touched: struct sfq_sched_data documents
>
>         unsigned int    quantum;        /* Allotment per round: MUST BE >= MTU */
>
> and sfq_init() derives its default from the device MTU:
>
>         q->quantum = clamp_t(u32, psched_mtu(qdisc_dev(sch)),
>                              256, 1 << 20);
>
> whereas sfq_change() now stores max(256U, ctl->quantum) without looking
> at psched_mtu(), so on a 1500-MTU device a requested quantum of 300 is
> stored verbatim.  Should the clamp use psched_mtu() as its lower bound,
> or should the "MUST BE >= MTU" comment be updated?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 4/9] net/sched: hhf: clamp quantum in change and init paths

[Jamal Hadi Salim]

hhf_change() accepts any quantum from userspace, including 1. With a
crafted size table qdisc_pkt_len reaches ~2 GiB, so quantum=1 makes
the deficit-refill loop spin ~2^31 times under the qdisc lock
(a soft lockup / denial of service).

Add max(256U, ...) in hhf_change() matching fq_codel_change(). Clamp
hhf_init() to [256, 1<<20] matching the siblings, and remove the old
fallback that only set quantum=256 on overflow.

Conditions to recreate the bug:
  CONFIG_NET_SCH_HHF=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

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

Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_hhf.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index 96acab6a8da0..fc72f825fbd9 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]);
@@ -613,7 +613,7 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
 	int i;
 
 	sch->limit = 1000;
-	q->quantum = psched_mtu(qdisc_dev(sch));
+	q->quantum = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 256, 1 << 20);
 	get_random_bytes(&q->perturbation, sizeof(q->perturbation));
 	INIT_LIST_HEAD(&q->new_buckets);
 	INIT_LIST_HEAD(&q->old_buckets);
@@ -624,10 +624,6 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
 	q->hhf_evict_timeout = HZ;      /* 1  sec */
 	q->hhf_non_hh_weight = 2;
 
-	if ((int)q->quantum <= 0 ||
-	    (u64)q->quantum * q->hhf_non_hh_weight > INT_MAX)
-		q->quantum = 256;
-
 	if (opt) {
 		int err = hhf_change(sch, opt, extack);
 
-- 
2.43.0

Re: [PATCH net v3 4/9] net/sched: hhf: clamp quantum in change and init paths

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] The changelog's justification is stale on the tree this patch
  applies to.

Pre-existing issues:
- [Medium] A TCA_HHF_HH_FLOWS_LIMIT value supplied at qdisc creation time
  is silently discarded.

--

net/sched: hhf: clamp quantum in change and init paths

This adds a 256 byte floor to the quantum accepted from userspace in
hhf_change(), clamps the quantum derived from the device MTU in hhf_init()
to [256, 1<<20], and drops the old overflow fallback that set quantum to
256.

Two questions below, one about the changelog and one about a pre-existing
detail in hhf_init().

>     hhf_change() accepts any quantum from userspace, including 1. With a
>     crafted size table qdisc_pkt_len reaches ~2 GiB, so quantum=1 makes
>     the deficit-refill loop spin ~2^31 times under the qdisc lock
>     (a soft lockup / denial of service).
>     ...
>       tc qdisc add dev dummy0 root hhf
>       tc qdisc change dev dummy0 root hhf quantum 1 stab data 32768 size_log 15 cell_log 0
>
>     Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")

[Severity: Medium]
Is the ~2 GiB / ~2^31 figure still accurate on this tree?

__qdisc_calculate_pkt_len() in net/sched/sch_api.c already ends with:

	/* A size table can inflate qdisc_pkt_len() beyond any real packet
	 * (via overhead, the data table, or size_log); cap it so deficit
	 * schedulers such as DRR/ETS terminate their refill loops.
	 */
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);

with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
That cap is present in the baseline, added by 8f735d64382d ("net/sched:
bound qdisc_pkt_len to prevent qdisc soft lockup").

With that clamp, the quoted reproducer (32768 << 15, i.e. 2^30) yields a
pkt_len of 2^20, so the refill loop in hhf_dequeue():

		if (bucket->deficit <= 0) {
			int weight = (bucket - q->buckets == WDRR_BUCKET_FOR_HH) ?
				      1 : q->hhf_non_hh_weight;

			bucket->deficit += weight * q->quantum;

is bounded to roughly 2^20 iterations with quantum=1, not 2^31. The
changelog of 8f735d64382d itself describes 1M iterations as completing in
a few milliseconds.

Could the changelog be reworded so it describes the bound that actually
applies here, and mention 8f735d64382d as the commit that closes the
soft-lockup case? Without that dependency noted, a stable backport of this
patch alone onto a tree lacking the pkt_len cap still leaves roughly
2 GiB / 256 refill iterations under the qdisc lock.

> diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
> index 96acab6a8da03..fc72f825fbd92 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]);

[ ... ]

> @@ -624,10 +624,6 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
>  	q->hhf_evict_timeout = HZ;      /* 1  sec */
>  	q->hhf_non_hh_weight = 2;
>  
> -	if ((int)q->quantum <= 0 ||
> -	    (u64)q->quantum * q->hhf_non_hh_weight > INT_MAX)
> -		q->quantum = 256;
> -
>  	if (opt) {
>  		int err = hhf_change(sch, opt, extack);
>  

[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
while looking at the ordering in hhf_init() around this hhf_change() call:
is a TCA_HHF_HH_FLOWS_LIMIT value passed at creation time dropped?

hhf_change() stores it:

	if (tb[TCA_HHF_HH_FLOWS_LIMIT])
		WRITE_ONCE(q->hh_flows_limit,
			   nla_get_u32(tb[TCA_HHF_HH_FLOWS_LIMIT]));

but on a fresh qdisc q->hh_flows is NULL, so hhf_init() then enters the
allocation branch which overwrites the same field:

		/* Cap max active HHs at twice len of hh_flows table. */
		q->hh_flows_limit = 2 * HH_FLOWS_CNT;

So tc qdisc add dev X root hhf hh_limit N appears to be ignored, while the
same option via tc qdisc change on an already initialised qdisc takes
effect. That field gates admission in hhf_get_flow():

	if (q->hh_flows_current_cnt >= q->hh_flows_limit) {

Would it make sense to only apply the default when the attribute was not
supplied?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 4/9] net/sched: hhf: clamp quantum in change and init paths

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
>
> - [Medium] The changelog's justification is stale on the tree this patch
>   applies to.
>
> Pre-existing issues:
> - [Medium] A TCA_HHF_HH_FLOWS_LIMIT value supplied at qdisc creation time
>   is silently discarded.
>
> --
>

1. [Medium] "~2 GiB / 2^31 figure stale"

Valid. Good catch on the backport angle.
Post-8f735d64382d the stab-inflated pkt_len is capped at 1 MiB, so the
hhf_dequeue() refill loop is bounded to ~2^20/quantum iterations,
_not_ 2^31. The changelog should describe the post-cap bound and note
8f735d64382d as the commit that closes the soft-lockup case. Stable
patches are going to include  8f735d64382d so we are good.
Documentation on the clamp itself is still correct defense-in-depth
for the mtu=0 case it also fixes.

2. [Medium, pre-existing] "TCA_HHF_HH_FLOWS_LIMIT at creation is
silently discarded"
Already tracked by our internal tracker as  bug QDISC-B855. It covers
the hhf flows-limit family; this specific create-path discard is noted
there as part of that fix's scope.

Summary: No code change requested for this patch.


cheers,
jamal
> net/sched: hhf: clamp quantum in change and init paths
>
> This adds a 256 byte floor to the quantum accepted from userspace in
> hhf_change(), clamps the quantum derived from the device MTU in hhf_init()
> to [256, 1<<20], and drops the old overflow fallback that set quantum to
> 256.
>
> Two questions below, one about the changelog and one about a pre-existing
> detail in hhf_init().
>
> >     hhf_change() accepts any quantum from userspace, including 1. With a
> >     crafted size table qdisc_pkt_len reaches ~2 GiB, so quantum=1 makes
> >     the deficit-refill loop spin ~2^31 times under the qdisc lock
> >     (a soft lockup / denial of service).
> >     ...
> >       tc qdisc add dev dummy0 root hhf
> >       tc qdisc change dev dummy0 root hhf quantum 1 stab data 32768 size_log 15 cell_log 0
> >
> >     Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
>
> [Severity: Medium]
> Is the ~2 GiB / ~2^31 figure still accurate on this tree?
>
> __qdisc_calculate_pkt_len() in net/sched/sch_api.c already ends with:
>
>         /* A size table can inflate qdisc_pkt_len() beyond any real packet
>          * (via overhead, the data table, or size_log); cap it so deficit
>          * schedulers such as DRR/ETS terminate their refill loops.
>          */
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>
> with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
> That cap is present in the baseline, added by 8f735d64382d ("net/sched:
> bound qdisc_pkt_len to prevent qdisc soft lockup").
>
> With that clamp, the quoted reproducer (32768 << 15, i.e. 2^30) yields a
> pkt_len of 2^20, so the refill loop in hhf_dequeue():
>
>                 if (bucket->deficit <= 0) {
>                         int weight = (bucket - q->buckets == WDRR_BUCKET_FOR_HH) ?
>                                       1 : q->hhf_non_hh_weight;
>
>                         bucket->deficit += weight * q->quantum;
>
> is bounded to roughly 2^20 iterations with quantum=1, not 2^31. The
> changelog of 8f735d64382d itself describes 1M iterations as completing in
> a few milliseconds.
>
> Could the changelog be reworded so it describes the bound that actually
> applies here, and mention 8f735d64382d as the commit that closes the
> soft-lockup case? Without that dependency noted, a stable backport of this
> patch alone onto a tree lacking the pkt_len cap still leaves roughly
> 2 GiB / 256 refill iterations under the qdisc lock.
>
> > diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
> > index 96acab6a8da03..fc72f825fbd92 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]);
>
> [ ... ]
>
> > @@ -624,10 +624,6 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
> >       q->hhf_evict_timeout = HZ;      /* 1  sec */
> >       q->hhf_non_hh_weight = 2;
> >
> > -     if ((int)q->quantum <= 0 ||
> > -         (u64)q->quantum * q->hhf_non_hh_weight > INT_MAX)
> > -             q->quantum = 256;
> > -
> >       if (opt) {
> >               int err = hhf_change(sch, opt, extack);
> >
>
> [Severity: Medium]
> This is a pre-existing issue and not something this patch introduces, but
> while looking at the ordering in hhf_init() around this hhf_change() call:
> is a TCA_HHF_HH_FLOWS_LIMIT value passed at creation time dropped?
>
> hhf_change() stores it:
>
>         if (tb[TCA_HHF_HH_FLOWS_LIMIT])
>                 WRITE_ONCE(q->hh_flows_limit,
>                            nla_get_u32(tb[TCA_HHF_HH_FLOWS_LIMIT]));
>
> but on a fresh qdisc q->hh_flows is NULL, so hhf_init() then enters the
> allocation branch which overwrites the same field:
>
>                 /* Cap max active HHs at twice len of hh_flows table. */
>                 q->hh_flows_limit = 2 * HH_FLOWS_CNT;
>
> So tc qdisc add dev X root hhf hh_limit N appears to be ignored, while the
> same option via tc qdisc change on an already initialised qdisc takes
> effect. That field gates admission in hhf_get_flow():
>
>         if (q->hh_flows_current_cnt >= q->hh_flows_limit) {
>
> Would it make sense to only apply the default when the attribute was not
> supplied?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 5/9] net/sched: dualpi2: clamp psched_mtu at all call sites

[Jamal Hadi Salim]

dualpi2_calculate_c_protection(), must_drop(), 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(). With a crafted size table
qdisc_pkt_len reaches ~2 GiB, causing a soft lockup / denial of service.

Clamp psched_mtu() to [1, 1<<20] at all three call sites.

Conditions to recreate the bug:
  CONFIG_NET_SCH_DUALPI2=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

  tc qdisc add dev dummy0 root dualpi2
  tc qdisc change dev dummy0 root dualpi2 stab data 32768 size_log 15 cell_log 0

Fixes: 320d031ad6e4 ("sched: Struct definition and parsing of dualpi2 qdisc")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_dualpi2.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

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

Re: [PATCH net v3 5/9] net/sched: dualpi2: clamp psched_mtu at all call sites

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Changelog/reproducer mismatch in "net/sched: dualpi2: clamp
  psched_mtu at all call sites": the commit message blames "a crafted…
- [Low] netdev coding-style violation and triplicated open-coded clamp.

--

net/sched: dualpi2: clamp psched_mtu at all call sites

The patch clamps psched_mtu(qdisc_dev(sch)) into [1, 1 << 20] at the three
places sch_dualpi2.c uses it: dualpi2_calculate_c_protection(), must_drop()
and get_memory_limit().  The stated goal is to avoid overflow of the signed
multiply for c_protection_init and the wrap of 2 * psched_mtu().

A couple of questions about the changelog and the style of the new code.

[Severity: Medium]

> With a crafted size table qdisc_pkt_len reaches ~2 GiB, causing a soft
> lockup / denial of service.
>
>   tc qdisc add dev dummy0 root dualpi2
>   tc qdisc change dev dummy0 root dualpi2 stab data 32768 size_log 15 cell_log 0

Do these two commands actually exercise any of the three changed lines?

The patch only bounds psched_mtu(), which is derived from the netdevice:

include/net/pkt_sched.h:
	static inline unsigned int psched_mtu(const struct net_device *dev)
	{
		return READ_ONCE(dev->mtu) + dev->hard_header_len;
	}

A TCA_STAB only rewrites qdisc_skb_cb(skb)->pkt_len in
__qdisc_calculate_pkt_len(), so a size table cannot influence
psched_mtu().  With the quoted commands dummy0 keeps its default MTU, so
dualpi2_calculate_c_protection(), must_drop() and get_memory_limit()
compute the same values before and after this patch.

Is the "~2 GiB" qdisc_pkt_len reachable on this tree at all?
__qdisc_calculate_pkt_len() already caps it, and that clamp is present in
the baseline too:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
	...
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	qdisc_skb_cb(skb)->pkt_len = pkt_len;
}

with include/net/pkt_sched.h:

	#define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */

Would it be more accurate to describe the trigger as a large device MTU?
dummy leaves the upper bound unset:

drivers/net/dummy.c:dummy_setup() {
	...
	dev->min_mtu = 0;
	dev->max_mtu = 0;
}

and dev_validate_mtu() only enforces an upper limit when dev->max_mtu > 0,
so "ip link set dev dummy0 mtu 2147483647" is what makes
(s32)psched_mtu() * ((int)wc - (int)wl) overflow.  Note that
TCA_DUALPI2_C_PROTECTION is range-limited to [0, MAX_WC] with MAX_WC 100,
so the signed multiply only overflows above roughly 21.5 MiB of MTU, and
2 * psched_mtu() only wraps above 2 GiB.

One more thing on scope: the size-table path the message mentions is left
untouched in this file.  dequeue_packet() still does

	*credit_change *= qdisc_pkt_len(skb);

and drop_and_retry() still accumulates

	q->deferred_drops_len += qdisc_pkt_len(skb);

Those are fine here because of the generic 1 MiB pkt_len cap, but the
Fixes: tag points at trees that may not carry that cap.  Should the
changelog say so?

> diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
> index 4f678d4ff10ec..4947def7c49ef 100644
> --- a/net/sched/sch_dualpi2.c
> +++ b/net/sched/sch_dualpi2.c

[ ... ]

> @@ -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);

[Severity: Low]
This isn't a bug, but the new declaration in must_drop() is the longest
line of the block and is placed last, which breaks the netdev declaration
ordering rule in Documentation/process/maintainer-netdev.rst ("Order the
variable declaration lines longest to shortest").  Moving the
initialization out of line, or reordering the block, would match the rule.

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

Would a small helper, e.g. dualpi2_psched_mtu(sch), be preferable here?
The same clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20) expression is
now open-coded three times, including this line-wrapped copy inside
mul_u32_u32(), so the "at all call sites" property has to be re-checked by
hand for any future psched_mtu() user or bound change.

>  
>  	if (upper_32_bits(memlim))
>  		return U32_MAX;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 5/9] net/sched: dualpi2: clamp psched_mtu at all call sites

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> - [Medium] Changelog/reproducer mismatch in "net/sched: dualpi2: clamp
>   psched_mtu at all call sites": the commit message blames "a crafted…
> - [Low] netdev coding-style violation and triplicated open-coded clamp.
>

My conclusion:

1. [Medium] "changelog/reproducer mismatch — stab does not reach
   psched_mtu()"
Valid point.
The real trigger is a large device MTU  (dummy accepts up to ~2^31-1
since dev->max_mtu = 0), not a size table; the changelog's stab-based
recipe exercises none of the three
 The correct reproducer is: "ip link set dev dummy0 mtu 2147483647"
Changelog issue only; the clamp is the right fix for the MTU path.

2. [Low] "declaration ordering + triplicated clamp"
Also valid:  Will fix the ordering (and consider a small helper) if
the series is ever resent.

On the deferred scope note (dequeue_packet() credit multiply,
deferred_drops_len accumulation): We verified separately the generic 1
MiB pkt_len cap bounds the multiply, but the s32 credit ACCUMULATION
can still wrap when the C queue is empty; filed as a follow-up
("under dualpi2 c_protection_credit s32 accumulation wrap
(C-queue-empty case)") .

cheers,
jamal

> --
>
> net/sched: dualpi2: clamp psched_mtu at all call sites
>
> The patch clamps psched_mtu(qdisc_dev(sch)) into [1, 1 << 20] at the three
> places sch_dualpi2.c uses it: dualpi2_calculate_c_protection(), must_drop()
> and get_memory_limit().  The stated goal is to avoid overflow of the signed
> multiply for c_protection_init and the wrap of 2 * psched_mtu().
>
> A couple of questions about the changelog and the style of the new code.
>
> [Severity: Medium]
>
> > With a crafted size table qdisc_pkt_len reaches ~2 GiB, causing a soft
> > lockup / denial of service.
> >
> >   tc qdisc add dev dummy0 root dualpi2
> >   tc qdisc change dev dummy0 root dualpi2 stab data 32768 size_log 15 cell_log 0
>
> Do these two commands actually exercise any of the three changed lines?
>
> The patch only bounds psched_mtu(), which is derived from the netdevice:
>
> include/net/pkt_sched.h:
>         static inline unsigned int psched_mtu(const struct net_device *dev)
>         {
>                 return READ_ONCE(dev->mtu) + dev->hard_header_len;
>         }
>
> A TCA_STAB only rewrites qdisc_skb_cb(skb)->pkt_len in
> __qdisc_calculate_pkt_len(), so a size table cannot influence
> psched_mtu().  With the quoted commands dummy0 keeps its default MTU, so
> dualpi2_calculate_c_protection(), must_drop() and get_memory_limit()
> compute the same values before and after this patch.
>
> Is the "~2 GiB" qdisc_pkt_len reachable on this tree at all?
> __qdisc_calculate_pkt_len() already caps it, and that clamp is present in
> the baseline too:
>
> net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
>         ...
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>         qdisc_skb_cb(skb)->pkt_len = pkt_len;
> }
>
> with include/net/pkt_sched.h:
>
>         #define QDISC_PKT_LEN_MAX       (1 << 20)       /* 1 MiB */
>
> Would it be more accurate to describe the trigger as a large device MTU?
> dummy leaves the upper bound unset:
>
> drivers/net/dummy.c:dummy_setup() {
>         ...
>         dev->min_mtu = 0;
>         dev->max_mtu = 0;
> }
>
> and dev_validate_mtu() only enforces an upper limit when dev->max_mtu > 0,
> so "ip link set dev dummy0 mtu 2147483647" is what makes
> (s32)psched_mtu() * ((int)wc - (int)wl) overflow.  Note that
> TCA_DUALPI2_C_PROTECTION is range-limited to [0, MAX_WC] with MAX_WC 100,
> so the signed multiply only overflows above roughly 21.5 MiB of MTU, and
> 2 * psched_mtu() only wraps above 2 GiB.
>
> One more thing on scope: the size-table path the message mentions is left
> untouched in this file.  dequeue_packet() still does
>
>         *credit_change *= qdisc_pkt_len(skb);
>
> and drop_and_retry() still accumulates
>
>         q->deferred_drops_len += qdisc_pkt_len(skb);
>
> Those are fine here because of the generic 1 MiB pkt_len cap, but the
> Fixes: tag points at trees that may not carry that cap.  Should the
> changelog say so?
>
> > diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
> > index 4f678d4ff10ec..4947def7c49ef 100644
> > --- a/net/sched/sch_dualpi2.c
> > +++ b/net/sched/sch_dualpi2.c
>
> [ ... ]
>
> > @@ -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);
>
> [Severity: Low]
> This isn't a bug, but the new declaration in must_drop() is the longest
> line of the block and is placed last, which breaks the netdev declaration
> ordering rule in Documentation/process/maintainer-netdev.rst ("Order the
> variable declaration lines longest to shortest").  Moving the
> initialization out of line, or reordering the block, would match the rule.
>
> >
> > -     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));
>
> Would a small helper, e.g. dualpi2_psched_mtu(sch), be preferable here?
> The same clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20) expression is
> now open-coded three times, including this line-wrapped copy inside
> mul_u32_u32(), so the "at all call sites" property has to be re-checked by
> hand for any future psched_mtu() user or bound change.
>
> >
> >       if (upper_32_bits(memlim))
> >               return U32_MAX;
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 6/9] net/sched: pie: clamp psched_mtu in pie_drop_early

[Jamal Hadi Salim]

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

Conditions to recreate the bug:
  CONFIG_NET_SCH_PIE=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

  tc qdisc add dev dummy0 root pie
  tc qdisc change dev dummy0 root pie stab data 32768 size_log 15 cell_log 0

Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_pie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH net v3 6/9] net/sched: pie: clamp psched_mtu in pie_drop_early

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The commit message's "Conditions to recreate the bug" section
  does not reproduce the described bug.

--

net/sched: pie: clamp psched_mtu in pie_drop_early

The patch clamps the value returned by psched_mtu() in pie_drop_early() to
the range [1, 1<<20], with the stated aim of preventing a very large device
MTU from zeroing the bytemode drop probability. The change itself is a
one-line clamp_t() in net/sched/sch_pie.c.

>     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].
>
>     Conditions to recreate the bug:
>       CONFIG_NET_SCH_PIE=y. Requires CAP_NET_ADMIN (namespace-local via
>       unshare -Urn suffices).
>
>       tc qdisc add dev dummy0 root pie
>       tc qdisc change dev dummy0 root pie stab data 32768 size_log 15 cell_log 0

[Severity: Medium]
Do these two commands actually reproduce the described condition?

The clamped quantity comes only from the netdevice:

include/net/pkt_sched.h:psched_mtu() {
	return READ_ONCE(dev->mtu) + dev->hard_header_len;
}

Neither "tc qdisc add ... root pie" nor "tc qdisc change ... stab data 32768
size_log 15 cell_log 0" changes dev->mtu or dev->hard_header_len, so dummy0
keeps its ether_setup() defaults of 1500 + 14 = 1514.  That is already inside
[1, 1<<20], so clamp_t() returns the same value and the reproducer sees no
difference before or after the patch.  Shouldn't the steps include something
like "ip link set dev dummy0 mtu 2147483634" (which dummy allows, since
dummy_setup() leaves dev->min_mtu = 0 and dev->max_mtu = 0)?

Also, does the size table have any bearing here?  A TCA_STAB table only
affects qdisc_skb_cb(skb)->pkt_len via __qdisc_calculate_pkt_len(), which is
already capped:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
	...
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	...
}

and pie_qdisc_enqueue() does not use it as packet_size:

net/sched/sch_pie.c:pie_qdisc_enqueue() {
	...
	if (!pie_drop_early(sch, &q->params, &q->vars, sch->qstats.backlog,
			    skb->len)) {
	...
}

Finally, the divide the message blames is gated on bytemode:

net/sched/sch_pie.c:pie_drop_early() {
	...
	if (params->bytemode && packet_size <= mtu)
		local_prob = (u64)packet_size * div_u64(local_prob, mtu);
	...
}

and bytemode is off by default:

include/net/pie.h:pie_params_init() {
	...
	params->bytemode = false;
	...
}

Since the listed commands never pass bytemode, is that branch ever reached in
the reproducer?  Could the "Conditions to recreate the bug" section be updated
to raise the MTU and enable bytemode, so that the Tested-by tag corresponds to
a sequence that exercises the clamped value and the divide?

>  {
>  	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);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 6/9] net/sched: pie: clamp psched_mtu in pie_drop_early

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> - [Medium] The commit message's "Conditions to recreate the bug" section
>   does not reproduce the described bug.
>
Valid
The clamp fix correctly hardens the mtu=0x80000000 case; the
changelog's reproducer and premise need the `ip link set mtu ...` step
instead.
Documentation-only fix.

Note: same wrong-reproducer issue as patch 5 (dualpi2); both
changelogs blame a stab path that psched_mtu() never reads.


cheers,
jamal
>
> net/sched: pie: clamp psched_mtu in pie_drop_early
>
> The patch clamps the value returned by psched_mtu() in pie_drop_early() to
> the range [1, 1<<20], with the stated aim of preventing a very large device
> MTU from zeroing the bytemode drop probability. The change itself is a
> one-line clamp_t() in net/sched/sch_pie.c.
>
> >     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].
> >
> >     Conditions to recreate the bug:
> >       CONFIG_NET_SCH_PIE=y. Requires CAP_NET_ADMIN (namespace-local via
> >       unshare -Urn suffices).
> >
> >       tc qdisc add dev dummy0 root pie
> >       tc qdisc change dev dummy0 root pie stab data 32768 size_log 15 cell_log 0
>
> [Severity: Medium]
> Do these two commands actually reproduce the described condition?
>
> The clamped quantity comes only from the netdevice:
>
> include/net/pkt_sched.h:psched_mtu() {
>         return READ_ONCE(dev->mtu) + dev->hard_header_len;
> }
>
> Neither "tc qdisc add ... root pie" nor "tc qdisc change ... stab data 32768
> size_log 15 cell_log 0" changes dev->mtu or dev->hard_header_len, so dummy0
> keeps its ether_setup() defaults of 1500 + 14 = 1514.  That is already inside
> [1, 1<<20], so clamp_t() returns the same value and the reproducer sees no
> difference before or after the patch.  Shouldn't the steps include something
> like "ip link set dev dummy0 mtu 2147483634" (which dummy allows, since
> dummy_setup() leaves dev->min_mtu = 0 and dev->max_mtu = 0)?
>
> Also, does the size table have any bearing here?  A TCA_STAB table only
> affects qdisc_skb_cb(skb)->pkt_len via __qdisc_calculate_pkt_len(), which is
> already capped:
>
> net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
>         ...
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>         ...
> }
>
> and pie_qdisc_enqueue() does not use it as packet_size:
>
> net/sched/sch_pie.c:pie_qdisc_enqueue() {
>         ...
>         if (!pie_drop_early(sch, &q->params, &q->vars, sch->qstats.backlog,
>                             skb->len)) {
>         ...
> }
>
> Finally, the divide the message blames is gated on bytemode:
>
> net/sched/sch_pie.c:pie_drop_early() {
>         ...
>         if (params->bytemode && packet_size <= mtu)
>                 local_prob = (u64)packet_size * div_u64(local_prob, mtu);
>         ...
> }
>
> and bytemode is off by default:
>
> include/net/pie.h:pie_params_init() {
>         ...
>         params->bytemode = false;
>         ...
> }
>
> Since the listed commands never pass bytemode, is that branch ever reached in
> the reproducer?  Could the "Conditions to recreate the bug" section be updated
> to raise the MTU and enable bytemode, so that the Tested-by tag corresponds to
> a sequence that exercises the clamped value and the divide?
>
> >  {
> >       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);
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 7/9] net/sched: drr: clamp quantum in change class

[Jamal Hadi Salim]

drr_change_class() rejects explicit quantum==0 but falls back to
psched_mtu() with no floor. 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 under the qdisc lock.

Add clamp_t(u32, quantum, 256, 1<<20) after the zero reject and on the
fallback path. The explicit-zero reject is preserved.

Conditions to recreate the bug:
  CONFIG_NET_SCH_DRR=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

  tc qdisc add dev dummy0 root drr
  tc class add dev dummy0 parent 1: classid 1:1 drr quantum 1

Fixes: 13d2a1d2b032 ("pkt_sched: add DRR scheduler")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_drr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 91b1ef824afa..8621d057edd9 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 = clamp_t(u32, quantum, 256, 1 << 20);
 	} else
-		quantum = psched_mtu(qdisc_dev(sch));
+		quantum = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)), 256, 1 << 20);
 
 	if (cl != NULL) {
 		if (tca[TCA_RATE]) {
-- 
2.43.0

Re: [PATCH net v3 7/9] net/sched: drr: clamp quantum in change class

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] net/sched/sch_drr.c:85 and :87 (drr_change_class) silently
  rewrite an administrator-supplied TCA_DRR_QUANTUM instead of honouring…
- [Medium] Commit message premise and reproduction recipe do not match
  the tree the patch applies to, and an undeclared stable-backport…

--

net/sched: drr: clamp quantum in change class

This adds a clamp_t(u32, quantum, 256, 1 << 20) in drr_change_class(),
applied both to an explicitly supplied TCA_DRR_QUANTUM and to the
psched_mtu() fallback, so that the deficit-refill loop in drr_dequeue()
cannot iterate on a very small quantum.  The existing rejection of an
explicit quantum of zero is kept.

Two questions below, one about the changelog and one about the new
clamping behaviour.

> drr_change_class() rejects explicit quantum==0 but falls back to
> psched_mtu() with no floor. 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 under the qdisc lock.

[Severity: Medium]

Is the "~2 GiB" figure still accurate for this tree?  The baseline
already ends __qdisc_calculate_pkt_len() in net/sched/sch_api.c with:

	/* A size table can inflate qdisc_pkt_len() beyond any real packet
	 * (via overhead, the data table, or size_log); cap it so deficit
	 * schedulers such as DRR/ETS terminate their refill loops.
	 */
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	qdisc_skb_cb(skb)->pkt_len = pkt_len;

with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.

Since drr_dequeue() advances the deficit by one quantum per round:

	WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum));

wouldn't the worst case with quantum=1 be around 1M rounds rather than
~2^31?  The loop bound seems tied to that packet-length cap, not to a
2 GiB length.

Related to that, the recipe given:

>   tc qdisc add dev dummy0 root drr
>   tc class add dev dummy0 parent 1: classid 1:1 drr quantum 1

installs no size table and sends no traffic, so it does not appear to
exercise the refill loop at all.  Could the reproducer be extended to
include the size table and a transmit step?

There is also a backport angle.  With the new floor at 256 the effective
iteration bound (~4096 rounds) comes from QDISC_PKT_LEN_MAX rather than
from this change.  Trees matching "Fixes: 13d2a1d2b032" have no such cap,
so this hunk on its own still allows roughly 8.4M refill iterations per
dequeue there.  Should the QDISC_PKT_LEN_MAX commit be named as a
backport prerequisite?

> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index 91b1ef824afaf..8621d057edd9f 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 = clamp_t(u32, quantum, 256, 1 << 20);

[Severity: High]

Should an out-of-range quantum be rewritten rather than rejected here?
Only the exact value 0 returns -EINVAL; every other value now falls
through this clamp and is committed, either via the existing-class path
in drr_change_class():

		if (tb[TCA_DRR_QUANTUM])
			WRITE_ONCE(cl->quantum, quantum);

or via cl->quantum = quantum on the create path.  Netlink then returns
success for a configuration that was not applied.

Does this create a set/get asymmetry?  drr_dump_class() reports the
stored value:

	if (nla_put_u32(skb, TCA_DRR_QUANTUM, READ_ONCE(cl->quantum)))

so "tc class add ... drr quantum 1" succeeds and reads back 256.

Does this also change service ratios silently?  drr_dequeue() refills by
cl->quantum each round, so relative shares are the ratio of class
quanta.  A 10:100 weighting would become 256:256, and 4 MiB:1 MiB would
become 1 MiB:1 MiB, on any later "tc class change ... quantum <n>"
against a class that worked before.

The declared policy still advertises the whole u32 range:

	[TCA_DRR_QUANTUM]	= { .type = NLA_U32 },

The sibling implementation of the same bounded-quantum idea rejects
instead of mutating, in net/sched/sch_qfq.c:

	static const struct netlink_range_validation lmax_range = {
		.min = QFQ_MIN_LMAX,
		.max = QFQ_MAX_LMAX,
	};

	static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = {
		[TCA_QFQ_WEIGHT] = NLA_POLICY_RANGE(NLA_U32, 1, QFQ_MAX_WEIGHT),
		[TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range),
	};

and the sfq patch in this same series rejects the identical upper bound
in sfq_change():

	if (ctl->quantum > 1 << 20) {
		NL_SET_ERR_MSG_MOD(extack, "quantum too large");
		return -EINVAL;
	}

Would a declared NLA_POLICY_FULL_RANGE (or an explicit -EINVAL plus
extack) be preferable, which would also subsume the zero check?

One more on the upper bound: a quantum larger than qdisc_pkt_len() cannot
add refill rounds, so the 1 << 20 maximum does not seem related to the
loop described in the changelog.  Is the narrowing of the accepted range
intentional, and if so should it be spelled out in the commit message?
The message currently reads:

> Add clamp_t(u32, quantum, 256, 1<<20) after the zero reject and on the
> fallback path. The explicit-zero reject is preserved.

which reads as a hardening change with no behaviour change, and does not
mention that previously accepted values are now altered.  That seems
worth stating given the Fixes: tag implies a stable backport.

>  	} else
> -		quantum = psched_mtu(qdisc_dev(sch));
> +		quantum = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)), 256, 1 << 20);
>  
>  	if (cl != NULL) {
>  		if (tca[TCA_RATE]) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 7/9] net/sched: drr: clamp quantum in change class

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
>
> - [High] net/sched/sch_drr.c:85 and :87 (drr_change_class) silently
>   rewrite an administrator-supplied TCA_DRR_QUANTUM instead of honouring…


Valid as a "description". My view is the patch is fine.
The rewrite-vs-reject choice is the series' deliberate uAPI - the
whole point is that a too-small quantum is crazy as illustrated by the
series so it  is never installed...
I am not sure if sashiko is looking at the net tree which has commit
8f735d64382d but qdisc_pkt_len cap which is clamp-style already, and
Eric accepted v3 on this basis.
The asymmetry is discoverable by re-read (dump echoes the stored value).
Summary: If the series is resent, we will state the rewrite behaviour
in the changelog.

> - [Medium] Commit message premise and reproduction recipe do not match
>   the tree the patch applies to, and an undeclared stable-backport…
>

Very valid, if i understood correctly. I think you mean: backporting
on trees matching Fixes: 13d2a1d2b032 there is no QDISC_PKT_LEN_MAX
cap, so things will go crazy there...

I would say It's worth mentioning 8f735d64382d as a backport
prerequisite not sure if that's in the commit, although i am not
familiar enough with the backport rules to make that call.
+Cc stable@vger.kernel.org


cheers,
jamal

> net/sched: drr: clamp quantum in change class
>
> This adds a clamp_t(u32, quantum, 256, 1 << 20) in drr_change_class(),
> applied both to an explicitly supplied TCA_DRR_QUANTUM and to the
> psched_mtu() fallback, so that the deficit-refill loop in drr_dequeue()
> cannot iterate on a very small quantum.  The existing rejection of an
> explicit quantum of zero is kept.
>
> Two questions below, one about the changelog and one about the new
> clamping behaviour.
>
> > drr_change_class() rejects explicit quantum==0 but falls back to
> > psched_mtu() with no floor. 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 under the qdisc lock.
>
> [Severity: Medium]
>
> Is the "~2 GiB" figure still accurate for this tree?  The baseline
> already ends __qdisc_calculate_pkt_len() in net/sched/sch_api.c with:
>
>         /* A size table can inflate qdisc_pkt_len() beyond any real packet
>          * (via overhead, the data table, or size_log); cap it so deficit
>          * schedulers such as DRR/ETS terminate their refill loops.
>          */
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>         qdisc_skb_cb(skb)->pkt_len = pkt_len;
>
> with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
>
> Since drr_dequeue() advances the deficit by one quantum per round:
>
>         WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum));
>
> wouldn't the worst case with quantum=1 be around 1M rounds rather than
> ~2^31?  The loop bound seems tied to that packet-length cap, not to a
> 2 GiB length.
>
> Related to that, the recipe given:
>
> >   tc qdisc add dev dummy0 root drr
> >   tc class add dev dummy0 parent 1: classid 1:1 drr quantum 1
>
> installs no size table and sends no traffic, so it does not appear to
> exercise the refill loop at all.  Could the reproducer be extended to
> include the size table and a transmit step?
>
> There is also a backport angle.  With the new floor at 256 the effective
> iteration bound (~4096 rounds) comes from QDISC_PKT_LEN_MAX rather than
> from this change.  Trees matching "Fixes: 13d2a1d2b032" have no such cap,
> so this hunk on its own still allows roughly 8.4M refill iterations per
> dequeue there.  Should the QDISC_PKT_LEN_MAX commit be named as a
> backport prerequisite?
>
> > diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> > index 91b1ef824afaf..8621d057edd9f 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 = clamp_t(u32, quantum, 256, 1 << 20);
>
> [Severity: High]
>
> Should an out-of-range quantum be rewritten rather than rejected here?
> Only the exact value 0 returns -EINVAL; every other value now falls
> through this clamp and is committed, either via the existing-class path
> in drr_change_class():
>
>                 if (tb[TCA_DRR_QUANTUM])
>                         WRITE_ONCE(cl->quantum, quantum);
>
> or via cl->quantum = quantum on the create path.  Netlink then returns
> success for a configuration that was not applied.
>
> Does this create a set/get asymmetry?  drr_dump_class() reports the
> stored value:
>
>         if (nla_put_u32(skb, TCA_DRR_QUANTUM, READ_ONCE(cl->quantum)))
>
> so "tc class add ... drr quantum 1" succeeds and reads back 256.
>
> Does this also change service ratios silently?  drr_dequeue() refills by
> cl->quantum each round, so relative shares are the ratio of class
> quanta.  A 10:100 weighting would become 256:256, and 4 MiB:1 MiB would
> become 1 MiB:1 MiB, on any later "tc class change ... quantum <n>"
> against a class that worked before.
>
> The declared policy still advertises the whole u32 range:
>
>         [TCA_DRR_QUANTUM]       = { .type = NLA_U32 },
>
> The sibling implementation of the same bounded-quantum idea rejects
> instead of mutating, in net/sched/sch_qfq.c:
>
>         static const struct netlink_range_validation lmax_range = {
>                 .min = QFQ_MIN_LMAX,
>                 .max = QFQ_MAX_LMAX,
>         };
>
>         static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = {
>                 [TCA_QFQ_WEIGHT] = NLA_POLICY_RANGE(NLA_U32, 1, QFQ_MAX_WEIGHT),
>                 [TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range),
>         };
>
> and the sfq patch in this same series rejects the identical upper bound
> in sfq_change():
>
>         if (ctl->quantum > 1 << 20) {
>                 NL_SET_ERR_MSG_MOD(extack, "quantum too large");
>                 return -EINVAL;
>         }
>
> Would a declared NLA_POLICY_FULL_RANGE (or an explicit -EINVAL plus
> extack) be preferable, which would also subsume the zero check?
>
> One more on the upper bound: a quantum larger than qdisc_pkt_len() cannot
> add refill rounds, so the 1 << 20 maximum does not seem related to the
> loop described in the changelog.  Is the narrowing of the accepted range
> intentional, and if so should it be spelled out in the commit message?
> The message currently reads:
>
> > Add clamp_t(u32, quantum, 256, 1<<20) after the zero reject and on the
> > fallback path. The explicit-zero reject is preserved.
>
> which reads as a hardening change with no behaviour change, and does not
> mention that previously accepted values are now altered.  That seems
> worth stating given the Fixes: tag implies a stable backport.
>
> >       } else
> > -             quantum = psched_mtu(qdisc_dev(sch));
> > +             quantum = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)), 256, 1 << 20);
> >
> >       if (cl != NULL) {
> >               if (tca[TCA_RATE]) {
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 8/9] net/sched: ets: clamp quantum in parse and fallback paths

[Jamal Hadi Salim]

ets_qdisc_change() falls back to psched_mtu() with no floor for bands
without an explicit quantum. With a crafted size table qdisc_pkt_len
reaches ~2 GiB, so a zero psched_mtu on a headerless device makes the
deficit-refill loop spin under the qdisc lock.

Move the floor into ets_quantum_parse() so explicitly configured quanta
are also clamped to [256, 1<<20], not just the fallback path.

Conditions to recreate the bug:
  CONFIG_NET_SCH_ETS=y. Requires CAP_NET_ADMIN (namespace-local via
  unshare -Urn suffices).

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

Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
Reported-by: Vega <vega@nebusec.ai>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_ets.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index 25fcf4079fec..6cc902a03838 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -83,11 +83,7 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
 			     unsigned int *quantum,
 			     struct netlink_ext_ack *extack)
 {
-	*quantum = nla_get_u32(attr);
-	if (!*quantum) {
-		NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
-		return -EINVAL;
-	}
+	*quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
 	return 0;
 }
 
@@ -632,11 +628,13 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 			return err;
 	}
 	/* If there are more bands than strict + quanta provided, the remaining
-	 * ones are ETS with quantum of MTU. Initialize the missing values here.
+	 * ones are ETS with quantum of max(MTU, 256). Initialize the missing
+	 * values here.
 	 */
 	for (i = nstrict; i < nbands; i++) {
 		if (!quanta[i])
-			quanta[i] = psched_mtu(qdisc_dev(sch));
+			quanta[i] = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)),
+					    256, 1 << 20);
 	}
 
 	/* Before commit, make sure we can allocate all new qdiscs */
-- 
2.43.0

Re: [PATCH net v3 8/9] net/sched: ets: clamp quantum in parse and fallback paths

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 1

- [High] ets_quantum_parse() (net/sched/sch_ets.c:82-88) no longer
  validates user input at all: the explicit rejection of a zero quantum…
- [Medium] The clamping added to ets_quantum_parse() contradicts in-tree
  tdc test cases that are NOT updated anywhere in this series.
- [Medium] The commit message's "Conditions to recreate the bug" command,
  `tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1`, cannot…
- [Medium] The changelog's causal premise — "With a crafted size table
  qdisc_pkt_len reaches ~2 GiB" — is stale for this tree.
- [Low] The comment introduced in ets_qdisc_change() states the fallback
  bands get a "quantum of max(MTU, 256)", but the code immediately below…

--

net/sched: ets: clamp quantum in parse and fallback paths

This moves the quantum floor from the psched_mtu() fallback in
ets_qdisc_change() into ets_quantum_parse(), so that both explicitly
configured quanta and the MTU-derived fallback are clamped to the range
[256, 1<<20].

A few questions about the changelog and about the parse path below.

> net/sched: ets: clamp quantum in parse and fallback paths
>
> ets_qdisc_change() falls back to psched_mtu() with no floor for bands
> without an explicit quantum. With a crafted size table qdisc_pkt_len
> reaches ~2 GiB, so a zero psched_mtu on a headerless device makes the
> deficit-refill loop spin under the qdisc lock.

[Severity: Medium]
Is the "~2 GiB" premise still accurate for this tree?  The baseline already
carries a 1 MiB cap on qdisc_pkt_len:

include/net/pkt_sched.h:
    #define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
    ...
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	qdisc_skb_cb(skb)->pkt_len = pkt_len;
}

with the comment there saying the value is capped so that deficit
schedulers such as DRR/ETS terminate their refill loops.

If qdisc_pkt_len cannot exceed 1 MiB, then the only non-terminating case
left in ets_qdisc_dequeue() is quantum == 0, where

	cl->deficit += READ_ONCE(cl->quantum);

never grows and the while (1) loop spins under the qdisc lock.  That case
comes purely from the unfloored psched_mtu() fallback on a device whose
MTU + hard_header_len is 0.  With the 1 MiB packet-length cap plus the new
floor of 256 the loop is bounded to 4096 refill passes, so the fix looks
adequate; could the changelog describe the actual current failure mode
instead?

> Conditions to recreate the bug:
>   CONFIG_NET_SCH_ETS=y. Requires CAP_NET_ADMIN (namespace-local via
>   unshare -Urn suffices).
>
>   tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1

[Severity: Medium]
Does this command actually install a qdisc?  ets_qdisc_quanta_parse()
starts assignment at the first non-strict band:

net/sched/sch_ets.c:ets_qdisc_quanta_parse() {
	int band = nstrict;
    ...
		case TCA_ETS_QUANTA_BAND:
			if (band >= nbands) {
				NL_SET_ERR_MSG_MOD(extack, "ETS quanta has more values than bands");
				return -EINVAL;
			}
    ...
}

With bands 3 and strict 2, the first quantum lands in quanta[2] and band
becomes 3, so the second quantum hits band >= nbands and the whole add
fails with -EINVAL.  The tdc case fe3c (bands 4 strict 2 quanta 1000 2000
3000, expExitCode 1) encodes that same kernel-side rejection.

The quoted command also has neither of the two ingredients the message
says are needed: there is no size table (stab ...) to inflate
qdisc_pkt_len, and dummy0 has psched_mtu() of 1500 + 14 rather than 0.
Could a reproducer be given that reaches the psched_mtu() == 0 fallback,
so the Fixes: tag can be evaluated by stable backporters?

> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 25fcf4079fece..6cc902a038387 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -83,11 +83,7 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
>  			     unsigned int *quantum,
>  			     struct netlink_ext_ack *extack)
>  {
> -	*quantum = nla_get_u32(attr);
> -	if (!*quantum) {
> -		NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
> -		return -EINVAL;
> -	}
> +	*quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
>  	return 0;
>  }

[Severity: High]
Does this change user visible behaviour in two ways that the changelog
does not mention?  The changelog only says the floor is moved into
ets_quantum_parse().

First, the -EINVAL for a zero quantum disappears.  ets_quantum_parse() is
the sole validation point for TCA_ETS_QUANTA_BAND, reached from both
ets_qdisc_quanta_parse() and ets_class_change(), and it can no longer
fail:

	tc qdisc add ... ets quanta 1000 0 800 700
	tc class change ... ets quantum 0

Both used to return -EINVAL with an extack; now they succeed with quantum
silently set to 256.  The extack argument and the int return of
ets_quantum_parse() are both dead now, and the if (err) branch in
ets_class_change() is unreachable.

Second, a new 1 MiB ceiling is applied to explicitly configured quanta, so
quanta 4194304 1048576 (a 4:1 ratio) becomes 1:1, and quanta 1 1000000
becomes 256:1000000.  The installed band ratios and the weights handed to
hardware in ets_offload_change() then differ from what was asked for.

The mutated value is stored in cl->quantum and echoed back by
ets_qdisc_dump():

	if (nla_put_u32(skb, TCA_ETS_QUANTA_BAND,
			READ_ONCE(q->classes[band].quantum)))

so user space can only discover the change by re-reading and diffing its
own configuration.  The declared policies still advertise the full u32
range:

	static const struct nla_policy ets_quanta_policy[TCA_ETS_MAX + 1] = {
		[TCA_ETS_QUANTA_BAND] = { .type = NLA_U32 },
	};

Would it be better to follow the sibling patch in this series,
"net/sched: drr: clamp quantum in change class", which keeps the explicit
rejection and clamps afterwards?

net/sched/sch_drr.c:drr_change_class() {
    ...
		if (quantum == 0) {
			NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
			return -EINVAL;
		}
		quantum = clamp_t(u32, quantum, 256, 1 << 20);
    ...
}

Only the internal psched_mtu()-derived fallback needs the floor for
ets_qdisc_dequeue()'s refill loop to terminate.

[Severity: Medium]
Do two in-tree tdc cases still assert the removed rejection?  In
tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json, case e51a
("Zero element in quanta"):

    "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
    "expExitCode": "1",
    "matchPattern": "qdisc ets",
    "matchCount": "0",

and case e7f2 ("Sole zero element in quanta"):

    "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 0",
    "expExitCode": "1",
    "matchPattern": "qdisc ets",
    "matchCount": "0",

After the clamp, 0 becomes 256, the add succeeds, tc exits 0 and the qdisc
is present, so both cases fail on expExitCode and on matchCount.  Exit
code 1 in this file is the kernel-rejection code (cf. f7c8 and fe3c),
versus 2 for tc-side rejection in 65b6 and 9877.

Diffing ets.json between this commit and the end of the series
(2ebfd9fc19fbc4d4615473751bf847fc57e820fa) shows only case 41f5 changed by
"selftests: tc-testing: update ETS test 41f5 for clamped quanta", so e51a
and e7f2 are still broken at the tip of the series.  Could they be updated
in the same series, or the zero rejection kept so they keep passing?

For completeness, the related 41f5 concern (the dump no longer echoing
quanta 4294967294 1 1) is resolved later in the series by that same
commit, which rewrites the pattern to "quanta 1048576 256 256".

>  
> @@ -632,11 +628,13 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  			return err;
>  	}
>  	/* If there are more bands than strict + quanta provided, the remaining
> -	 * ones are ETS with quantum of MTU. Initialize the missing values here.
> +	 * ones are ETS with quantum of max(MTU, 256). Initialize the missing
> +	 * values here.
>  	 */
>  	for (i = nstrict; i < nbands; i++) {
>  		if (!quanta[i])
> -			quanta[i] = psched_mtu(qdisc_dev(sch));
> +			quanta[i] = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)),
> +					    256, 1 << 20);
>  	}

[Severity: Low]
This isn't a bug, but the comment says max(MTU, 256) while the code below
it computes min(max(MTU, 256), 1 MiB).  The upper bound is missing from
the comment, so on a device where psched_mtu() exceeds 1 MiB the described
behaviour and the actual behaviour differ.

>  
>  	/* Before commit, make sure we can allocate all new qdiscs */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 8/9] net/sched: ets: clamp quantum in parse and fallback paths

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 3 · Low: 1
>
> - [High] ets_quantum_parse() (net/sched/sch_ets.c:82-88) no longer
>   validates user input at all: the explicit rejection of a zero quantum…

Well... same argument as previous patch on drr.
The suggestion to mirror drr's reject-then-clamp (keep -EINVAL for 0,
clamp the rest) is reasonable but only if i have to resend.



> - [Medium] The clamping added to ets_quantum_parse() contradicts in-tree
>   tdc test cases that are NOT updated anywhere in this series.

Disagree.
Our own tdc tests show 386/386 pass on the patched kernel:
iproute2 rejects a literal 0 client-side, so e51a/e7f2 never reach the
kernel with 0. The tests keep passing; they just no longer exercise a
kernel-side check.

> - [Medium] The commit message's "Conditions to recreate the bug" command,
>   `tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1`, cannot…

Valid.
with bands 3 strict 2 the first quantum lands in the last non-strict
band and the second value trips the band-count check (-EINVAL, tdc
fe3c). The recipe should be `quanta 1` (single value) with a huge-MTU
device.
Iff i have to resend...

> - [Medium] The changelog's causal premise — "With a crafted size table
>   qdisc_pkt_len reaches ~2 GiB" — is stale for this tree.

More changelog wording.. if i have to resend...

> - [Low] The comment introduced in ets_qdisc_change() states the fallback
>   bands get a "quantum of max(MTU, 256)", but the code immediately below…

 cosmetic; correct as written since psched_mtu = mtu + hdr_len.

Summary:
Only the "high" issue might be worth considering (for consistency with
drr, not that it breaks anything)
Others is just Documentation/changelog, some worth changing if we resend.

cheers,
jamal
> --
>
> net/sched: ets: clamp quantum in parse and fallback paths
>
> This moves the quantum floor from the psched_mtu() fallback in
> ets_qdisc_change() into ets_quantum_parse(), so that both explicitly
> configured quanta and the MTU-derived fallback are clamped to the range
> [256, 1<<20].
>
> A few questions about the changelog and about the parse path below.
>
> > net/sched: ets: clamp quantum in parse and fallback paths
> >
> > ets_qdisc_change() falls back to psched_mtu() with no floor for bands
> > without an explicit quantum. With a crafted size table qdisc_pkt_len
> > reaches ~2 GiB, so a zero psched_mtu on a headerless device makes the
> > deficit-refill loop spin under the qdisc lock.
>
> [Severity: Medium]
> Is the "~2 GiB" premise still accurate for this tree?  The baseline already
> carries a 1 MiB cap on qdisc_pkt_len:
>
> include/net/pkt_sched.h:
>     #define QDISC_PKT_LEN_MAX   (1 << 20)       /* 1 MiB */
>
> net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
>     ...
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>         qdisc_skb_cb(skb)->pkt_len = pkt_len;
> }
>
> with the comment there saying the value is capped so that deficit
> schedulers such as DRR/ETS terminate their refill loops.
>
> If qdisc_pkt_len cannot exceed 1 MiB, then the only non-terminating case
> left in ets_qdisc_dequeue() is quantum == 0, where
>
>         cl->deficit += READ_ONCE(cl->quantum);
>
> never grows and the while (1) loop spins under the qdisc lock.  That case
> comes purely from the unfloored psched_mtu() fallback on a device whose
> MTU + hard_header_len is 0.  With the 1 MiB packet-length cap plus the new
> floor of 256 the loop is bounded to 4096 refill passes, so the fix looks
> adequate; could the changelog describe the actual current failure mode
> instead?
>
> > Conditions to recreate the bug:
> >   CONFIG_NET_SCH_ETS=y. Requires CAP_NET_ADMIN (namespace-local via
> >   unshare -Urn suffices).
> >
> >   tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1
>
> [Severity: Medium]
> Does this command actually install a qdisc?  ets_qdisc_quanta_parse()
> starts assignment at the first non-strict band:
>
> net/sched/sch_ets.c:ets_qdisc_quanta_parse() {
>         int band = nstrict;
>     ...
>                 case TCA_ETS_QUANTA_BAND:
>                         if (band >= nbands) {
>                                 NL_SET_ERR_MSG_MOD(extack, "ETS quanta has more values than bands");
>                                 return -EINVAL;
>                         }
>     ...
> }
>
> With bands 3 and strict 2, the first quantum lands in quanta[2] and band
> becomes 3, so the second quantum hits band >= nbands and the whole add
> fails with -EINVAL.  The tdc case fe3c (bands 4 strict 2 quanta 1000 2000
> 3000, expExitCode 1) encodes that same kernel-side rejection.
>
> The quoted command also has neither of the two ingredients the message
> says are needed: there is no size table (stab ...) to inflate
> qdisc_pkt_len, and dummy0 has psched_mtu() of 1500 + 14 rather than 0.
> Could a reproducer be given that reaches the psched_mtu() == 0 fallback,
> so the Fixes: tag can be evaluated by stable backporters?
>
> > diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > index 25fcf4079fece..6cc902a038387 100644
> > --- a/net/sched/sch_ets.c
> > +++ b/net/sched/sch_ets.c
> > @@ -83,11 +83,7 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
> >                            unsigned int *quantum,
> >                            struct netlink_ext_ack *extack)
> >  {
> > -     *quantum = nla_get_u32(attr);
> > -     if (!*quantum) {
> > -             NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
> > -             return -EINVAL;
> > -     }
> > +     *quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
> >       return 0;
> >  }
>
> [Severity: High]
> Does this change user visible behaviour in two ways that the changelog
> does not mention?  The changelog only says the floor is moved into
> ets_quantum_parse().
>
> First, the -EINVAL for a zero quantum disappears.  ets_quantum_parse() is
> the sole validation point for TCA_ETS_QUANTA_BAND, reached from both
> ets_qdisc_quanta_parse() and ets_class_change(), and it can no longer
> fail:
>
>         tc qdisc add ... ets quanta 1000 0 800 700
>         tc class change ... ets quantum 0
>
> Both used to return -EINVAL with an extack; now they succeed with quantum
> silently set to 256.  The extack argument and the int return of
> ets_quantum_parse() are both dead now, and the if (err) branch in
> ets_class_change() is unreachable.
>
> Second, a new 1 MiB ceiling is applied to explicitly configured quanta, so
> quanta 4194304 1048576 (a 4:1 ratio) becomes 1:1, and quanta 1 1000000
> becomes 256:1000000.  The installed band ratios and the weights handed to
> hardware in ets_offload_change() then differ from what was asked for.
>
> The mutated value is stored in cl->quantum and echoed back by
> ets_qdisc_dump():
>
>         if (nla_put_u32(skb, TCA_ETS_QUANTA_BAND,
>                         READ_ONCE(q->classes[band].quantum)))
>
> so user space can only discover the change by re-reading and diffing its
> own configuration.  The declared policies still advertise the full u32
> range:
>
>         static const struct nla_policy ets_quanta_policy[TCA_ETS_MAX + 1] = {
>                 [TCA_ETS_QUANTA_BAND] = { .type = NLA_U32 },
>         };
>
> Would it be better to follow the sibling patch in this series,
> "net/sched: drr: clamp quantum in change class", which keeps the explicit
> rejection and clamps afterwards?
>
> net/sched/sch_drr.c:drr_change_class() {
>     ...
>                 if (quantum == 0) {
>                         NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
>                         return -EINVAL;
>                 }
>                 quantum = clamp_t(u32, quantum, 256, 1 << 20);
>     ...
> }
>
> Only the internal psched_mtu()-derived fallback needs the floor for
> ets_qdisc_dequeue()'s refill loop to terminate.
>
> [Severity: Medium]
> Do two in-tree tdc cases still assert the removed rejection?  In
> tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json, case e51a
> ("Zero element in quanta"):
>
>     "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
>     "expExitCode": "1",
>     "matchPattern": "qdisc ets",
>     "matchCount": "0",
>
> and case e7f2 ("Sole zero element in quanta"):
>
>     "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 0",
>     "expExitCode": "1",
>     "matchPattern": "qdisc ets",
>     "matchCount": "0",
>
> After the clamp, 0 becomes 256, the add succeeds, tc exits 0 and the qdisc
> is present, so both cases fail on expExitCode and on matchCount.  Exit
> code 1 in this file is the kernel-rejection code (cf. f7c8 and fe3c),
> versus 2 for tc-side rejection in 65b6 and 9877.
>
> Diffing ets.json between this commit and the end of the series
> (2ebfd9fc19fbc4d4615473751bf847fc57e820fa) shows only case 41f5 changed by
> "selftests: tc-testing: update ETS test 41f5 for clamped quanta", so e51a
> and e7f2 are still broken at the tip of the series.  Could they be updated
> in the same series, or the zero rejection kept so they keep passing?
>
> For completeness, the related 41f5 concern (the dump no longer echoing
> quanta 4294967294 1 1) is resolved later in the series by that same
> commit, which rewrites the pattern to "quanta 1048576 256 256".
>
> >
> > @@ -632,11 +628,13 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
> >                       return err;
> >       }
> >       /* If there are more bands than strict + quanta provided, the remaining
> > -      * ones are ETS with quantum of MTU. Initialize the missing values here.
> > +      * ones are ETS with quantum of max(MTU, 256). Initialize the missing
> > +      * values here.
> >        */
> >       for (i = nstrict; i < nbands; i++) {
> >               if (!quanta[i])
> > -                     quanta[i] = psched_mtu(qdisc_dev(sch));
> > +                     quanta[i] = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)),
> > +                                         256, 1 << 20);
> >       }
>
> [Severity: Low]
> This isn't a bug, but the comment says max(MTU, 256) while the code below
> it computes min(max(MTU, 256), 1 MiB).  The upper bound is missing from
> the comment, so on a device where psched_mtu() exceeds 1 MiB the described
> behaviour and the actual behaviour differ.
>
> >
> >       /* Before commit, make sure we can allocate all new qdiscs */
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

[PATCH net v3 9/9] selftests: tc-testing: update ETS test 41f5 for clamped quanta

[Jamal Hadi Salim]

Commit "net/sched: ets: clamp quantum in parse and fallback paths"
moved the quantum floor into ets_quantum_parse(), so every explicitly
configured quantum is now clamped to [256, 1 << 20], not just the
psched_mtu() fallback.

Test 41f5 passes "quanta 4294967294 1 1" and matches the values back
verbatim, so all three bands now differ from what it expects:

  before: bands 3 quanta 4294967294 1 1
  after:  bands 3 quanta 1048576 256 256

Update the match pattern accordingly.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
index ee09e6d6fdf3..d2eab61c099a 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
@@ -987,7 +987,7 @@
     },
     {
         "id": "41f5",
-        "name": "ETS offload where the sum of quanta wraps u32",
+        "name": "ETS offload with out-of-range quanta clamped",
         "category": [
             "qdisc",
             "ets"
@@ -1002,7 +1002,7 @@
         "cmdUnderTest": "$TC qdisc add dev $ETH root ets quanta 4294967294 1 1",
         "expExitCode": "0",
         "verifyCmd": "$TC qdisc show dev $ETH",
-        "matchPattern": "qdisc ets .*bands 3 quanta 4294967294 1 1",
+        "matchPattern": "qdisc ets .*bands 3 quanta 1048576 256 256",
         "matchCount": "1",
         "teardown": [
             "echo \"1\" > /sys/bus/netdevsim/del_device"
-- 
2.43.0

Re: [PATCH net v3 9/9] selftests: tc-testing: update ETS test 41f5 for clamped quanta

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

- [Medium] Incomplete selftest update / undisclosed behavioural change:
  the accompanying commit 0bbbcd21591a ("net/sched: ets: clamp quantum…

Pre-existing issues:
- [Low] Concern that test 41f5 ("ETS offload with out-of-range quanta
  clamped") only checks the software qdisc dump via `tc qdisc show` and…

--

selftests: tc-testing: update ETS test 41f5 for clamped quanta

This updates tdc test 41f5 in qdiscs/ets.json so that its name and match
pattern reflect the new quantum clamping to [256, 1 << 20] introduced by
"net/sched: ets: clamp quantum in parse and fallback paths".  The test
still passes "quanta 4294967294 1 1" but now expects "quanta 1048576 256
256" in the qdisc dump.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> index ee09e6d6fdf3c..d2eab61c099a8 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> @@ -987,7 +987,7 @@
>      },
>      {
>          "id": "41f5",
> -        "name": "ETS offload where the sum of quanta wraps u32",
> +        "name": "ETS offload with out-of-range quanta clamped",
>          "category": [
>              "qdisc",
>              "ets"
> @@ -1002,7 +1002,7 @@
>          "cmdUnderTest": "$TC qdisc add dev $ETH root ets quanta 4294967294 1 1",
>          "expExitCode": "0",
>          "verifyCmd": "$TC qdisc show dev $ETH",
> -        "matchPattern": "qdisc ets .*bands 3 quanta 4294967294 1 1",
> +        "matchPattern": "qdisc ets .*bands 3 quanta 1048576 256 256",
>          "matchCount": "1",

[Severity: Low]
This isn't a bug introduced by this patch, but a note on what the renamed
test now covers.

The verification is only "$TC qdisc show dev $ETH", which reports the
software qdisc quanta.  The quanta actually handed to the driver are
filled in separately by ets_offload_change() in net/sched/sch_ets.c:

	qopt.replace_params.quanta[i] = quantum;
	qopt.replace_params.weights[i] = weight;

and netdevsim's nsim_setup_tc_ets() in drivers/net/netdevsim/tc.c does
nothing with them:

	case TC_ETS_REPLACE:
	case TC_ETS_DESTROY:
		break;

so nothing tdc can observe reflects the offload request itself.  That is
pre-existing and matches the idiom used by the other cases in this file.

Separately, since each quantum is now capped at 1 << 20 and there are at
most 16 bands, is the u32 sum-wrap condition the test was originally
written for still reachable at all?  If not, the rename retires that
coverage without a replacement.

[Severity: Medium]
Should the two neighbouring tests in this same file be updated in the
same patch?

The kernel-side commit in this series also removed the explicit
zero-quantum rejection from ets_quantum_parse(), which now reads:

net/sched/sch_ets.c:ets_quantum_parse() {
	*quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
	return 0;
}

The previous "ETS quantum cannot be zero" -EINVAL and extack are gone,
and nothing else in the path rejects 0: ets_quanta_policy declares
TCA_ETS_QUANTA_BAND as a bare NLA_U32 with no range, and
ets_qdisc_quanta_parse() only runs __nla_validate_nested() plus the band
count check before calling ets_quantum_parse().  A band quantum of 0 is
therefore clamped to 256 and the qdisc is created.

Tests e51a "Zero element in quanta" and e7f2 "Sole zero element in
quanta" still encode the old behaviour, for example e51a:

        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
        "expExitCode": "1",
        "verifyCmd": "$TC qdisc show dev $DUMMY",
        "matchPattern": "qdisc ets",
        "matchCount": "0"

If tc forwards the literal 0 to the kernel, which is what 41f5 suggests
given that it passes 4294967294 and 1 through unmodified, then the add
now returns 0 instead of 1 and "tc qdisc show" prints "qdisc ets", so
both tests fail.  If tc filters the 0 locally the tests keep passing but
no longer exercise any kernel check, and nothing covers the new
accept-and-clamp semantics.

Either way, could the removal of the zero-quantum -EINVAL and its extack
be mentioned in the changelog?  Neither this commit message nor the
kernel one notes that the netlink error contract for
TCA_ETS_QUANTA_BAND == 0 changed, and sch_drr.c deliberately kept its
explicit zero reject.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com

Re: [PATCH net v3 9/9] selftests: tc-testing: update ETS test 41f5 for clamped quanta

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
>
> - [Medium] Incomplete selftest update / undisclosed behavioural change:
>   the accompanying commit 0bbbcd21591a ("net/sched: ets: clamp quantum…

Note: the e51a/e7f2 breakage branch is wrong (verified: iproute2
rejects the literal 0 client-side, both tests pass on the patched
kernel per our tests).
Note2: The underlying point is fair, though: the error-contract change
for TCA_ETS_QUANTA_BAND == 0 deserves one changelog sentence.

> Pre-existing issues:
> - [Low] Concern that test 41f5 ("ETS offload with out-of-range quanta
>   clamped") only checks the software qdisc dump via `tc qdisc show` and…

That matches the file's existing goal; the retired sum-wrap case is
indeed unreachable with the 1 MiB per-band ceiling (16 bands x 1 MiB
<< 2^32).

IMO, other than the changelog change for "medium" - nothing else to see here.

Phew! Cant believe i went over all that. I thought this new era was
about keeping up with huge influx of bug reports - but the influx from
the bots is higher. I am sure the s/w quality will improve but man oh
man this is too much work (change the changelog, no human has ever
asked me that!)

cheers,
jamal


> --
>
> selftests: tc-testing: update ETS test 41f5 for clamped quanta
>
> This updates tdc test 41f5 in qdiscs/ets.json so that its name and match
> pattern reflect the new quantum clamping to [256, 1 << 20] introduced by
> "net/sched: ets: clamp quantum in parse and fallback paths".  The test
> still passes "quanta 4294967294 1 1" but now expects "quanta 1048576 256
> 256" in the qdisc dump.
>
> > diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> > index ee09e6d6fdf3c..d2eab61c099a8 100644
> > --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> > +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> > @@ -987,7 +987,7 @@
> >      },
> >      {
> >          "id": "41f5",
> > -        "name": "ETS offload where the sum of quanta wraps u32",
> > +        "name": "ETS offload with out-of-range quanta clamped",
> >          "category": [
> >              "qdisc",
> >              "ets"
> > @@ -1002,7 +1002,7 @@
> >          "cmdUnderTest": "$TC qdisc add dev $ETH root ets quanta 4294967294 1 1",
> >          "expExitCode": "0",
> >          "verifyCmd": "$TC qdisc show dev $ETH",
> > -        "matchPattern": "qdisc ets .*bands 3 quanta 4294967294 1 1",
> > +        "matchPattern": "qdisc ets .*bands 3 quanta 1048576 256 256",
> >          "matchCount": "1",
>
> [Severity: Low]
> This isn't a bug introduced by this patch, but a note on what the renamed
> test now covers.
>
> The verification is only "$TC qdisc show dev $ETH", which reports the
> software qdisc quanta.  The quanta actually handed to the driver are
> filled in separately by ets_offload_change() in net/sched/sch_ets.c:
>
>         qopt.replace_params.quanta[i] = quantum;
>         qopt.replace_params.weights[i] = weight;
>
> and netdevsim's nsim_setup_tc_ets() in drivers/net/netdevsim/tc.c does
> nothing with them:
>
>         case TC_ETS_REPLACE:
>         case TC_ETS_DESTROY:
>                 break;
>
> so nothing tdc can observe reflects the offload request itself.  That is
> pre-existing and matches the idiom used by the other cases in this file.
>
> Separately, since each quantum is now capped at 1 << 20 and there are at
> most 16 bands, is the u32 sum-wrap condition the test was originally
> written for still reachable at all?  If not, the rename retires that
> coverage without a replacement.
>
> [Severity: Medium]
> Should the two neighbouring tests in this same file be updated in the
> same patch?
>
> The kernel-side commit in this series also removed the explicit
> zero-quantum rejection from ets_quantum_parse(), which now reads:
>
> net/sched/sch_ets.c:ets_quantum_parse() {
>         *quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
>         return 0;
> }
>
> The previous "ETS quantum cannot be zero" -EINVAL and extack are gone,
> and nothing else in the path rejects 0: ets_quanta_policy declares
> TCA_ETS_QUANTA_BAND as a bare NLA_U32 with no range, and
> ets_qdisc_quanta_parse() only runs __nla_validate_nested() plus the band
> count check before calling ets_quantum_parse().  A band quantum of 0 is
> therefore clamped to 256 and the qdisc is created.
>
> Tests e51a "Zero element in quanta" and e7f2 "Sole zero element in
> quanta" still encode the old behaviour, for example e51a:
>
>         "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
>         "expExitCode": "1",
>         "verifyCmd": "$TC qdisc show dev $DUMMY",
>         "matchPattern": "qdisc ets",
>         "matchCount": "0"
>
> If tc forwards the literal 0 to the kernel, which is what 41f5 suggests
> given that it passes 4294967294 and 1 through unmodified, then the add
> now returns 0 instead of 1 and "tc qdisc show" prints "qdisc ets", so
> both tests fail.  If tc filters the 0 locally the tests keep passing but
> no longer exercise any kernel check, and nothing covers the new
> accept-and-clamp semantics.
>
> Either way, could the removal of the zero-quantum -EINVAL and its extack
> be mentioned in the changelog?  Neither this commit message nor the
> kernel one notes that the netlink error contract for
> TCA_ETS_QUANTA_BAND == 0 changed, and sch_drr.c deliberately kept its
> explicit zero reject.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com