Re: [PATCH v1 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: 0 · Low: 1

- [Low] Claim (INVALIDATED by verification): 'ip_tunnel_delete_net() no
  longer uses its struct rtnl_link_ops *ops argument yet the prototype…

--

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

This clears itn->fb_tunnel_dev at the start of ip_tunnel_delete_net() so
that the fallback device is treated like any other tunnel device, drops
the for_each_netdev_safe() scan that matched dev->rtnl_link_ops == ops,
and removes the now redundant fb_tunnel_dev clearing from
ip_tunnel_uninit().

No Link: tags present in the commit header.

> 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: Low]
With the for_each_netdev_safe() scan gone, is the ops argument of
ip_tunnel_delete_net() still used anywhere in the function?

The prototype in include/net/ip_tunnels.h and all five callers
(ipgre_exit_rtnl_net(), ipgre_tap_exit_rtnl_net(),
erspan_exit_rtnl_net(), vti_exit_rtnl_net(), ipip_exit_rtnl_net()) still
pass their link ops:

	void ip_tunnel_delete_net(struct net *net, unsigned int id,
				  struct rtnl_link_ops *ops,
				  struct list_head *dev_to_kill);

The follow-up patch in this series, "ip_tunnel: Don't pass rtnl_link_ops
to ip_tunnel_delete_net().", removes the argument from the declaration
and from all five call sites, so the unused parameter only exists for one
commit and does not produce a compiler warning. Would it be simpler to
fold that removal into this patch, or to note in the changelog that the
argument becomes unused here?

A second, related observation about coverage: the removed loop was the
only place that picked up devices by netns membership, so after this
change ip_tunnel_delete_net(net) only walks the itn hashes of net. A
device that lives in the dying netns but is hashed in another netns' itn
(created with link-netns, e.g. "ip -n nsB link add gre1 link-netns nsA
type gre ..." followed by "ip netns del nsB") is no longer queued here.

Tracing that case, it still gets destroyed by default_device_exit_batch():

net/core/dev.c:default_device_exit_batch() {
	...
	for_each_netdev_reverse(net, dev) {
		if (dev->rtnl_link_ops && dev->rtnl_link_ops->dellink)
			dev->rtnl_link_ops->dellink(dev, &dev_kill_list);
		else
			unregister_netdevice_queue(dev, &dev_kill_list);
	}
	...
}

which reaches ip_tunnel_dellink() and, since fb_tunnel_dev != dev there,
does ip_tunnel_del() plus unregister. So nothing appears to be stranded.
Could the changelog mention this change in which devices
ip_tunnel_delete_net() queues, since the last patch of the series
("ip_tunnel: Support per-netns device unregistration.") is where that new
behaviour is finally described?

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

[ ... ]

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