| Message ID | 20130925055519.GW7660@secunet.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert <steffen.klassert@secunet.com> wrote: > Currently we can not update the tunnel parameters of > the fallback tunnels because we don't find them in the > hash lists. Fix this by adding them on initialization. > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > --- > net/ipv4/ip_tunnel.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > index b8ce640..f2348f2 100644 > --- a/net/ipv4/ip_tunnel.c > +++ b/net/ipv4/ip_tunnel.c > @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id, > /* FB netdevice is special: we have one, and only one per netns. > * Allowing to move it to another netns is clearly unsafe. > */ > - if (!IS_ERR(itn->fb_tunnel_dev)) > + if (!IS_ERR(itn->fb_tunnel_dev)) { > itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL; > + ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev)); > + } > rtnl_unlock(); > fallback tunnel s not required to be in hash table, Its is returned if none of hashed tunnels are matched, ref ip_tunnel_lookup(). Can you post command to reproduce this issue? > return PTR_RET(itn->fb_tunnel_dev); > -- > 1.7.9.5 > > -- > 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 -- 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, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote: > On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert > <steffen.klassert@secunet.com> wrote: > > Currently we can not update the tunnel parameters of > > the fallback tunnels because we don't find them in the > > hash lists. Fix this by adding them on initialization. > > > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > > --- > > net/ipv4/ip_tunnel.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > > index b8ce640..f2348f2 100644 > > --- a/net/ipv4/ip_tunnel.c > > +++ b/net/ipv4/ip_tunnel.c > > @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id, > > /* FB netdevice is special: we have one, and only one per netns. > > * Allowing to move it to another netns is clearly unsafe. > > */ > > - if (!IS_ERR(itn->fb_tunnel_dev)) > > + if (!IS_ERR(itn->fb_tunnel_dev)) { > > itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL; > > + ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev)); > > + } > > rtnl_unlock(); > > > fallback tunnel s not required to be in hash table, Its is returned if > none of hashed tunnels are matched, ref ip_tunnel_lookup(). > Can you post command to reproduce this issue? > Something like ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1 worked until v3.9 and stopped working with v3.10. -- 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 Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert <steffen.klassert@secunet.com> wrote: > On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote: >> On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert >> <steffen.klassert@secunet.com> wrote: >> > Currently we can not update the tunnel parameters of >> > the fallback tunnels because we don't find them in the >> > hash lists. Fix this by adding them on initialization. >> > >> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> >> > --- >> > net/ipv4/ip_tunnel.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c >> > index b8ce640..f2348f2 100644 >> > --- a/net/ipv4/ip_tunnel.c >> > +++ b/net/ipv4/ip_tunnel.c >> > @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id, >> > /* FB netdevice is special: we have one, and only one per netns. >> > * Allowing to move it to another netns is clearly unsafe. >> > */ >> > - if (!IS_ERR(itn->fb_tunnel_dev)) >> > + if (!IS_ERR(itn->fb_tunnel_dev)) { >> > itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL; >> > + ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev)); >> > + } >> > rtnl_unlock(); >> > >> fallback tunnel s not required to be in hash table, Its is returned if >> none of hashed tunnels are matched, ref ip_tunnel_lookup(). >> Can you post command to reproduce this issue? >> > > Something like > > ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1 > > worked until v3.9 and stopped working with v3.10. OK, I see the bug, tunnel exact match lookup does not check fb tunnel. There are two options. 1. Fix ip_tunnel_find() to check for fb tunnel. 2. Add fb tunnel to hash table, which is what ur patch does. I think your patch is better solution as it get rid of special case. But patch is not complete. It needs to remove fb tunnel checks on netdev unregister. -- 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 Thu, Sep 26, 2013 at 11:24:07AM -0700, Pravin Shelar wrote: > On Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert > <steffen.klassert@secunet.com> wrote: > > On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote: > >> fallback tunnel s not required to be in hash table, Its is returned if > >> none of hashed tunnels are matched, ref ip_tunnel_lookup(). > >> Can you post command to reproduce this issue? > >> > > > > Something like > > > > ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1 > > > > worked until v3.9 and stopped working with v3.10. > > OK, I see the bug, tunnel exact match lookup does not check fb tunnel. > There are two options. > 1. Fix ip_tunnel_find() to check for fb tunnel. > 2. Add fb tunnel to hash table, which is what ur patch does. > I think your patch is better solution as it get rid of special case. > But patch is not complete. It needs to remove fb tunnel checks on > netdev unregister. It looks like this is another bug that requires an additional patch. We add the fallback tunnel to the unregister list when we iterate over all netdevices in the namespace at the beginning of ip_tunnel_destroy() and then again explicitly at the end of ip_tunnel_destroy(). -- 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 Fri, Sep 27, 2013 at 12:56 AM, Steffen Klassert <steffen.klassert@secunet.com> wrote: > On Thu, Sep 26, 2013 at 11:24:07AM -0700, Pravin Shelar wrote: >> On Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert >> <steffen.klassert@secunet.com> wrote: >> > On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote: >> >> fallback tunnel s not required to be in hash table, Its is returned if >> >> none of hashed tunnels are matched, ref ip_tunnel_lookup(). >> >> Can you post command to reproduce this issue? >> >> >> > >> > Something like >> > >> > ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1 >> > >> > worked until v3.9 and stopped working with v3.10. >> >> OK, I see the bug, tunnel exact match lookup does not check fb tunnel. >> There are two options. >> 1. Fix ip_tunnel_find() to check for fb tunnel. >> 2. Add fb tunnel to hash table, which is what ur patch does. >> I think your patch is better solution as it get rid of special case. >> But patch is not complete. It needs to remove fb tunnel checks on >> netdev unregister. > > It looks like this is another bug that requires an additional patch. > We add the fallback tunnel to the unregister list when we iterate over > all netdevices in the namespace at the beginning of ip_tunnel_destroy() > and then again explicitly at the end of ip_tunnel_destroy(). right. And if the special case is removed, code will become simple. -- 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/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c index b8ce640..f2348f2 100644 --- a/net/ipv4/ip_tunnel.c +++ b/net/ipv4/ip_tunnel.c @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id, /* FB netdevice is special: we have one, and only one per netns. * Allowing to move it to another netns is clearly unsafe. */ - if (!IS_ERR(itn->fb_tunnel_dev)) + if (!IS_ERR(itn->fb_tunnel_dev)) { itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL; + ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev)); + } rtnl_unlock(); return PTR_RET(itn->fb_tunnel_dev);
Currently we can not update the tunnel parameters of the fallback tunnels because we don't find them in the hash lists. Fix this by adding them on initialization. Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> --- net/ipv4/ip_tunnel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)