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

On Tue, Sep 8, 2026 at 6:58 PM [off-list ref] wrote:

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().

quoted

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 9ad63f1af37a6..051d7fd689029 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c

[ ... ]

quoted

@@ -895,20 +900,22 @@ static void ip_tunnel_update(struct ip_tunnel_net *itn,
      netdev_state_change(dev);
 }

-static void __ip_tunnel_dellink(struct net_device *dev, struct list_head *head)
+static void __ip_tunnel_dellink(struct net *net, struct net_device *dev,
+                             struct list_head *head)
 {
      struct ip_tunnel *tunnel = netdev_priv(dev);
      struct ip_tunnel_net *itn;

      itn = net_generic(tunnel->net, tunnel->ip_tnl_net_id);
      ip_tunnel_del(itn, tunnel);
-     unregister_netdevice_queue(dev, head);
+     unregister_netdevice_queue_net(net, dev, head);
 }

 int ip_tunnel_ctl(struct net_device *dev, struct ip_tunnel_parm_kern *p,
                int cmd)
 {
      struct ip_tunnel *t = netdev_priv(dev);
+     struct net *orig_net = dev_net(dev);
      struct ip_tunnel_net *itn;
      LIST_HEAD(dev_kill_list);
      struct net *net = t->net;

@@ -979,7 +986,7 @@ int ip_tunnel_ctl(struct net_device *dev, struct ip_tunnel_parm_kern *p,
                      }
              }

-             if (t) {
+             if (t && !ip_tunnel_unregistering(t)) {
                      err = 0;
                      ip_tunnel_update(itn, t, dev, p, true, 0);
              } else {

[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?

No, ip_tunnel_find() is called under the mutex, so
"t && ip_tunnel_unregistering(t)" never happens.

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?

Good point, ip_tunnel_ctl() needs to check !itn->fb_tunnel_dev
or maybe_get_net().

---
pw-bot: cr

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)?

[ ... ]

quoted

@@ -1194,7 +1204,7 @@ void ip_tunnel_delete_net(struct net *net, unsigned int id,
              struct ip_tunnel *t;

              hlist_for_each_entry_safe(t, n, thead, hash_node)
-                     __ip_tunnel_dellink(t->dev, head);
+                     __ip_tunnel_dellink(net, t->dev, head);
      }

      mutex_unlock(&itn->tunnels_lock);

[ ... ]

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