Re: [PATCH v2 net-next 6/7] ip_tunnel: Protect ip_tunnel_net.tunnels[] with mutex.

[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] Reported concern (judged a false positive after verification):
  under the new mutex the check-then-act sequences in `ip_tunnel_ctl()`…

--

ip_tunnel: Protect ip_tunnel_net.tunnels[] with mutex.

This adds a per-netns mutex to struct ip_tunnel_net and takes it around the
paths that add, remove, look up or update entries in
ip_tunnel_net.tunnels[], replacing the implicit RTNL protection assumed by
ip_tunnel_find().  A DEBUG_NET_WARN_ON_ONCE(netdev_need_ops_lock(dev)) is
also added to ip_tunnel_ctl() to document that the ops lock must be a no-op
for tunnel devices.

No Link: tags are present in the commit header.

I have no regressions to report against this commit.  Quoting the hunks I
looked at most closely, with a note on the one candidate issue I chased and
why it does not look reachable here.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 3ba03c2b3b900..9ad63f1af37a6 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c

[ ... ]

> @@ -988,7 +1003,7 @@ int ip_tunnel_ctl(struct net_device *dev, struct ip_tunnel_parm_kern *p,
>  			dev = t->dev;
>  		}
>  
> -		ip_tunnel_dellink(dev, &dev_kill_list);
> +		__ip_tunnel_dellink(dev, &dev_kill_list);
>  		err = 0;
>  		break;

[ ... ]

> @@ -1164,6 +1184,8 @@ void ip_tunnel_delete_net(struct net *net, unsigned int id,
>  
>  	ASSERT_RTNL_NET(net);
>  
> +	mutex_lock(&itn->tunnels_lock);
> +
>  	WRITE_ONCE(itn->fb_tunnel_dev, NULL);
>  
>  	for (h = 0; h < IP_TNL_HASH_SIZE; h++) {
> @@ -1172,8 +1194,10 @@ 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(t->dev, head);
>  	}
> +
> +	mutex_unlock(&itn->tunnels_lock);
>  }
>  EXPORT_SYMBOL_GPL(ip_tunnel_delete_net);

The one thing I checked was whether a tunnel already unhashed and queued for
unregistration by ip_tunnel_delete_net() above could come back, either via
ip_tunnel_update() in ip_tunnel_changelink()/ip_tunnel_ctl() (ip_tunnel_del()
becomes a no-op, then ip_tunnel_add() relinks it) or via a second
__ip_tunnel_dellink() queueing it onto another kill list.

That does not look reachable at this commit: rtnl_net_lock() still takes the
global RTNL, and the pernet exit path runs ip_tunnel_delete_net() plus the
queued unregister_netdevice_many*() inside one RTNL section, so the ioctl and
rtnetlink paths cannot interleave with teardown yet.  The later patch in the
series adding ip_tunnel_unregistering() covers those call sites anyway.

> @@ -1276,6 +1306,8 @@ int ip_tunnel_changelink(struct net_device *dev, struct nlattr *tb[],
>  
>  	ip_tunnel_update(itn, t, dev, p, !tb[IFLA_MTU], fwmark);
>  out:
> +	mutex_unlock(&itn->tunnels_lock);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(ip_tunnel_changelink);

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