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

On Fri, Sep 4, 2026 at 5:42 AM [off-list ref] 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().

quoted

    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.

quoted

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

[ ... ]

quoted

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