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

[netdev-bot+sashiko@kernel.org]

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