Re: [PATCH net 3/3 repost2] net/sched: sch_teql: reject qdiscs from another network namespace

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The fix checks that a teql slave and its master are in the
  same netns only once, in teql_qdisc_init() (net/sched/sch_teql.c:211).
- [Low] tc-testing case 124e ('Try to add teql as a child qdisc',
  tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json) no…

--

net/sched: sch_teql: reject qdiscs from another network namespace

This patch rejects a teql qdisc in teql_qdisc_init() if its device is in a
different network namespace from the teql master. It also moves
skb_queue_head_init() ahead of all validation, so every failure path sees
an initialized queue.

Link: https://lore.kernel.org/netdev/20261022144830.67948-1-shawnq1@163.com/ (local)

[ ... ]

[Severity: Medium]
Is this same-netns rule only checked at attach time? The check compares
against the current netns of m->dev. But nothing seems to keep the teql
master in init_net, because teql_master_setup() never sets
dev->netns_immutable:

net/sched/sch_teql.c:teql_master_setup() {
    ...
	dev->flags		= IFF_NOARP;
	dev->hard_header_len	= LL_MAX_HEADER;
	netif_keep_dst(dev);
}

__dev_change_net_namespace() refuses a move only when netns_immutable is
set. So "ip link set teql0 netns X" would still succeed after slaves on
init_net devices have attached.

During the move, dev_shutdown() tears down only teql0's own qdiscs. The
slave qdiscs on the init_net devices stay linked in master->slaves.

After that, CAP_NET_ADMIN in netns X (possibly granted by an unprivileged
user namespace) can bring teql0 up. teql_master_xmit() would then send
that netns's packets through netdev_start_xmit() on the init_net slave
devices. __teql_resolve() would also create neighbour entries on those
init_net devices.

Moving teql0 needs CAP_NET_ADMIN in init_net, so the unprivileged
reproducer in the commit message can't reach this path.

The commit message says:

    The teql master devices are created once in the initial network
    namespace and their Qdisc_ops are global.

They can still be moved to another netns after creation, though. Bonding
handles the same case in bond_setup():

	/* Don't allow bond devices to change network namespaces. */
	bond_dev->netns_immutable = true;

Should teql_master_setup() also set dev->netns_immutable = true, so that
this attach-time check is enough?

 	if (sch->parent != TC_H_ROOT) {
 		NL_SET_ERR_MSG_MOD(extack, "teql can only be used as root");
 		return -EOPNOTSUPP;

[Severity: Low]
Does the new check now hide the root-only check from tc-testing case 124e
("Try to add teql as a child qdisc") in
tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json?

That case requires nsPlugin, which moves $DUMMY into the test netns and
runs every command under "ip netns exec". The teql0 master is registered
in init_net.

For 124e, the hard_header_len check and the -ELOOP check both pass. The
new netns check then fires before the sch->parent != TC_H_ROOT check.
Both checks return -EOPNOTSUPP, so tc exits with 2 either way. The test
only checks expExitCode 2 and an empty child list.

As a result, 124e would keep passing even if the root-only rejection were
broken. No tdc case covers the new cross-netns rejection either, since the
other teql cases run without nsPlugin.

Would it make sense to put the netns check after the parent check, run
124e in init_net, or match the extack string? A separate test case for
the cross-netns rejection might also help.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050%40mojatatu.com