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
