Re: [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget

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_hfsc.c:hfsc_classify(): the new `drift` budget
  is a *cumulative* per-packet allowance of 8 non-descending hops, not…

--

net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget

hfsc_classify() can loop forever between two interior classes when two
flowid binds that were each legal at bind time become mutually pointing
after hfsc_adjust_levels() raises a level. The patch adds a budget of
non-descending hops to the classify walk and drops the packet with a
rate-limited warning when the budget runs out.

Link: https://sashiko.dev/#/patchset/QDISC-CTUU.v1.20260908094501@mojatatu.com

[ ... ]

[Severity: Medium]
Can this drop packets for an acyclic configuration?  drift is seeded once
before the while loop in hfsc_classify() and the only mutation is the
post-decrement above, so a descending hop never returns any budget.  That
makes the budget cumulative over the whole walk rather than the bound on
"consecutive non-descending (lateral / upward) hops" the macro comment
describes, and rather than what the changelog says:

  "Bound the traversal, mirroring HTB: count only the non-descending hops"
  "a terminating lateral chain (the level-drift false-positive family)
   still classifies normally"

A terminating lateral chain longer than eight hops, or several shorter
lateral segments separated by legal descents, would exhaust the budget.
Such a chain is constructible with plain tc commands because bindings are
only checked at bind time:

net/sched/sch_hfsc.c:hfsc_bind_tcf() {
	if (cl != NULL) {
		if (p != NULL && p->level <= cl->level)
			return 0;

and levels are raised afterwards with no revalidation:

net/sched/sch_hfsc.c:hfsc_adjust_levels() {
	do {
		level = 0;
		list_for_each_entry(p, &cl->children, siblings) {
			if (p->level >= level)
				level = p->level + 1;
		}
		WRITE_ONCE(cl->level, level);
	} while ((cl = cl->cl_parent) != NULL);

So for sibling classes A1..A10, each with one leaf child, binding a filter
on A_i with flowid A_{i+1} while A_{i+1} is still a leaf passes
hfsc_bind_tcf(), and adding a child to A_{i+1} afterwards turns that hop
lateral.  Would resetting drift back to HFSC_CLASSIFY_MAX_DRIFT on every
descending hop match the documented intent?

Should this path use break instead of return NULL?  The two existing
failure exits in the same loop break out to the default class:

		if (!cl) {
			cl = hfsc_find_class(res.classid, sch);
			if (!cl)
				break; /* filter selected invalid classid */
			if (cl->level >= head->level)
				break; /* filter may only point downwards */
		}

	/* classification failed, try default class */
	cl = hfsc_find_class(TC_H_MAKE(TC_H_MAJ(sch->handle),
				       READ_ONCE(q->defcls)), sch);

With return NULL, a configured defcls leaf is skipped and hfsc_enqueue()
does qdisc_qstats_drop(sch) plus __qdisc_drop(skb, to_free).

On the macro comment itself, is "mirroring htb's TC_HTB_MAXDEPTH headroom"
accurate?  htb_classify() increments hops on every inner hop and HTB
enforces a maximum depth of TC_HTB_MAXDEPTH structurally, so its bound
cannot reject a legal walk.  HFSC caps neither class level nor hierarchy
depth, so the value 8 has no equivalent backing here.  Could the comment
and the changelog sentence "anything beyond a small budget is a cycle" be
reworded to describe the heuristic that is actually implemented?

Also, the warning text asserts "classify loop detected" on a walk where no
class has been revisited.  Would a message that reports the budget being
exhausted be more accurate?

 		/* apply inner filter chain */
 		tcf = rcu_dereference_bh(cl->filter_list);
 		head = cl;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-CTUU.v2.20260913192614%40mojatatu.com