Message ID | 7c34e7243a146fc5ccdf6349892355746741ff26.1461346798.git.jbenc@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote: > In ipgre mode (i.e. not gretap) with collect metadata flag set, the tunnel > is incorrectly assumed to be mGRE in NBMA mode (see commit 6a5f44d7a048c). > This is not the case, we're controlling the encapsulation addresses by > lwtunnel metadata. And anyway, assigning dev->header_ops in collect metadata > mode does not make sense. > > Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.") > Signed-off-by: Jiri Benc <jbenc@redhat.com> > --- > net/ipv4/ip_gre.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index af5d1f38217f..d0abde4236af 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -893,7 +893,7 @@ static int ipgre_tunnel_init(struct net_device *dev) > netif_keep_dst(dev); > dev->addr_len = 4; > > - if (iph->daddr) { > + if (iph->daddr && !tunnel->collect_md) { > #ifdef CONFIG_NET_IPGRE_BROADCAST > if (ipv4_is_multicast(iph->daddr)) { > if (!iph->saddr) > @@ -902,8 +902,9 @@ static int ipgre_tunnel_init(struct net_device *dev) > dev->header_ops = &ipgre_header_ops; > } > #endif I think we should we return error in case of such configuration rather than silently ignoring it.
On Fri, 22 Apr 2016 14:04:48 -0700, pravin shelar wrote: > I think we should we return error in case of such configuration rather > than silently ignoring it. I thought about it and I'm not sure. We're not returning an error currently, starting returning it now might be perceived as uAPI breakage. But given it doesn't work at all currently, there are apparently no users yet. I'll wait for more feedback. Jiri
On 04/22/16 at 11:20pm, Jiri Benc wrote: > On Fri, 22 Apr 2016 14:04:48 -0700, pravin shelar wrote: > > I think we should we return error in case of such configuration rather > > than silently ignoring it. > > I thought about it and I'm not sure. We're not returning an error > currently, starting returning it now might be perceived as uAPI > breakage. > > But given it doesn't work at all currently, there are apparently no > users yet. I'll wait for more feedback. As a user, I would probably favour receiving an error for a configuration that can't possibly work and was not working before.
On Sat, 23 Apr 2016 03:41:43 +0200, Thomas Graf wrote: > On 04/22/16 at 11:20pm, Jiri Benc wrote: > > On Fri, 22 Apr 2016 14:04:48 -0700, pravin shelar wrote: > > > I think we should we return error in case of such configuration rather > > > than silently ignoring it. > > > > I thought about it and I'm not sure. We're not returning an error > > currently, starting returning it now might be perceived as uAPI > > breakage. > > > > But given it doesn't work at all currently, there are apparently no > > users yet. I'll wait for more feedback. > > As a user, I would probably favour receiving an error for a configuration > that can't possibly work and was not working before. Okay, I'll change this in v2. Thanks, Pravin and Thomas. Jiri
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index af5d1f38217f..d0abde4236af 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -893,7 +893,7 @@ static int ipgre_tunnel_init(struct net_device *dev) netif_keep_dst(dev); dev->addr_len = 4; - if (iph->daddr) { + if (iph->daddr && !tunnel->collect_md) { #ifdef CONFIG_NET_IPGRE_BROADCAST if (ipv4_is_multicast(iph->daddr)) { if (!iph->saddr) @@ -902,8 +902,9 @@ static int ipgre_tunnel_init(struct net_device *dev) dev->header_ops = &ipgre_header_ops; } #endif - } else + } else if (!tunnel->collect_md) { dev->header_ops = &ipgre_header_ops; + } return ip_tunnel_init(dev); }
In ipgre mode (i.e. not gretap) with collect metadata flag set, the tunnel is incorrectly assumed to be mGRE in NBMA mode (see commit 6a5f44d7a048c). This is not the case, we're controlling the encapsulation addresses by lwtunnel metadata. And anyway, assigning dev->header_ops in collect metadata mode does not make sense. Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.") Signed-off-by: Jiri Benc <jbenc@redhat.com> --- net/ipv4/ip_gre.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)