Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit body describes the patch as a change relative to an
  unmerged v2 posting, not as a change to the tree.
- [Low] Both rejection cases check only that tc exits with 2 and that no
  matching qdisc is dumped.

--

selftests: tc-testing: teql: cover each attach rejection on its own

This commit moves tdc case 124e out of the nsPlugin test namespace and
into init_net with its own dummy device. Only the root-only check in
teql_qdisc_init() can then refuse it. It also adds case 6a06, which
attaches teql0 at root from the test namespace, so that only the
namespace check can refuse that one.

Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
Link: https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/

> It no longer proves that a child attach is
> refused as such, and the case added in the previous posting covers the
> namespace check alone.

[ ... ]

> Attach a root qdisc from the test namespace in the second case, so that
> only the namespace check can refuse that one.

[Severity: Low]
This isn't a bug, but these sentences read as if a namespace rejection
case is already in the tree. Is that the case?

Case 6a06 ("Reject TEQL attach from a different network namespace") is
added in full by this commit. The parent commit, "net/sched: sch_teql:
reject qdiscs from another network namespace", changes nothing under
tools/testing/selftests/tc-testing/.

The only place that says a new case is added is the v1 -> v2 and
v2 -> v3 changelog. That text sits below the "---", so it is dropped
when the patch is applied.

Could the body say outright that this commit adds new coverage for the
namespace rejection and moves 124e into init_net?

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> index 0179c57104adb..ab5d0881c0070 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json

[ ... ]

> @@ -104,7 +100,35 @@
>          "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:1",
>          "matchJSON": [],
>          "teardown": [
> -            "$TC qdisc del dev $DUMMY root handle 1:"
> +            "$TC qdisc del dev $DUMMY root handle 1:",
> +            "$IP link del dev $DUMMY"
> +        ]
> +    },
> +    {
> +        "id": "6a06",
> +        "name": "Reject TEQL attach from a different network namespace",

[ ... ]

> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY root handle 3: teql0",
> +        "expExitCode": "2",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc teql0 3: root",
> +        "matchCount": "0",

[Severity: Low]
Can this case pass even when the namespace check is not what refused
the attach?

tc exits with 2 for any netlink error. The verify step only checks that
no teql0 qdisc shows up in the dump. Case 124e has the same weakness:
it checks only expExitCode "2" and matchJSON [].

In teql_qdisc_init(), both rejections return the same errno. Only the
extack text tells them apart:

net/sched/sch_teql.c:teql_qdisc_init() {
    ...
	if (sch->parent != TC_H_ROOT) {
		NL_SET_ERR_MSG_MOD(extack, "teql can only be used as root");
		return -EOPNOTSUPP;
	}

	if (dev_net(dev) != dev_net(m->dev)) {
		NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
		return -EOPNOTSUPP;
	}
    ...
}

The tc-testing config sets CONFIG_NET_SCH_TEQL=m. Unlike the other
sch_* modules, sch_teql.c has no MODULE_ALIAS_NET_SCH(). So the
alias-based request_module() never autoloads it, and the lookup fails:

net/sched/sch_api.c:qdisc_create() {
    ...
	ops = qdisc_lookup_ops(kind);
	if (!ops) {
		err = -ENOENT;
		NL_SET_ERR_MSG(extack, "Specified qdisc kind is unknown");
		goto err_out;
	}
    ...
}

When that happens, tc still exits with 2 and nothing named teql0
appears. Both 6a06 and 124e then pass, whether or not the kernel has
either check. That includes a kernel without the namespace fix.

The module is loaded only by try_modprobe() in tdc.sh. That function
just prints "skipping" when modprobe fails. Running tdc.py directly,
for example with -e 6a06 or -c teql, never loads it.

The commit message says "only the root-only check can refuse it" and
"only the namespace check can refuse that one". That only holds when
sch_teql is loaded.

Would it make sense for each case to match the specific extack
message? Another option is to fail when the teql0 kind is unavailable.

The positive teql cases would fail in the same setup, so the suite as a
whole would still show that something is wrong.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[Jamal Hadi Salim]

On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit body describes the patch as a change relative to an
>   unmerged v2 posting, not as a change to the tree.
> - [Low] Both rejection cases check only that tc exits with 2 and that no
>   matching qdisc is dumped.
>

Again cosmetics - so far no reason to resend.
6a06 is added by this commit, not by a previous posting, if i resend i
would reword to state that.
The module point cannot cause a pass here - it would fail several other tests..


cheers,
jamal
> --
>
> selftests: tc-testing: teql: cover each attach rejection on its own
>
> This commit moves tdc case 124e out of the nsPlugin test namespace and
> into init_net with its own dummy device. Only the root-only check in
> teql_qdisc_init() can then refuse it. It also adds case 6a06, which
> attaches teql0 at root from the test namespace, so that only the
> namespace check can refuse that one.
>
> Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
> Link: https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/
>
> > It no longer proves that a child attach is
> > refused as such, and the case added in the previous posting covers the
> > namespace check alone.
>
> [ ... ]
>
> > Attach a root qdisc from the test namespace in the second case, so that
> > only the namespace check can refuse that one.
>
> [Severity: Low]
> This isn't a bug, but these sentences read as if a namespace rejection
> case is already in the tree. Is that the case?
>
> Case 6a06 ("Reject TEQL attach from a different network namespace") is
> added in full by this commit. The parent commit, "net/sched: sch_teql:
> reject qdiscs from another network namespace", changes nothing under
> tools/testing/selftests/tc-testing/.
>
> The only place that says a new case is added is the v1 -> v2 and
> v2 -> v3 changelog. That text sits below the "---", so it is dropped
> when the patch is applied.
>
> Could the body say outright that this commit adds new coverage for the
> namespace rejection and moves 124e into init_net?
>
> > diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> > index 0179c57104adb..ab5d0881c0070 100644
> > --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> > +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
>
> [ ... ]
>
> > @@ -104,7 +100,35 @@
> >          "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:1",
> >          "matchJSON": [],
> >          "teardown": [
> > -            "$TC qdisc del dev $DUMMY root handle 1:"
> > +            "$TC qdisc del dev $DUMMY root handle 1:",
> > +            "$IP link del dev $DUMMY"
> > +        ]
> > +    },
> > +    {
> > +        "id": "6a06",
> > +        "name": "Reject TEQL attach from a different network namespace",
>
> [ ... ]
>
> > +        "setup": [],
> > +        "cmdUnderTest": "$TC qdisc add dev $DUMMY root handle 3: teql0",
> > +        "expExitCode": "2",
> > +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> > +        "matchPattern": "qdisc teql0 3: root",
> > +        "matchCount": "0",
>
> [Severity: Low]
> Can this case pass even when the namespace check is not what refused
> the attach?
>
> tc exits with 2 for any netlink error. The verify step only checks that
> no teql0 qdisc shows up in the dump. Case 124e has the same weakness:
> it checks only expExitCode "2" and matchJSON [].
>
> In teql_qdisc_init(), both rejections return the same errno. Only the
> extack text tells them apart:
>
> net/sched/sch_teql.c:teql_qdisc_init() {
>     ...
>         if (sch->parent != TC_H_ROOT) {
>                 NL_SET_ERR_MSG_MOD(extack, "teql can only be used as root");
>                 return -EOPNOTSUPP;
>         }
>
>         if (dev_net(dev) != dev_net(m->dev)) {
>                 NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
>                 return -EOPNOTSUPP;
>         }
>     ...
> }
>
> The tc-testing config sets CONFIG_NET_SCH_TEQL=m. Unlike the other
> sch_* modules, sch_teql.c has no MODULE_ALIAS_NET_SCH(). So the
> alias-based request_module() never autoloads it, and the lookup fails:
>
> net/sched/sch_api.c:qdisc_create() {
>     ...
>         ops = qdisc_lookup_ops(kind);
>         if (!ops) {
>                 err = -ENOENT;
>                 NL_SET_ERR_MSG(extack, "Specified qdisc kind is unknown");
>                 goto err_out;
>         }
>     ...
> }
>
> When that happens, tc still exits with 2 and nothing named teql0
> appears. Both 6a06 and 124e then pass, whether or not the kernel has
> either check. That includes a kernel without the namespace fix.
>
> The module is loaded only by try_modprobe() in tdc.sh. That function
> just prints "skipping" when modprobe fails. Running tdc.py directly,
> for example with -e 6a06 or -c teql, never loads it.
>
> The commit message says "only the root-only check can refuse it" and
> "only the namespace check can refuse that one". That only holds when
> sch_teql is loaded.
>
> Would it make sense for each case to match the specific extack
> message? Another option is to fail when the teql0 kind is unavailable.
>
> The positive teql cases would fail in the same setup, so the suite as a
> whole would still show that something is wrong.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[Simon Horman]

On Sat, Nov 07, 2026 at 05:15:49PM -0500, Jamal Hadi Salim wrote:
> On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found more severe
> > issue(s) in other patches of this series. This mail is only an FYI, in case
> > you want to address the nit picks below when you respin.
> >
> > Low priority / nit picks for your consideration:
> >
> > - [Low] The commit body describes the patch as a change relative to an
> >   unmerged v2 posting, not as a change to the tree.
> > - [Low] Both rejection cases check only that tc exits with 2 and that no
> >   matching qdisc is dumped.
> >
> 
> Again cosmetics - so far no reason to resend.
> 6a06 is added by this commit, not by a previous posting, if i resend i
> would reword to state that.
> The module point cannot cause a pass here - it would fail several other tests..

Reviewed-by: Simon Horman <horms@kernel.org>