From: Xiao Liang shaw.leon@gmail.com Date: Fri, 14 Feb 2025 17:22:28 +0800
On Thu, Feb 13, 2025 at 7:00 PM Kuniyuki Iwashima kuniyu@amazon.com wrote:
From: Xiao Liang shaw.leon@gmail.com Date: Thu, 13 Feb 2025 17:55:32 +0800
On Thu, Feb 13, 2025 at 4:37 PM Xiao Liang shaw.leon@gmail.com wrote:
On Thu, Feb 13, 2025 at 3:05 PM Kuniyuki Iwashima kuniyu@amazon.com wrote:
[...]
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 863852abe8ea..108600dc716f 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -1498,7 +1498,8 @@ static int ip6gre_tunnel_init_common(struct net_device *dev) tunnel = netdev_priv(dev);
tunnel->dev = dev;
tunnel->net = dev_net(dev);
if (!tunnel->net)tunnel->net = dev_net(dev);Same question as patch 5 for here and other parts. Do we need this check and assignment ?
ip6gre_newlink_common -> nt->net = dev_net(dev) -> register_netdevice -> ndo_init / ip6gre_tunnel_init() -> ip6gre_tunnel_init_common -> tunnel->net = dev_net(dev)
Will remove this line.
However, fb tunnel of ip6_tunnel, ip6_vti and sit can have tunnel->net == NULL here. Take ip6_tunnel for example:
ip6_tnl_init_net() -> ip6_fb_tnl_dev_init() -> register_netdev() -> register_netdevice() -> ip6_tnl_dev_init()
This code path (including ip6_fb_tnl_dev_init()) doesn't set tunnel->net. But for ip6_gre, ip6gre_fb_tunnel_init() does.
Ah, okay. Then, let's set net in a single place, which would be better than spreading net assignment and adding null check in ->ndo_init(), and maybe apply the same to IPv4 tunnels ?
Tunnels are created in three ways: a) rtnetlink newlink, b) ioctl SIOCADDTUNNEL and c) during per netns init (fb). The code paths don't have much in common, and refactoring to set net in a single place is somewhat beyond the scope of this series. But for now I think we could put a general rule: net should be set prior to register_netdevice().
For IPv4 tunnels, tunnel->net of a) is set in ip_tunnel_newlink(). b) and c) are set in __ip_tunnel_create(): ip_tunnel_init_net() -> __ip_tunnel_create() ip_tunnel_ctl() -> ip_tunnel_create() -> __ip_tunnel_create() So net has already been initialized when register_netdevice() is called.
But it varies for IPv6 tunnels. Some set net for a) or c) while some don't. This patch has "fixed" for a). As for c) we can adopt the way of ip6_gre - setting net in *_fb_tunnel_init(), then remove the check in ndo_init().
Is it reasonable?
Yes, fair enough.