| Message ID | 1380643500-5018-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Tue, 1 Oct 2013 18:04:59 +0200 > rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param via > rtnl"), but I forget to assign rtnl ops to fb tunnels. > > Now that it is done, we must remove the explicit call to > unregister_netdevice_queue(), because the fallback tunnel is added to the queue > in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices (this > is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Applied and queued up for -stable. But I imagine since the x-netns changes aren't in various -stable branches this will need to be adjusted a bit? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 01/10/2013 18:59, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Tue, 1 Oct 2013 18:04:59 +0200 > >> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param via >> rtnl"), but I forget to assign rtnl ops to fb tunnels. >> >> Now that it is done, we must remove the explicit call to >> unregister_netdevice_queue(), because the fallback tunnel is added to the queue >> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices (this >> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > Applied and queued up for -stable. > > But I imagine since the x-netns changes aren't in various -stable > branches this will need to be adjusted a bit? Yes, it's what I've tried to say in the commit log ;-) In fact, before the x-netns changes, we must keep the unregister_netdevice_queue() line. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 02/10/2013 09:15, Nicolas Dichtel a écrit : > Le 01/10/2013 18:59, David Miller a écrit : >> From: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> Date: Tue, 1 Oct 2013 18:04:59 +0200 >> >>> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param via >>> rtnl"), but I forget to assign rtnl ops to fb tunnels. >>> >>> Now that it is done, we must remove the explicit call to >>> unregister_netdevice_queue(), because the fallback tunnel is added to the queue >>> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices (this >>> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >>> >>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> >> Applied and queued up for -stable. Another things about ipip: between 0974658da47c ("ipip: advertise tunnel param via rtnl", v3.8) and fd58156e456d ("IPIP: Use ip-tunneling code.", v3.10) the fb device of ipip module has the same problem. Should I send a patch? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Wed, 02 Oct 2013 09:36:02 +0200 > Le 02/10/2013 09:15, Nicolas Dichtel a écrit : >> Le 01/10/2013 18:59, David Miller a écrit : >>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>> Date: Tue, 1 Oct 2013 18:04:59 +0200 >>> >>>> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel >>>> param via >>>> rtnl"), but I forget to assign rtnl ops to fb tunnels. >>>> >>>> Now that it is done, we must remove the explicit call to >>>> unregister_netdevice_queue(), because the fallback tunnel is added to >>>> the queue >>>> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices >>>> (this >>>> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >>>> >>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>> >>> Applied and queued up for -stable. > Another things about ipip: between 0974658da47c ("ipip: advertise > tunnel param > via rtnl", v3.8) and fd58156e456d ("IPIP: Use ip-tunneling code.", > v3.10) the > fb device of ipip module has the same problem. > Should I send a patch? Yes please do, thanks for noticing this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 02/10/2013 23:08, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Wed, 02 Oct 2013 09:36:02 +0200 > >> Le 02/10/2013 09:15, Nicolas Dichtel a écrit : >>> Le 01/10/2013 18:59, David Miller a écrit : >>>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>>> Date: Tue, 1 Oct 2013 18:04:59 +0200 >>>> >>>>> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel >>>>> param via >>>>> rtnl"), but I forget to assign rtnl ops to fb tunnels. >>>>> >>>>> Now that it is done, we must remove the explicit call to >>>>> unregister_netdevice_queue(), because the fallback tunnel is added to >>>>> the queue >>>>> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices >>>>> (this >>>>> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >>>>> >>>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>>> >>>> Applied and queued up for -stable. >> Another things about ipip: between 0974658da47c ("ipip: advertise >> tunnel param >> via rtnl", v3.8) and fd58156e456d ("IPIP: Use ip-tunneling code.", >> v3.10) the >> fb device of ipip module has the same problem. >> Should I send a patch? > > Yes please do, thanks for noticing this. > In fact, I just notice that 3.9 branch is EoL (bug is only in 3.8 and 3.9). Should I still send a patch ? If yes, based on which tree/branch? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Thu, 03 Oct 2013 10:06:24 +0200 > In fact, I just notice that 3.9 branch is EoL (bug is only in 3.8 and > 3.9). > Should I still send a patch ? If yes, based on which tree/branch? No stable backports are needed then. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 2, 2013 at 3:15 AM, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > Le 01/10/2013 18:59, David Miller a écrit : > >> From: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> Date: Tue, 1 Oct 2013 18:04:59 +0200 >> >>> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param >>> via >>> rtnl"), but I forget to assign rtnl ops to fb tunnels. >>> >>> Now that it is done, we must remove the explicit call to >>> unregister_netdevice_queue(), because the fallback tunnel is added to >>> the queue >>> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices >>> (this >>> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >>> >>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> >> >> Applied and queued up for -stable. >> >> But I imagine since the x-netns changes aren't in various -stable >> branches this will need to be adjusted a bit? > > Yes, it's what I've tried to say in the commit log ;-) > > In fact, before the x-netns changes, we must keep the > unregister_netdevice_queue() line. In 3.11 linux-stable, this patch was merged between 3.11.4 and 3.11.5 in commit 3783100, after the x-netns changes in commit 5e6700b3bf, but the unregister_netdevice_queue was kept. I think that caused the following bug. In 3.11.6, a simple `modprobe sit && rmmod sit` hits the BUG at net/core/dev.c:5039: BUG_ON(dev->reg_state != NETREG_REGISTERED); The device is actually NETREG_RELEASED at one point because the device is now unregistered twice. The issue goes away by porting the remainder of the original commit: the one liner that removes the call to unregister_netdevice_queue. +++ b/net/ipv6/sit.c @@ -1708,7 +1708,6 @@ static void __net_exit sit_exit_net(struct net *net) sit_destroy_tunnels(sitn, &list); - unregister_netdevice_queue(sitn->fb_tunnel_dev, &list); unregister_netdevice_many(&list); If correct, let me know if I should send a proper one-line patch against 3.11.y. Since I haven't looked at this code before, I found it safer to report the issue first. 5e6700b3bf was not applied to 3.10 stable, so that branch is not affected. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 22/10/2013 01:30, Willem de Bruijn a écrit : > On Wed, Oct 2, 2013 at 3:15 AM, Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: >> Le 01/10/2013 18:59, David Miller a écrit : >> >>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>> Date: Tue, 1 Oct 2013 18:04:59 +0200 >>> >>>> rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param >>>> via >>>> rtnl"), but I forget to assign rtnl ops to fb tunnels. >>>> >>>> Now that it is done, we must remove the explicit call to >>>> unregister_netdevice_queue(), because the fallback tunnel is added to >>>> the queue >>>> in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices >>>> (this >>>> is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). >>>> >>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >>> >>> >>> Applied and queued up for -stable. >>> >>> But I imagine since the x-netns changes aren't in various -stable >>> branches this will need to be adjusted a bit? >> >> Yes, it's what I've tried to say in the commit log ;-) >> >> In fact, before the x-netns changes, we must keep the >> unregister_netdevice_queue() line. > > In 3.11 linux-stable, this patch was merged between 3.11.4 and 3.11.5 > in commit 3783100, after the x-netns changes in commit 5e6700b3bf, but > the unregister_netdevice_queue was kept. > > I think that caused the following bug. In 3.11.6, a simple `modprobe > sit && rmmod sit` hits the BUG at net/core/dev.c:5039: > > BUG_ON(dev->reg_state != NETREG_REGISTERED); > > The device is actually NETREG_RELEASED at one point because the device > is now unregistered twice. The issue goes away by porting the > remainder of the original commit: the one liner that removes the call > to unregister_netdevice_queue. > > +++ b/net/ipv6/sit.c > @@ -1708,7 +1708,6 @@ static void __net_exit sit_exit_net(struct net *net) > > sit_destroy_tunnels(sitn, &list); > - unregister_netdevice_queue(sitn->fb_tunnel_dev, &list); > unregister_netdevice_many(&list); > > If correct, let me know if I should send a proper one-line patch > against 3.11.y. Since I haven't looked at this code before, I found it > safer to report the issue first. Yes, this line should be removed, like it was done in the original patch (x-netns for sit is part of 3.11). > > 5e6700b3bf was not applied to 3.10 stable, so that branch is not affected. Right. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index afd5605aea7c..19269453a8ea 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -1666,6 +1666,7 @@ static int __net_init sit_init_net(struct net *net) goto err_alloc_dev; } dev_net_set(sitn->fb_tunnel_dev, net); + sitn->fb_tunnel_dev->rtnl_link_ops = &sit_link_ops; /* FB netdevice is special: we have one, and only one per netns. * Allowing to move it to another netns is clearly unsafe. */ @@ -1700,7 +1701,6 @@ static void __net_exit sit_exit_net(struct net *net) rtnl_lock(); sit_destroy_tunnels(sitn, &list); - unregister_netdevice_queue(sitn->fb_tunnel_dev, &list); unregister_netdevice_many(&list); rtnl_unlock(); }
rtnl ops where introduced by ba3e3f50a0e5 ("sit: advertise tunnel param via rtnl"), but I forget to assign rtnl ops to fb tunnels. Now that it is done, we must remove the explicit call to unregister_netdevice_queue(), because the fallback tunnel is added to the queue in sit_destroy_tunnels() when checking rtnl_link_ops of all netdevices (this is valid since commit 5e6700b3bf98 ("sit: add support of x-netns")). Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/ipv6/sit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)