Re: [PATCH v2 net-next 2/7] ip_tunnel: Set itn->fb_tunnel_dev to NULL in ip_tunnel_delete_net().

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Commit message omits two behavioural changes that the diff
  makes to net/ipv4/ip_tunnel.c.

--

ip_tunnel: Set itn->fb_tunnel_dev to NULL in ip_tunnel_delete_net().

This drops the for_each_netdev_safe() loop from ip_tunnel_delete_net() and
instead clears itn->fb_tunnel_dev there, so the unconditional hash walk can
queue every tunnel, including the fallback device.  The matching
fb_tunnel_dev clearing in ip_tunnel_uninit() is removed.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..d29e2ba7cdd3a 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -1155,26 +1155,19 @@ void ip_tunnel_delete_net(struct net *net, unsigned int id,
>  			  struct list_head *head)
>  {
>  	struct ip_tunnel_net *itn = net_generic(net, id);
> -	struct net_device *dev, *aux;
>  	int h;
>  
>  	ASSERT_RTNL_NET(net);
>  
> -	for_each_netdev_safe(net, dev, aux)
> -		if (dev->rtnl_link_ops == ops)
> -			unregister_netdevice_queue(dev, head);
> +	WRITE_ONCE(itn->fb_tunnel_dev, NULL);

[Severity: Medium]
Could the changelog say something about the devices that only the removed
for_each_netdev_safe() loop covered?

The loop walked devices by residence (dev_net(dev) == net), while the hash
walk below covers devices by tunnel->net, i.e. the netns the ip_tunnel is
hashed in.  Those two differ when IFLA_LINK_NETNSID is used, because
ipgre_newlink() and friends pass params->link_net into ip_tunnel_newlink():

    ip -n ns1 link add gre1 link-netns ns2 type gre local X remote Y
    ip netns del ns1

Here gre1 lives in ns1 but is hashed in ns2's itn, so after this change
ip_tunnel_delete_net(ns1) does not see it and its removal is deferred to
net/core/dev.c:default_device_exit_batch():

    if (dev->rtnl_link_ops && dev->rtnl_link_ops->dellink)
            dev->rtnl_link_ops->dellink(dev, &dev_kill_list);

I traced that path and the device does get unregistered there (ipgre_link_ops
does not set netns_refund, so default_device_exit_net() leaves it alone and
the second loop calls ->dellink()), so this looks like a teardown ordering
change rather than a leak.

But it is the same path the changelog describes as unusable:

  "This also makes default_device_exit_batch() impossible to remove
   the device since it calls ->dellink()."

Since itn->fb_tunnel_dev is NULL by then, ip_tunnel_dellink() no longer skips
the device, so the statement holds only for the fallback device before the
netns is dying.  Would it be worth spelling out that teardown of tunnels
resident in the dying netns but hashed in another netns' itn now relies on
default_device_exit_batch()?

>  	for (h = 0; h < IP_TNL_HASH_SIZE; h++) {
> -		struct ip_tunnel *t;
> -		struct hlist_node *n;
>  		struct hlist_head *thead = &itn->tunnels[h];
> +		struct hlist_node *n;
> +		struct ip_tunnel *t;
>  
>  		hlist_for_each_entry_safe(t, n, thead, hash_node)
> -			/* If dev is in the same netns, it has already
> -			 * been added to the list by the previous loop.
> -			 */
> -			if (!net_eq(dev_net(t->dev), net))
> -				unregister_netdevice_queue(t->dev, head);
> +			unregister_netdevice_queue(t->dev, head);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(ip_tunnel_delete_net);
> @@ -1310,8 +1303,6 @@ void ip_tunnel_uninit(struct net_device *dev)
>  
>  	itn = net_generic(net, tunnel->ip_tnl_net_id);
>  	ip_tunnel_del(itn, netdev_priv(dev));
> -	if (itn->fb_tunnel_dev == dev)
> -		WRITE_ONCE(itn->fb_tunnel_dev, NULL);
>  
>  	dst_cache_reset(&tunnel->dst_cache);
>  }

[Severity: Medium]
This hunk partially reverts ba61539c6ae5 ("ip_tunnel: fix use-after-free in
ip_tunnel_lookup()"), and the changelog does not mention it.  Should the
reason the guard is now redundant be recorded somewhere?

As far as I can tell the new code depends on the invariant that a fallback
device can only ever be unregistered from ip_tunnel_delete_net():

  - ip_tunnel_dellink() skips it while itn->fb_tunnel_dev is set
  - ip_tunnel_ctl() refuses SIOCDELTUNNEL on it with -EPERM
  - ip_tunnel_init_net() sets netns_immutable = true, so it cannot move

That invariant does hold at this commit, so I did not find a live
use-after-free.  Two related points though:

net/ipv6/ip6_gre.c and net/ipv6/sit.c still clear fb_tunnel_dev from their
uninit paths, so the two families now differ without an explanation.

And ip_tunnel_init_net() adds the fallback device to itn->tunnels:

    ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));

so the hash walk in ip_tunnel_delete_net() queues it directly anyway.  Is the
statement

  "So, ip_tunnel_delete_net() has to iterate devices in the dying netns
   and call unregister_netdevice_queue() directly."

still accurate as a description of what the fallback device needs, given the
device is reachable through the hash?

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