Re: [PATCH v2 net-next 7/7] ip_tunnel: Support per-netns device unregistration.

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

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

- [High] In `ip_tunnel_ctl()` (net/ipv4/ip_tunnel.c:930-933) the newly
  added gate `if (!itn->fb_tunnel_dev) { err = -EBUSY; goto done; }`…

--

ip_tunnel: Support per-netns device unregistration.

This switches __ip_tunnel_dellink() to unregister_netdevice_queue_net() so
ip_tunnel_delete_net() can queue cross-netns devices for per-netns
unregistration, and adds ip_tunnel_unregistering() checks so
ip_tunnel_changelink()/ip_tunnel_dellink()/ip_tunnel_ctl() skip devices
that are already queued.  It also adds a NULL check on itn->fb_tunnel_dev
in ip_tunnel_ctl().

[ ... ]

[Severity: High]
Is itn->fb_tunnel_dev == NULL a reliable indicator that the netns is
dying?

ip_tunnel_init_net() leaves fb_tunnel_dev permanently NULL for a perfectly
live netns:

net/ipv4/ip_tunnel.c:ip_tunnel_init_net() {
	...
	if (!ops || !net_has_fallback_tunnels(net)) {
		struct ip_tunnel_net *it_init_net;

		it_init_net = net_generic(&init_net, ip_tnl_net_id);
		itn->type = it_init_net->type;
		itn->fb_tunnel_dev = NULL;
		return 0;
	}
	...
}

net_has_fallback_tunnels() returns false for all non-init netns when
net.core.fb_tunnels_only_for_init_net == 1, and for every netns including
init_net when it is 2 (or the fb_tunnels=none boot arg is used).  That
sysctl is mode 0644 in net/core/sysctl_net_core.c, so it is runtime
settable.

In those configurations, does this new gate make all four legacy ioctls
fail with -EBUSY on ipip/gre/gretap/erspan/vti devices?  On the parent
commit, with fb_tunnel_dev == NULL:

  - SIOCGETTUNNEL skips the "dev == itn->fb_tunnel_dev" branch and does
    memcpy(p, &t->parms, sizeof(*p)) with t = netdev_priv(dev)
  - SIOCADDTUNNEL reaches ip_tunnel_create(net, itn, p), which only needs
    itn->rtnl_link_ops (assigned before the early return above)
  - SIOCCHGTUNNEL takes the "dev != itn->fb_tunnel_dev" branch
  - SIOCDELTUNNEL reaches __ip_tunnel_dellink()

The path is reachable per device without any fallback device existing,
since ipgre_netdev_ops, ipip_netdev_ops and vti_netdev_ops all install
.ndo_siocdevprivate = ip_tunnel_siocdevprivate:

ioctl(fd, SIOCGETTUNNEL, {ifr_name="gre1"})
  -> ip_tunnel_siocdevprivate()
    -> dev->netdev_ops->ndo_tunnel_ctl(dev, &p, cmd)
      -> ip_tunnel_ctl()

So does "ip tunnel show/add/change/del" on a netlink-created device stop
working after this change when fallback tunnels are disabled?  The
SIOCGETTUNNEL branch has no capability check, so this is reachable
unprivileged as well.

Is the gate needed for NULL-safety at all?  Every remaining dereference
of itn->fb_tunnel_dev inside ip_tunnel_ctl() is guarded by a
"dev == itn->fb_tunnel_dev" comparison, so its only role appears to be the
dying-netns policy described in the commit message.

Would an explicit "itn is being destroyed" flag (or the
ip_tunnel_unregistering() semantics added by this same patch) work better
here, given that fb_tunnel_dev == NULL cannot distinguish "this netns
never had a fallback device" from "this netns is going away"?

Other in-tree code already treats a NULL fb_tunnel_dev as a normal state
for a healthy netns:

net/ipv4/ip_vti.c:vti_init_net() {
	...
	itn = net_generic(net, vti_net_id);
	if (itn->fb_tunnel_dev)
		vti_fb_tunnel_init(itn->fb_tunnel_dev);
	...
}

 	switch (cmd) {
 	case SIOCGETTUNNEL:
 		if (dev == itn->fb_tunnel_dev) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909234422.2416506-1-kuniyu%40google.com