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

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