Re: [PATCH net v5] net: qualcomm: rmnet: require CAP_NET_ADMIN in the real device netns for config ops

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

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

- [Medium] Wrong device passed to the netns gate on the ndo_del_slave
  path.
- [Low] Missing Fixes: tag for the third fixed site.

--

net: qualcomm: rmnet: require CAP_NET_ADMIN in the real device netns for config ops

This adds capability checks in the real device's netns to three rmnet
config paths: rmnet_changelink() and the bridge add/del paths get
rtnl_dev_link_net_capable(), and rmnet_set_coalesce() gets an explicit
ns_capable() test.  Rejections are reported through extack where one is
available.

A couple of questions below.

Fixes: 2abb5792387e ("net: qualcomm: rmnet: Allow configuration updates to existing devices")
Fixes: 60d58f971c1077 ("net: qualcomm: rmnet: Implement bridge mode")
Cc: stable@vger.kernel.org

[Severity: Low]
The commit message enumerates three defective config paths, and the v5
changelog says the rmnet_set_coalesce() gate was added in this revision,
but only two Fixes: tags are present.

rmnet_set_coalesce() and the ETHTOOL_COALESCE_TX_AGGR interface it uses
came in with db8a563a9d9024 ("net: qualcomm: rmnet: add ethtool support
for configuring tx aggregation") in v6.3, so neither tag covers the
rmnet_vnd.c hunk.

With Cc: stable present, the oldest tag (60d58f971c1077, 2017) points
stable tooling at branches where rmnet_set_coalesce() does not exist and
the rmnet_vnd.c hunk cannot apply.  Would it make sense to add:

  Fixes: db8a563a9d9024 ("net: qualcomm: rmnet: add ethtool support for configuring tx aggregation")

[ ... ]

[Severity: Medium]
Should the first argument here be slave_dev rather than rmnet_dev?

rtnl_dev_link_net_capable() short-circuits when the device passed as dev
already lives in link_net:

net/core/rtnetlink.c:rtnl_dev_link_net_capable() {
	return net_eq(link_net, dev_net(dev)) ||
	       ns_capable(link_net->user_ns, CAP_NET_ADMIN);
}

so the dev argument has to be the device whose netns the rtnl core
already required CAP_NET_ADMIN for.  For ndo_add_slave/ndo_del_slave that
is the RTM_SETLINK target, i.e. slave_dev, not the master:

net/core/rtnetlink.c:do_set_master() {
	struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
	...
		err = ops->ndo_del_slave(upper_dev, dev);
	...
		upper_dev = __dev_get_by_index(dev_net(dev), ifindex);
		...
		err = ops->ndo_add_slave(upper_dev, dev, extack);
}

On the add path the master is re-resolved with
__dev_get_by_index(dev_net(dev), ifindex), so dev_net(rmnet_dev) equals
dev_net(slave_dev) and the two spellings happen to agree.  On the del path
upper_dev comes from the existing adjacency with no netns re-check, so
rmnet_dev can be in a different netns than slave_dev.

If rmnet_dev sits in real_dev's netns while slave_dev lives elsewhere,
does net_eq(dev_net(rmnet_dev), dev_net(real_dev)) make the new gate pass
without checking any capability, even though the caller was only
authorized against dev_net(slave_dev)?  rmnet_del_bridge() then goes on to
clear real_port->bridge_ep and real_port->rmnet_mode via
rmnet_unregister_bridge() and unregisters the slave port.

That topology looks constructible: rmnet_vnd_setup() does not set
netns_immutable, __dev_change_net_namespace() has no upper/lower device
restriction and preserves adjacencies, and rmnet_config_notify_cb() only
acts when rmnet_is_real_dev_registered(dev), so moving the rmnet device
into real_dev's netns leaves the master/slave link intact.

Passing slave_dev on both paths would keep the two sites consistent and
match what the core actually authorized.

[ ... ]

This is the hunk referred to in the Fixes: tag question above.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906123212.10829-1-suruurism%40gmail.com