Re: [PATCH net 1/2] net/sched: hhf: cap hh_flows_limit at change time

On Sat, Sep 12, 2026 at 4:36 PM [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 · Low: 0

- [Medium] net/sched/sch_hhf.c: the new `[TCA_HHF_HH_FLOWS_LIMIT] =
  NLA_POLICY_MAX(NLA_U32, 2 * HH_FLOWS_CNT)` policy entry (line 530) is…

These findings by sashiko-nipa are accurate but as in the previous
codel thread they concern changelog and selftest-robustness nits.
IOW, there is _no code defect_. The -ERANGE behavior change on add
path and the changelog overstatement are worth a one-line fix if/when
the thread is re-rev'd, but IMO are not a revision blocker


cheers,
jamal

--

net/sched: hhf: cap hh_flows_limit at change time

The patch bounds TCA_HHF_HH_FLOWS_LIMIT with NLA_POLICY_MAX() at
2*HH_FLOWS_CNT, passes extack down to the nested parse so the rejected
value is reported, and moves the hh_flows_limit default in hhf_init()
ahead of the hhf_change() call so a user-supplied value at add time is no
longer overwritten.

Closes: https://sashiko.dev/#/patchset/20260822195509.112717-1-jhs@mojatatu.com

quoted

diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index fc72f825fbd92..5dec1ed969adf 100644
--- a/net/sched/sch_hhf.c
+++ b/net/sched/sch_hhf.c

@@ -527,7 +527,7 @@ static void hhf_destroy(struct Qdisc *sch)
 static const struct nla_policy hhf_policy[TCA_HHF_MAX + 1] = {
      [TCA_HHF_BACKLOG_LIMIT]  = { .type = NLA_U32 },
      [TCA_HHF_QUANTUM]        = { .type = NLA_U32 },
-     [TCA_HHF_HH_FLOWS_LIMIT] = { .type = NLA_U32 },
+     [TCA_HHF_HH_FLOWS_LIMIT] = NLA_POLICY_MAX(NLA_U32, 2 * HH_FLOWS_CNT),

[Severity: Medium]
Does this policy entry also change the behaviour of the qdisc creation
path?  The subject says "cap hh_flows_limit at change time", but hhf_init()
calls hhf_change(), so the same policy is applied on add.

validate_nla() dispatches the max check from the pt->validation_type
switch, independently of the validation level:

lib/nlattr.c:validate_nla() {
        ...
        case NLA_VALIDATE_MAX:
                err = nla_validate_int_range(pt, nla, extack, validate);
                if (err)
                        return err;
        ...
}

so even the NL_VALIDATE_LIBERAL level used by
nla_parse_nested_deprecated() returns -ERANGE, and hhf_init() propagates
it:

        if (opt) {
                int err = hhf_change(sch, opt, extack);

                if (err)
                        return err;
        }

At baseline, tc qdisc add dev X root hhf hh_limit 4294967295 returned
success, because hhf_init() unconditionally reassigned the safe default
after hhf_change():

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

With this change the same command fails with -ERANGE and no qdisc is
installed.  Is that intended for a Cc: stable patch, and should it be
mentioned in the changelog?

The commit message says:

    Configs relying on hh_limit above the default were relying
    on unbounded, unsafe behaviour and are not supported going forward.

Is that accurate for the add path?  A setup doing tc qdisc add ... hh_limit
<value above 2048> was running with hh_flows_limit clobbered to 2048, so it
was not exposed to the unbounded allocation behaviour described earlier in
the changelog; only tc qdisc change could reach that state.

The sibling knob in the same function is clamped rather than rejected, from
the referenced eb56a495f59b:

        if (tb[TCA_HHF_QUANTUM])
                new_quantum = max(256U, nla_get_u32(tb[TCA_HHF_QUANTUM]));

Would a min() on hh_flows_limit in hhf_change() give the same memory bound
without turning previously-successful add commands into failures?  If
rejection is preferred, could the changelog state that the add path now
returns -ERANGE for values above 2*HH_FLOWS_CNT?

quoted

@@ -546,7 +546,7 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt,
      u32 new_hhf_non_hh_weight = q->hhf_non_hh_weight;

      err = nla_parse_nested_deprecated(tb, TCA_HHF_MAX, opt, hhf_policy,
-                                       NULL);
+                                       extack);
      if (err < 0)
              return err;

@@ -624,6 +624,9 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
      q->hhf_evict_timeout = HZ;      /* 1  sec */
      q->hhf_non_hh_weight = 2;

+     /* Cap max active HHs at twice len of hh_flows table. */
+     q->hh_flows_limit = 2 * HH_FLOWS_CNT;
+
      if (opt) {
              int err = hhf_change(sch, opt, extack);

@@ -639,8 +642,6 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt,
              for (i = 0; i < HH_FLOWS_CNT; i++)
                      INIT_LIST_HEAD(&q->hh_flows[i]);

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

Moving the default ahead of hhf_change() is what makes the add-time value
effective for the first time, so this hunk and the policy hunk together
produce the new add-path failure described above.

--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-B855.v1.20260911153152%40mojatatu.com