On Thu, Feb 13, 2025 at 7:00 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote: > > From: Xiao Liang <shaw.leon@xxxxxxxxx> > Date: Thu, 13 Feb 2025 17:55:32 +0800 > > On Thu, Feb 13, 2025 at 4:37 PM Xiao Liang <shaw.leon@xxxxxxxxx> wrote: > > > > > > On Thu, Feb 13, 2025 at 3:05 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> 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? Thanks.
