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

[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 · 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…

--

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

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

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