Re: [PATCH v1 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] ip_tunnel_ctl()'s SIOCADDTUNNEL arm can register a brand-new
  netdevice into, and re-hash a tunnel in, an ip_tunnel_net that…

--

ip_tunnel: Support per-netns device unregistration.

This switches __ip_tunnel_dellink() to unregister_netdevice_queue_net() so
ip_tunnel devices living in a different netns than the dying link_net are
queued per-netns, and adds an ip_tunnel_unregistering() guard to the
SIOCCHGTUNNEL and SIOCDELTUNNEL arms of ip_tunnel_ctl(), to
ip_tunnel_dellink() and to ip_tunnel_changelink().

[ ... ]

[Severity: High]
The SIOCCHGTUNNEL arm now consults ip_tunnel_unregistering(), and so do
SIOCDELTUNNEL, ip_tunnel_dellink() and ip_tunnel_changelink().  Should the
creation arm just above in ip_tunnel_ctl() get an equivalent check?  It is
the only path that mutates itn and registers a device with no liveness test:

net/ipv4/ip_tunnel.c:ip_tunnel_ctl() {
	...
	itn = net_generic(net, t->ip_tnl_net_id);   /* net == t->net */
	...
		t = ip_tunnel_find(itn, p, itn->type);

		if (cmd == SIOCADDTUNNEL) {
			if (!t) {
				t = ip_tunnel_create(net, itn, p);
	...
}

ip_tunnel_create() -> __ip_tunnel_create() does dev_net_set(dev, net) plus
register_netdevice(), then ip_tunnel_add(itn, nt) re-populates the hash.

Can that happen after ip_tunnel_delete_net() has already made its final
pass over the same itn?  With CONFIG_DEBUG_NET_SMALL_RTNL=y, this is the
sequence the commit message itself demonstrates with gre1:

cleanup_net()
  ops_undo_list() -> ipgre_exit_rtnl(ns2)
    ip_tunnel_delete_net(ns2)
      WRITE_ONCE(itn->fb_tunnel_dev, NULL);
      __ip_tunnel_dellink(ns2, gre1, head)
        unregister_netdevice_queue_net(ns2, gre1, head)

Since dev_net(gre1) is ns1 and not ns2, unregister_netdevice_queue_net()
only queues the device:

net/core/dev.c:unregister_netdevice_queue_net() {
	...
	list_add_tail(&dev->unreg_list_net, &net->dev_unreg_head);
	rtnl_net_queue_work(net);
	...
}

so gre1 stays registered after ops_exit_rtnl_list() drops RTNL, exactly as
the bpftrace output shows (gre1 is torn down later from
rtnl_net_work_func).  While it is still registered, an ioctl in ns1 can
reach ip_tunnel_ctl(gre1, p, SIOCADDTUNNEL) holding only
rtnl_net_lock(ns1):

dev_ioctl() -> dev_ifsioc() -> dev_siocdevprivate()
  -> ip_tunnel_siocdevprivate() -> ndo_tunnel_ctl -> ip_tunnel_ctl()

ip_tunnel_ctl() then uses net = t->net = ns2 and itn = net_generic(ns2, id),
the itn that was just drained.  ip_tunnel_find() returns NULL on the empty
hash, so a new device is registered into ns2 and hashed into that itn.

Does anything reclaim that device afterwards?  The last sweep of ns2's
device list is in default_device_exit_batch():

net/core/dev.c:default_device_exit_batch() {
	...
	unregister_netdevice_many(&dev_kill_list);
	rtnl_unlock();

	rtnl_net_flush_workqueue();
	...
}

The scan and rtnl_unlock() precede rtnl_net_flush_workqueue(), and gre1's
unregistration is still pending at that point, so an ioctl winning RTNL in
that gap can create a device in ns2 after the final sweep.  ops_free_list()
would then kfree() ns2's ip_tunnel_net while the new tunnel is hashed in
it, and cleanup_net() would free ns2's net->gen and struct net while a
registered netdevice still points at them through dev_net()/dev_list, since
netdevices hold no netns reference.

The per-tunnel guard added here, hlist_unhashed(&t->hash_node), cannot
cover a tunnel that does not exist yet.  Would a separate "this itn is
dying" test work here, for instance keying off the marker
ip_tunnel_delete_net() already publishes with
WRITE_ONCE(itn->fb_tunnel_dev, NULL)?

[ ... ]

[ ... ]

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