[PATCH net] net/sched: sch_hfsc: bound the classify inner-filter walk

[PATCH net] net/sched: sch_hfsc: bound the classify inner-filter walk

[Jamal Hadi Salim]

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, so two binds that were each legal at bind time can point at
each other; the classify walk then bounces between the two classes
forever with the qdisc lock held and BH disabled — a soft lockup from
a single packet. The stuck walk trips the watchdog on both KASAN and
KASAN-off builds:

  watchdog: BUG: soft lockup - CPU#3 stuck for 13s! [ping:444]
  RIP: 0010:u32_classify+0x542/0x17f0
  ...
  tcf_classify+0x66/0xa0
  hfsc_enqueue+0x166/0xdf0

  watchdog: BUG: soft lockup - CPU#0 stuck for 13s! [ping:340]
  tcf_action_exec+0x37/0x3e0
  u32_classify+0x12a/0x550
  hfsc_enqueue+0x7a/0x380
  Kernel panic - not syncing: softlockup: hung tasks

Bound the traversal the same way the HTB side was fixed: a sane walk
strictly descends the class tree, so it consumes fewer hops than the
level the walk starts at; anything beyond that is a cycle. Drop the
packet with a rate-limited warning when the bound is exhausted.

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/20260824161809.4147223-1-victor@mojatatu.com
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_hfsc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index e87f5021a199..71744a6c2f38 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1133,6 +1133,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 hops;
 	int result;
 
 	if (TC_H_MAJ(skb->priority ^ sch->handle) == 0 &&
@@ -1142,6 +1143,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	head = &q->root;
+	hops = head->level;
 	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 +1169,15 @@ 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, and levels
+		 * drift after bind time, so this walk can cycle.
+		 */
+		if (hops-- == 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

Re: [PATCH net] net/sched: sch_hfsc: bound the classify inner-filter walk

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] hfsc_classify() (net/sched/sch_hfsc.c:1146) seeds the new cycle
  guard with `hops = head->level` where `head = &q->root`.

--

net/sched: sch_hfsc: bound the classify inner-filter walk

This adds a hop budget to the inner-filter walk in hfsc_classify(), seeded
from the root class level, and drops the packet with a rate-limited warning
when the budget is exhausted. The intent is to stop the classify walk from
spinning forever when two flowid binds end up pointing at each other after
hfsc_adjust_levels() raises a class level.

Link: https://sashiko.dev/#/patchset/20260824161809.4147223-1-victor@mojatatu.com

> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index e87f5021a1995..71744a6c2f38e 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -1133,6 +1133,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 hops;
>  	int result;
>  
>  	if (TC_H_MAJ(skb->priority ^ sch->handle) == 0 &&
> @@ -1142,6 +1143,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
>  
>  	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>  	head = &q->root;
> +	hops = head->level;
        ^^^^^^^^^^^^^^^^^^

[Severity: High]

Can a legal, acyclic chain of flowid binds consume more hops than
q->root.level and get dropped here?

The budget root->level is an upper bound on the number of internal classes
visited only if every filter step strictly descends the tree. But as the
commit message itself points out, the descent check is only reached on the
classid-lookup path:

	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 */
	}

For a flowid bind, res.class is already set and no level relation is
enforced at classify time.  hfsc_bind_tcf() checks the relation once, at
bind time:

	if (cl != NULL) {
		if (p != NULL && p->level <= cl->level)
			return 0;

and hfsc_adjust_levels() later raises levels without revalidating the
stored res.class:

		WRITE_ONCE(cl->level, level);

So the walk can step between classes of equal level without cycling.
A configuration that the current code accepts at every step, with no cycle
at all:

- A1 child of root with one leaf child -> A1->level == 1, root->level == 2,
  so hops == 2.
- A2 as a leaf child of root (level 0); filter on A1 with flowid A2 is
  accepted because p(A1)->level 1 > cl(A2)->level 0.
- Add a leaf child to A2 -> A2->level becomes 1 (root->level stays 2).
- A3 as a leaf child of root; filter on A2 with flowid A3 accepted
  (1 > 0); then add a leaf child to A3 -> A3->level == 1.
- Filter on A3 with flowid A3's leaf child (strictly descending), root
  filter selects A1.

Walking a matching packet: A1 (hops 2 -> 1), A2 (hops 1 -> 0), A3 hits
hops-- == 0 and returns NULL, so hfsc_enqueue() drops the packet even
though the next step was a leaf.  Before this patch the same packet was
enqueued into A3's leaf.

Would a configuration-independent constant avoid this, the way the HTB
side does it in htb_classify()?

	if (++hops > TC_HTB_MAXDEPTH) {
		pr_warn_ratelimited("htb: classify loop detected, dropping packet\n");
		return NULL;
	}

The commit message says "Bound the traversal the same way the HTB side was
fixed: a sane walk strictly descends the class tree, so it consumes fewer
hops than the level the walk starts at".  Is that premise consistent with
the rest of the message, which states the strict-descent check "never runs"
for flowid binds and that levels drift after bind time?

>  	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 +1169,15 @@ 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, and levels
> +		 * drift after bind time, so this walk can cycle.
> +		 */
> +		if (hops-- == 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;

Also, since budget exhaustion is reported unconditionally as "classify loop
detected" without any evidence that a class was revisited, would the
warning be misleading for the acyclic case above?

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

Re: [PATCH net] net/sched: sch_hfsc: bound the classify inner-filter walk

[Jamal Hadi Salim]

On Sat, Sep 12, 2026 at 4:36 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
>
> - [High] hfsc_classify() (net/sched/sch_hfsc.c:1146) seeds the new cycle
>   guard with `hops = head->level` where `head = &q->root`.
>

Yes, this was an oversight on my part. I will send a v2.

pw-bot: cr

cheers,
jamal
> --
>
> net/sched: sch_hfsc: bound the classify inner-filter walk
>
> This adds a hop budget to the inner-filter walk in hfsc_classify(), seeded
> from the root class level, and drops the packet with a rate-limited warning
> when the budget is exhausted. The intent is to stop the classify walk from
> spinning forever when two flowid binds end up pointing at each other after
> hfsc_adjust_levels() raises a class level.
>
> Link: https://sashiko.dev/#/patchset/20260824161809.4147223-1-victor@mojatatu.com
>
> > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> > index e87f5021a1995..71744a6c2f38e 100644
> > --- a/net/sched/sch_hfsc.c
> > +++ b/net/sched/sch_hfsc.c
> > @@ -1133,6 +1133,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 hops;
> >       int result;
> >
> >       if (TC_H_MAJ(skb->priority ^ sch->handle) == 0 &&
> > @@ -1142,6 +1143,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> >
> >       *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> >       head = &q->root;
> > +     hops = head->level;
>         ^^^^^^^^^^^^^^^^^^
>
> [Severity: High]
>
> Can a legal, acyclic chain of flowid binds consume more hops than
> q->root.level and get dropped here?
>
> The budget root->level is an upper bound on the number of internal classes
> visited only if every filter step strictly descends the tree. But as the
> commit message itself points out, the descent check is only reached on the
> classid-lookup path:
>
>         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 */
>         }
>
> For a flowid bind, res.class is already set and no level relation is
> enforced at classify time.  hfsc_bind_tcf() checks the relation once, at
> bind time:
>
>         if (cl != NULL) {
>                 if (p != NULL && p->level <= cl->level)
>                         return 0;
>
> and hfsc_adjust_levels() later raises levels without revalidating the
> stored res.class:
>
>                 WRITE_ONCE(cl->level, level);
>
> So the walk can step between classes of equal level without cycling.
> A configuration that the current code accepts at every step, with no cycle
> at all:
>
> - A1 child of root with one leaf child -> A1->level == 1, root->level == 2,
>   so hops == 2.
> - A2 as a leaf child of root (level 0); filter on A1 with flowid A2 is
>   accepted because p(A1)->level 1 > cl(A2)->level 0.
> - Add a leaf child to A2 -> A2->level becomes 1 (root->level stays 2).
> - A3 as a leaf child of root; filter on A2 with flowid A3 accepted
>   (1 > 0); then add a leaf child to A3 -> A3->level == 1.
> - Filter on A3 with flowid A3's leaf child (strictly descending), root
>   filter selects A1.
>
> Walking a matching packet: A1 (hops 2 -> 1), A2 (hops 1 -> 0), A3 hits
> hops-- == 0 and returns NULL, so hfsc_enqueue() drops the packet even
> though the next step was a leaf.  Before this patch the same packet was
> enqueued into A3's leaf.
>
> Would a configuration-independent constant avoid this, the way the HTB
> side does it in htb_classify()?
>
>         if (++hops > TC_HTB_MAXDEPTH) {
>                 pr_warn_ratelimited("htb: classify loop detected, dropping packet\n");
>                 return NULL;
>         }
>
> The commit message says "Bound the traversal the same way the HTB side was
> fixed: a sane walk strictly descends the class tree, so it consumes fewer
> hops than the level the walk starts at".  Is that premise consistent with
> the rest of the message, which states the strict-descent check "never runs"
> for flowid binds and that levels drift after bind time?
>
> >       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 +1169,15 @@ 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, and levels
> > +              * drift after bind time, so this walk can cycle.
> > +              */
> > +             if (hops-- == 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;
>
> Also, since budget exhaustion is reported unconditionally as "classify loop
> detected" without any evidence that a class was revisited, would the
> warning be misleading for the acyclic case above?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-CTUU.v1.20260908094501%40mojatatu.com