Re: [PATCH net v2 2/2] selftests: tc-testing: add a lateral-drift hfsc classify-walk test

[netdev-bot+sashiko@kernel.org]

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