* [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget
@ 2026-09-14 8:06 Jamal Hadi Salim
2026-09-14 8:06 ` [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test Jamal Hadi Salim
2026-09-15 8:17 ` [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget netdev-bot+sashiko
0 siblings, 2 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-14 8:06 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Jiri Pirko, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Victor Nogueira,
Sashiko, hybris, stable
hfsc_classify() applies the "filter may only point downwards" level
check only when the filter result carries no bound class. A filter
created with a flowid gets res.class set once at bind time, so the
check never runs for it during classification. hfsc_adjust_levels() can
later raise a class's level without revalidating existing bindings,
leaving two binds that were each legal at bind time pointing at each
other; the classify walk then bounces between two interior classes
forever with the qdisc lock held and BH disabled — a soft lockup from a
single packet.
Bound the traversal, mirroring HTB: count only the non-descending hops,
which a legitimate walked tree can only take by level drift after bind
time; anything beyond a small budget is a cycle.
Drop the packet with a rate-limited warning when the budget is
exhausted. Descending hops never consume budget, so legitimately deep
trees are unaffected, and a terminating lateral chain (the level-drift
false-positive family) still classifies normally.
Conditions to recreate the bug:
- CONFIG_NET_SCHED, CONFIG_NET_SCH_HFSC, CONFIG_NET_CLS_U32,
CONFIG_LOCKUP_DETECTOR.
- Build a cycle with two legal-at-bind-time flowid binds and a level
drift: class X 1:1 (child of root) with leaf child 1:10; class Y 1:2
(sibling of X) with children 1:20 and 1:200; root u32 filter flowid
1:1; filter on X flowid 1:2 (legal when Y is a leaf); after Y's level
rises to 2, filter on Y flowid 1:1 (legal then). Send one packet (ping
on the device). Unfixed kernel: classify spins with the qdisc lock
held; with softlockup_panic=1 it panics.
- Reachable from unprivileged user via unshare -Urn (CAP_NET_ADMIN).
Fixes: a2f79227138c ("net_sched: sch_hfsc: fix classification loops")
Reported-by: Sashiko (gemini + nipa) <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/QDISC-CTUU.v1.20260908094501@mojatatu.com
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2 changes:
- Replace the absolute hop bound (root->level) with a budget that
counts only non-descending hops, so a legal lateral level
chain still classifies. (Sashiko)
- Reword the "strict descent" changelog claim (Sashiko).
net/sched/sch_hfsc.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index e87f5021a199..b073efaf2bf5 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -386,6 +386,12 @@ cftree_update(struct hfsc_class *cl)
#define SM_MASK ((1ULL << SM_SHIFT) - 1)
#define ISM_MASK ((1ULL << ISM_SHIFT) - 1)
+/*
+ * bound on consecutive non-descending (lateral / upward) hops in a
+ * classify walk, mirroring htb's TC_HTB_MAXDEPTH headroom
+ */
+#define HFSC_CLASSIFY_MAX_DRIFT 8
+
static inline u64
seg_x2y(u64 x, u64 sm)
{
@@ -1133,6 +1139,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
struct hfsc_class *head, *cl;
struct tcf_result res;
struct tcf_proto *tcf;
+ unsigned int drift;
int result;
if (TC_H_MAJ(skb->priority ^ sch->handle) == 0 &&
@@ -1142,6 +1149,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
head = &q->root;
+ drift = HFSC_CLASSIFY_MAX_DRIFT;
tcf = rcu_dereference_bh(q->root.filter_list);
while (tcf && (result = tcf_classify_qdisc(skb, tcf, &res, false)) >= 0) {
#ifdef CONFIG_NET_CLS_ACT
@@ -1167,6 +1175,16 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
if (cl->level == 0)
return cl; /* hit leaf class */
+ /*
+ * flowid binds skip the level check above (res.class is set
+ * at bind time and levels drift after), so a walk can follow
+ * lateral hops without descending; bound them here.
+ */
+ if (cl->level >= head->level && drift-- == 0) {
+ pr_warn_ratelimited("hfsc: classify loop detected, dropping packet\n");
+ return NULL;
+ }
+
/* apply inner filter chain */
tcf = rcu_dereference_bh(cl->filter_list);
head = cl;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test
2026-09-14 8:06 [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget Jamal Hadi Salim
@ 2026-09-14 8:06 ` Jamal Hadi Salim
2026-09-15 8:17 ` netdev-bot+sashiko
2026-09-15 8:17 ` [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget netdev-bot+sashiko
1 sibling, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-14 8:06 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Jiri Pirko, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Victor Nogueira,
Sashiko, hybris
The classify-loop fix bounds the number of consecutive lateral
hops, so the guard must not misfire on a legal
walk that reaches its leaf through a level-drift lateral chain. Add a
case that builds exactly that chain (root -> 1:1 -> 1:2 -> 1:3 ->
leaf 1:31, with 1:2/1:3 deepened to interior level 1 after the binds)
and asserts traffic still reaches the leaf. Passes on the patched
kernel; on a too-tight bound it would drop the packet instead.
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
.../tc-testing/tc-tests/qdiscs/hfsc.json | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
index c98c339424d4..82d2f8af1460 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
@@ -169,5 +169,39 @@
"teardown": [
"$TC qdisc del dev $DUMMY handle 1: root"
]
+ },
+ {
+ "id": "8c39",
+ "name": "HFSC classify walk still reaches leaf after lateral drift",
+ "category": [
+ "qdisc",
+ "hfsc"
+ ],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "setup": [
+ "ip link set lo up",
+ "$TC qdisc add dev lo handle 1: root hfsc default 31",
+ "$TC class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbit",
+ "$TC class add dev lo parent 1: classid 1:2 hfsc rt m2 100kbit",
+ "$TC class add dev lo parent 1: classid 1:3 hfsc rt m2 100kbit",
+ "$TC class add dev lo parent 1:1 classid 1:10 hfsc rt m2 50kbit",
+ "$TC class add dev lo parent 1:2 classid 1:20 hfsc rt m2 10kbit",
+ "$TC class add dev lo parent 1:3 classid 1:30 hfsc rt m2 10kbit",
+ "$TC class add dev lo parent 1:3 classid 1:31 hfsc rt m2 100kbit",
+ "$TC filter add dev lo parent 1: protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:1",
+ "$TC filter add dev lo parent 1:1 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:2",
+ "$TC filter add dev lo parent 1:2 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:3",
+ "$TC filter add dev lo parent 1:3 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:31"
+ ],
+ "cmdUnderTest": "ping -n -c 10 -W 1 127.0.0.1; sleep 1",
+ "expExitCode": "0",
+ "verifyCmd": "$TC -s class show dev lo",
+ "matchPattern": "class hfsc 1:31 parent 1:3 rt.*\\n.*Sent [0-9]+ bytes [1-9][0-9]* pkt",
+ "matchCount": "1",
+ "teardown": [
+ "$TC qdisc del dev lo handle 1: root"
+ ]
}
]
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget
2026-09-14 8:06 [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget Jamal Hadi Salim
2026-09-14 8:06 ` [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test Jamal Hadi Salim
@ 2026-09-15 8:17 ` netdev-bot+sashiko
2026-09-16 11:22 ` Jamal Hadi Salim
1 sibling, 1 reply; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-15 8:17 UTC (permalink / raw)
To: jhs
Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, victor,
sashiko-bot, hybris
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
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index e87f5021a1995..b073efaf2bf5f 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -386,6 +386,12 @@ cftree_update(struct hfsc_class *cl)
> #define SM_MASK ((1ULL << SM_SHIFT) - 1)
> #define ISM_MASK ((1ULL << ISM_SHIFT) - 1)
>
> +/*
> + * bound on consecutive non-descending (lateral / upward) hops in a
> + * classify walk, mirroring htb's TC_HTB_MAXDEPTH headroom
> + */
> +#define HFSC_CLASSIFY_MAX_DRIFT 8
> +
> static inline u64
> seg_x2y(u64 x, u64 sm)
> {
[ ... ]
> @@ -1142,6 +1149,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
>
> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> head = &q->root;
> + drift = HFSC_CLASSIFY_MAX_DRIFT;
> tcf = rcu_dereference_bh(q->root.filter_list);
> while (tcf && (result = tcf_classify_qdisc(skb, tcf, &res, false)) >= 0) {
> #ifdef CONFIG_NET_CLS_ACT
> @@ -1167,6 +1175,16 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> if (cl->level == 0)
> return cl; /* hit leaf class */
>
> + /*
> + * flowid binds skip the level check above (res.class is set
> + * at bind time and levels drift after), so a walk can follow
> + * lateral hops without descending; bound them here.
> + */
> + if (cl->level >= head->level && drift-- == 0) {
> + pr_warn_ratelimited("hfsc: classify loop detected, dropping packet\n");
> + return NULL;
> + }
[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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test
2026-09-14 8:06 ` [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test Jamal Hadi Salim
@ 2026-09-15 8:17 ` netdev-bot+sashiko
2026-09-16 11:23 ` Jamal Hadi Salim
0 siblings, 1 reply; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-15 8:17 UTC (permalink / raw)
To: jhs
Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, victor,
sashiko-bot, hybris
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 2 · Low: 0
- [High] Test 8c39 ("HFSC classify walk still reaches leaf after lateral
drift") never exercises the new lateral-hop drift budget in…
- [Medium] Test 8c39 makes the qdisc's default class the same leaf it
asserts on: the root qdisc is created with `hfsc default 31`…
- [Medium] Test 8c39's cmdUnderTest is the compound shell command `ping
-n -c 10 -W 1 127.0.0.1; sleep 1` with expExitCode "0"…
--
selftests: tc-testing: add a lateral-drift hfsc classify-walk test
This adds tc-testing case 8c39, which builds an HFSC hierarchy with
lateral filter binds (root -> 1:1 -> 1:2 -> 1:3 -> leaf 1:31), pings
through it, and checks that packets are counted on class 1:31. The
intent is to show that the new classify-walk drift budget does not
misfire on a legal lateral chain.
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> index c98c339424d4e..82d2f8af1460d 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> @@ -169,5 +169,39 @@
> "teardown": [
> "$TC qdisc del dev $DUMMY handle 1: root"
> ]
> + },
> + {
> + "id": "8c39",
> + "name": "HFSC classify walk still reaches leaf after lateral drift",
> + "category": [
> + "qdisc",
> + "hfsc"
> + ],
> + "plugins": {
> + "requires": "nsPlugin"
> + },
> + "setup": [
> + "ip link set lo up",
> + "$TC qdisc add dev lo handle 1: root hfsc default 31",
> + "$TC class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbit",
> + "$TC class add dev lo parent 1: classid 1:2 hfsc rt m2 100kbit",
> + "$TC class add dev lo parent 1: classid 1:3 hfsc rt m2 100kbit",
> + "$TC class add dev lo parent 1:1 classid 1:10 hfsc rt m2 50kbit",
> + "$TC class add dev lo parent 1:2 classid 1:20 hfsc rt m2 10kbit",
> + "$TC class add dev lo parent 1:3 classid 1:30 hfsc rt m2 10kbit",
> + "$TC class add dev lo parent 1:3 classid 1:31 hfsc rt m2 100kbit",
> + "$TC filter add dev lo parent 1: protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:1",
> + "$TC filter add dev lo parent 1:1 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:2",
> + "$TC filter add dev lo parent 1:2 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:3",
> + "$TC filter add dev lo parent 1:3 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:31"
> + ],
[Severity: High]
Does this setup order actually build the lateral chain the test needs?
All of the child class adds (1:10, 1:20, 1:30, 1:31) come before the
filter adds, so by the time the lateral binds are issued 1:2 and 1:3 are
already interior classes at level 1.
For "$TC filter add dev lo parent 1:1 ... flowid 1:2", the class 1:1 is
passed as the bind base, and hfsc_bind_tcf() rejects the bind:
net/sched/sch_hfsc.c:hfsc_bind_tcf() {
...
if (cl != NULL) {
if (p != NULL && p->level <= cl->level)
return 0;
qdisc_class_get(&cl->cl_common);
}
...
}
With p = 1:1 (level 1) and cl = 1:2 (level 1), p->level <= cl->level
holds, so 0 is returned. __tcf_bind_filter() then stores that as
res.class:
include/net/pkt_cls.h:__tcf_bind_filter() {
cl = q->ops->cl_ops->bind_tcf(q, base, r->classid);
cl = __cls_set_class(&r->class, cl);
...
}
Nothing is propagated back to userspace, so the tc command still exits 0
and the setup step looks like it succeeded. The same applies to the
1:2 -> 1:3 filter.
At classify time the second hop therefore has res.class == NULL and the
walk stops at the pre-existing downward-only check rather than at the new
drift check:
net/sched/sch_hfsc.c:hfsc_classify() {
...
cl = (struct hfsc_class *)res.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 */
}
...
}
1 >= 1 breaks the loop, so the drift budget is never consumed and the
test would pass identically with a bound of 0.
Related to that, the commit message says the chain is built "with 1:2/1:3
deepened to interior level 1 after the binds", but in the setup list the
deepening class adds precede the binds. Should each deepening add follow
its corresponding lateral bind instead, i.e. bind 1:1 -> 1:2 while 1:2 is
still level 0, then add 1:20; bind 1:2 -> 1:3 while 1:3 is still level 0,
then add 1:30 and 1:31?
The commit message also states "on a too-tight bound it would drop the
packet instead" - with the current ordering, is that still true?
> + "cmdUnderTest": "ping -n -c 10 -W 1 127.0.0.1; sleep 1",
> + "expExitCode": "0",
[Severity: Medium]
Which command's exit status does expExitCode compare against here?
tdc runs cmdUnderTest through a shell:
tools/testing/selftests/tc-testing/tdc.py:exec_cmd() {
proc = subprocess.Popen(command,
shell=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env=ENVIR)
...
}
Because the ';' is a shell separator, proc.returncode is the status of
sleep 1, which is always 0. Does that make the expExitCode "0" check
unable to fail even if ping is missing or loses every echo request?
There is a second effect from the compound command. nsPlugin prepends
the netns wrapper only to the head of the split command:
tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py:adjust_command() {
cmdlist = command.split()
...
cmdlist.insert(0, self.args.NAMES['NS'])
cmdlist.insert(0, 'exec')
cmdlist.insert(0, 'netns')
cmdlist.insert(0, self.args.NAMES['IP'])
...
command = ' '.join(cmdlist)
}
So does "sleep 1" run in the initial namespace rather than in the test
namespace? Would putting the delay in the verify step, or dropping it,
keep the whole command inside the namespace and let the ping status be
the one that is checked?
> + "verifyCmd": "$TC -s class show dev lo",
> + "matchPattern": "class hfsc 1:31 parent 1:3 rt.*\\n.*Sent [0-9]+ bytes [1-9][0-9]* pkt",
> + "matchCount": "1",
[Severity: Medium]
Can this assertion distinguish a successful lateral walk from a failed
classification?
The root qdisc is created with "hfsc default 31" and the pattern counts
packets on class 1:31, which is the same class the default fallback
selects:
net/sched/sch_hfsc.c:hfsc_classify() {
...
/* classification failed, try default class */
cl = hfsc_find_class(TC_H_MAKE(TC_H_MAJ(sch->handle),
READ_ONCE(q->defcls)), sch);
if (cl == NULL || cl->level > 0)
return NULL;
return cl;
}
Any classification failure - a rejected bind, an early downward-only
break, or a future misfire of the drift guard - still credits the packets
to 1:31, so the tc output looks the same either way.
Would a default class distinct from the walk's terminal leaf (for example
default 30), or a default that is not a valid leaf so failures drop, make
this test able to fail when the guard misbehaves?
> + "teardown": [
> + "$TC qdisc del dev lo handle 1: root"
> + ]
> }
> ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-CTUU.v2.20260913192614%40mojatatu.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget
2026-09-15 8:17 ` [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget netdev-bot+sashiko
@ 2026-09-16 11:22 ` Jamal Hadi Salim
0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-16 11:22 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, victor,
sashiko-bot, hybris
On Tue, Sep 15, 2026 at 4:17 AM <netdev-bot+sashiko@kernel.org> 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_hfsc.c:hfsc_classify(): the new `drift` budget
> is a *cumulative* per-packet allowance of 8 non-descending hops, not…
>
I am going to send a v3 - i think the tdc test needs some improvement.
pw-bot:cr
cheers,
jamal
> --
>
> 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
>
> > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> > index e87f5021a1995..b073efaf2bf5f 100644
> > --- a/net/sched/sch_hfsc.c
> > +++ b/net/sched/sch_hfsc.c
> > @@ -386,6 +386,12 @@ cftree_update(struct hfsc_class *cl)
> > #define SM_MASK ((1ULL << SM_SHIFT) - 1)
> > #define ISM_MASK ((1ULL << ISM_SHIFT) - 1)
> >
> > +/*
> > + * bound on consecutive non-descending (lateral / upward) hops in a
> > + * classify walk, mirroring htb's TC_HTB_MAXDEPTH headroom
> > + */
> > +#define HFSC_CLASSIFY_MAX_DRIFT 8
> > +
> > static inline u64
> > seg_x2y(u64 x, u64 sm)
> > {
>
> [ ... ]
>
> > @@ -1142,6 +1149,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> >
> > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> > head = &q->root;
> > + drift = HFSC_CLASSIFY_MAX_DRIFT;
> > tcf = rcu_dereference_bh(q->root.filter_list);
> > while (tcf && (result = tcf_classify_qdisc(skb, tcf, &res, false)) >= 0) {
> > #ifdef CONFIG_NET_CLS_ACT
> > @@ -1167,6 +1175,16 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> > if (cl->level == 0)
> > return cl; /* hit leaf class */
> >
> > + /*
> > + * flowid binds skip the level check above (res.class is set
> > + * at bind time and levels drift after), so a walk can follow
> > + * lateral hops without descending; bound them here.
> > + */
> > + if (cl->level >= head->level && drift-- == 0) {
> > + pr_warn_ratelimited("hfsc: classify loop detected, dropping packet\n");
> > + return NULL;
> > + }
>
> [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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test
2026-09-15 8:17 ` netdev-bot+sashiko
@ 2026-09-16 11:23 ` Jamal Hadi Salim
0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-16 11:23 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, victor,
sashiko-bot, hybris
On Tue, Sep 15, 2026 at 4:17 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 2 · Low: 0
>
pw-bot:cr
cheers,
jamal
> - [High] Test 8c39 ("HFSC classify walk still reaches leaf after lateral
> drift") never exercises the new lateral-hop drift budget in…
> - [Medium] Test 8c39 makes the qdisc's default class the same leaf it
> asserts on: the root qdisc is created with `hfsc default 31`…
> - [Medium] Test 8c39's cmdUnderTest is the compound shell command `ping
> -n -c 10 -W 1 127.0.0.1; sleep 1` with expExitCode "0"…
>
> --
>
> selftests: tc-testing: add a lateral-drift hfsc classify-walk test
>
> This adds tc-testing case 8c39, which builds an HFSC hierarchy with
> lateral filter binds (root -> 1:1 -> 1:2 -> 1:3 -> leaf 1:31), pings
> through it, and checks that packets are counted on class 1:31. The
> intent is to show that the new classify-walk drift budget does not
> misfire on a legal lateral chain.
>
> > diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> > index c98c339424d4e..82d2f8af1460d 100644
> > --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> > +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/hfsc.json
> > @@ -169,5 +169,39 @@
> > "teardown": [
> > "$TC qdisc del dev $DUMMY handle 1: root"
> > ]
> > + },
> > + {
> > + "id": "8c39",
> > + "name": "HFSC classify walk still reaches leaf after lateral drift",
> > + "category": [
> > + "qdisc",
> > + "hfsc"
> > + ],
> > + "plugins": {
> > + "requires": "nsPlugin"
> > + },
> > + "setup": [
> > + "ip link set lo up",
> > + "$TC qdisc add dev lo handle 1: root hfsc default 31",
> > + "$TC class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbit",
> > + "$TC class add dev lo parent 1: classid 1:2 hfsc rt m2 100kbit",
> > + "$TC class add dev lo parent 1: classid 1:3 hfsc rt m2 100kbit",
> > + "$TC class add dev lo parent 1:1 classid 1:10 hfsc rt m2 50kbit",
> > + "$TC class add dev lo parent 1:2 classid 1:20 hfsc rt m2 10kbit",
> > + "$TC class add dev lo parent 1:3 classid 1:30 hfsc rt m2 10kbit",
> > + "$TC class add dev lo parent 1:3 classid 1:31 hfsc rt m2 100kbit",
> > + "$TC filter add dev lo parent 1: protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:1",
> > + "$TC filter add dev lo parent 1:1 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:2",
> > + "$TC filter add dev lo parent 1:2 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:3",
> > + "$TC filter add dev lo parent 1:3 protocol ip prio 1 u32 match u8 0 0 at 0 flowid 1:31"
> > + ],
>
> [Severity: High]
> Does this setup order actually build the lateral chain the test needs?
>
> All of the child class adds (1:10, 1:20, 1:30, 1:31) come before the
> filter adds, so by the time the lateral binds are issued 1:2 and 1:3 are
> already interior classes at level 1.
>
> For "$TC filter add dev lo parent 1:1 ... flowid 1:2", the class 1:1 is
> passed as the bind base, and hfsc_bind_tcf() rejects the bind:
>
> net/sched/sch_hfsc.c:hfsc_bind_tcf() {
> ...
> if (cl != NULL) {
> if (p != NULL && p->level <= cl->level)
> return 0;
> qdisc_class_get(&cl->cl_common);
> }
> ...
> }
>
> With p = 1:1 (level 1) and cl = 1:2 (level 1), p->level <= cl->level
> holds, so 0 is returned. __tcf_bind_filter() then stores that as
> res.class:
>
> include/net/pkt_cls.h:__tcf_bind_filter() {
> cl = q->ops->cl_ops->bind_tcf(q, base, r->classid);
> cl = __cls_set_class(&r->class, cl);
> ...
> }
>
> Nothing is propagated back to userspace, so the tc command still exits 0
> and the setup step looks like it succeeded. The same applies to the
> 1:2 -> 1:3 filter.
>
> At classify time the second hop therefore has res.class == NULL and the
> walk stops at the pre-existing downward-only check rather than at the new
> drift check:
>
> net/sched/sch_hfsc.c:hfsc_classify() {
> ...
> cl = (struct hfsc_class *)res.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 */
> }
> ...
> }
>
> 1 >= 1 breaks the loop, so the drift budget is never consumed and the
> test would pass identically with a bound of 0.
>
> Related to that, the commit message says the chain is built "with 1:2/1:3
> deepened to interior level 1 after the binds", but in the setup list the
> deepening class adds precede the binds. Should each deepening add follow
> its corresponding lateral bind instead, i.e. bind 1:1 -> 1:2 while 1:2 is
> still level 0, then add 1:20; bind 1:2 -> 1:3 while 1:3 is still level 0,
> then add 1:30 and 1:31?
>
> The commit message also states "on a too-tight bound it would drop the
> packet instead" - with the current ordering, is that still true?
>
> > + "cmdUnderTest": "ping -n -c 10 -W 1 127.0.0.1; sleep 1",
> > + "expExitCode": "0",
>
> [Severity: Medium]
> Which command's exit status does expExitCode compare against here?
>
> tdc runs cmdUnderTest through a shell:
>
> tools/testing/selftests/tc-testing/tdc.py:exec_cmd() {
> proc = subprocess.Popen(command,
> shell=True,
> stdout=subprocess.PIPE,
> stderr=subprocess.PIPE,
> env=ENVIR)
> ...
> }
>
> Because the ';' is a shell separator, proc.returncode is the status of
> sleep 1, which is always 0. Does that make the expExitCode "0" check
> unable to fail even if ping is missing or loses every echo request?
>
> There is a second effect from the compound command. nsPlugin prepends
> the netns wrapper only to the head of the split command:
>
> tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py:adjust_command() {
> cmdlist = command.split()
> ...
> cmdlist.insert(0, self.args.NAMES['NS'])
> cmdlist.insert(0, 'exec')
> cmdlist.insert(0, 'netns')
> cmdlist.insert(0, self.args.NAMES['IP'])
> ...
> command = ' '.join(cmdlist)
> }
>
> So does "sleep 1" run in the initial namespace rather than in the test
> namespace? Would putting the delay in the verify step, or dropping it,
> keep the whole command inside the namespace and let the ping status be
> the one that is checked?
>
> > + "verifyCmd": "$TC -s class show dev lo",
> > + "matchPattern": "class hfsc 1:31 parent 1:3 rt.*\\n.*Sent [0-9]+ bytes [1-9][0-9]* pkt",
> > + "matchCount": "1",
>
> [Severity: Medium]
> Can this assertion distinguish a successful lateral walk from a failed
> classification?
>
> The root qdisc is created with "hfsc default 31" and the pattern counts
> packets on class 1:31, which is the same class the default fallback
> selects:
>
> net/sched/sch_hfsc.c:hfsc_classify() {
> ...
> /* classification failed, try default class */
> cl = hfsc_find_class(TC_H_MAKE(TC_H_MAJ(sch->handle),
> READ_ONCE(q->defcls)), sch);
> if (cl == NULL || cl->level > 0)
> return NULL;
>
> return cl;
> }
>
> Any classification failure - a rejected bind, an early downward-only
> break, or a future misfire of the drift guard - still credits the packets
> to 1:31, so the tc output looks the same either way.
>
> Would a default class distinct from the walk's terminal leaf (for example
> default 30), or a default that is not a valid leaf so failures drop, make
> this test able to fail when the guard misbehaves?
>
> > + "teardown": [
> > + "$TC qdisc del dev lo handle 1: root"
> > + ]
> > }
> > ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-CTUU.v2.20260913192614%40mojatatu.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-16 11:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-14 8:06 [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget Jamal Hadi Salim
2026-09-14 8:06 ` [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test Jamal Hadi Salim
2026-09-15 8:17 ` netdev-bot+sashiko
2026-09-16 11:23 ` Jamal Hadi Salim
2026-09-15 8:17 ` [PATCH net v2 1/2] net/sched: sch_hfsc: bound the classify inner-filter walk with a drift budget netdev-bot+sashiko
2026-09-16 11:22 ` Jamal Hadi Salim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox