[PATCH net v2 0/3] gre: fix lwtunnel support

[PATCH net v2 0/3] gre: fix lwtunnel support

[Jiri Benc]

This patchset fixes a few bugs in gre metadata mode implementation, mainly
with ipgre.

As an example, in this setup:

ip a a 192.168.1.1/24 dev eth0
ip l a gre1 type gre external
ip l s gre1 up
ip a a 192.168.99.1/24 dev gre1
ip r a 192.168.99.2/32 encap ip dst 192.168.1.2 ttl 10 dev gre1
ping 192.168.99.2

the traffic does not go through before this patchset and does as expected
with it applied.

v2: Rejecting invalid configuration, added patch 3, dropped patch for
    ETH_P_TEB (will target net-next).

Jiri Benc (3):
  gre: do not assign header_ops in collect metadata mode
  gre: build header correctly for collect metadata tunnels
  gre: allow creation of gretap interfaces in metadata mode

 net/ipv4/ip_gre.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[PATCH net v2 1/3] gre: do not assign header_ops in collect metadata mode

[Jiri Benc]

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.

Similarly, when a multicast remote IP address is set together with the
collect metadata flag, the processing described above would happen, too. As
there's not much sense in specifying remote/local IP address for lwtunnels,
reject such configuration.

v2: Reject configuration specifying both remote/local address and collect
    metadata flag.

Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/ipv4/ip_gre.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index af5d1f38217f..c035b43b1d4b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -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);
 }
@@ -946,6 +947,15 @@ static int ipgre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
 	if (flags & (GRE_VERSION|GRE_ROUTING))
 		return -EINVAL;
 
+	if (data[IFLA_GRE_COLLECT_METADATA]) {
+		if (data[IFLA_GRE_REMOTE] &&
+		    nla_get_in_addr(data[IFLA_GRE_REMOTE]))
+			return -EINVAL;
+		if (data[IFLA_GRE_LOCAL] &&
+		    nla_get_in_addr(data[IFLA_GRE_LOCAL]))
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
1.8.3.1

Re: [PATCH net v2 1/3] gre: do not assign header_ops in collect metadata mode

[Sergei Shtylyov]

Hello.

On 4/24/2016 2:00 PM, Jiri Benc 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).

    Didn't checkpatch.pl complain about the commit citing style?

> 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.
>
> Similarly, when a multicast remote IP address is set together with the
> collect metadata flag, the processing described above would happen, too. As
> there's not much sense in specifying remote/local IP address for lwtunnels,
> reject such configuration.
>
> v2: Reject configuration specifying both remote/local address and collect
>      metadata flag.
>
> Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
[...]

MBR, Sergei

Re: [PATCH net v2 1/3] gre: do not assign header_ops in collect metadata mode

[Jiri Benc]

On Sun, 24 Apr 2016 13:00:20 +0200, Jiri Benc 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.
> 
> Similarly, when a multicast remote IP address is set together with the
> collect metadata flag, the processing described above would happen, too. As
> there's not much sense in specifying remote/local IP address for lwtunnels,
> reject such configuration.
> 
> v2: Reject configuration specifying both remote/local address and collect
>     metadata flag.

Thinking about this more, this *is* uAPI breakage and we can't do that.
It affects also gretap and current users of gretap lwtunnels would be
broken. This is even more likely as it's not possible to create gretap
lwtunnel using iproute2 without specifying a fake remote address.

In theory, we could wrap the current ipgre_tunnel_validate with a new
function that would be set as ipgre_link_ops->validate but that would
add unnecessary complexity and would make ipgre and gretap behavior
different.

I'm sorry, we have to live with what we have. This should have been
taken into account when you implemented lwtunnels for GRE, we can't
change this now, it's too late.

I'll send v3 with this patch being restored to exactly what I had in v1.

 Jiri

[PATCH net v2 2/3] gre: build header correctly for collect metadata tunnels

[Jiri Benc]

In ipgre (i.e. not gretap) + collect metadata mode, the skb was assumed to
contain Ethernet header and was encapsulated as ETH_P_TEB. This is not the
case, the interface is ARPHRD_IPGRE and the protocol to be used for
encapsulation is skb->protocol.

Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
Acked-by: Pravin B Shelar <pshelar@ovn.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
v2: unchanged
---
 net/ipv4/ip_gre.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c035b43b1d4b..02b12db59437 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -523,7 +523,8 @@ static struct rtable *gre_get_rt(struct sk_buff *skb,
 	return ip_route_output_key(net, fl);
 }
 
-static void gre_fb_xmit(struct sk_buff *skb, struct net_device *dev)
+static void gre_fb_xmit(struct sk_buff *skb, struct net_device *dev,
+			__be16 proto)
 {
 	struct ip_tunnel_info *tun_info;
 	const struct ip_tunnel_key *key;
@@ -575,7 +576,7 @@ static void gre_fb_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	flags = tun_info->key.tun_flags & (TUNNEL_CSUM | TUNNEL_KEY);
-	build_header(skb, tunnel_hlen, flags, htons(ETH_P_TEB),
+	build_header(skb, tunnel_hlen, flags, proto,
 		     tunnel_id_to_key(tun_info->key.tun_id), 0);
 
 	df = key->tun_flags & TUNNEL_DONT_FRAGMENT ?  htons(IP_DF) : 0;
@@ -616,7 +617,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 	const struct iphdr *tnl_params;
 
 	if (tunnel->collect_md) {
-		gre_fb_xmit(skb, dev);
+		gre_fb_xmit(skb, dev, skb->protocol);
 		return NETDEV_TX_OK;
 	}
 
@@ -660,7 +661,7 @@ static netdev_tx_t gre_tap_xmit(struct sk_buff *skb,
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 
 	if (tunnel->collect_md) {
-		gre_fb_xmit(skb, dev);
+		gre_fb_xmit(skb, dev, htons(ETH_P_TEB));
 		return NETDEV_TX_OK;
 	}
 
-- 
1.8.3.1

[PATCH net v2 3/3] gre: allow creation of gretap interfaces in metadata mode

[Jiri Benc]

The IFLA_GRE_REMOTE attribute does not make sense together with collect
metadata and is ignored in such case. However, iproute2 always sets it; it
will be zero if there's no remote address specified on the command line.

Remove the check for non-zero IFLA_GRE_REMOTE when collect medata flag is
set.

Before the patch, this command returns failure, after the patch, it works as
expected:

ip link add gre1 type gretap external

Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
New in v2.
---
 net/ipv4/ip_gre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 02b12db59437..27716c72b8e2 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -974,7 +974,7 @@ static int ipgre_tap_validate(struct nlattr *tb[], struct nlattr *data[])
 	if (!data)
 		goto out;
 
-	if (data[IFLA_GRE_REMOTE]) {
+	if (data[IFLA_GRE_REMOTE] && !data[IFLA_GRE_COLLECT_METADATA]) {
 		memcpy(&daddr, nla_data(data[IFLA_GRE_REMOTE]), 4);
 		if (!daddr)
 			return -EINVAL;
-- 
1.8.3.1

Re: [PATCH net v2 3/3] gre: allow creation of gretap interfaces in metadata mode

[pravin shelar]

On Sun, Apr 24, 2016 at 4:00 AM, Jiri Benc <jbenc@redhat.com> wrote:
> The IFLA_GRE_REMOTE attribute does not make sense together with collect
> metadata and is ignored in such case. However, iproute2 always sets it; it
> will be zero if there's no remote address specified on the command line.
>
> Remove the check for non-zero IFLA_GRE_REMOTE when collect medata flag is
> set.
>
Rather than cover up in ip_gre kernel module, why not just fix
iproute2 to set the attribute correctly?


> Before the patch, this command returns failure, after the patch, it works as
> expected:
>
> ip link add gre1 type gretap external
>
> Fixes: 2e15ea390e6f4 ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> New in v2.
> ---

>  net/ipv4/ip_gre.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Re: [PATCH net v2 3/3] gre: allow creation of gretap interfaces in metadata mode

[Jiri Benc]

On Mon, 25 Apr 2016 11:00:35 -0700, pravin shelar wrote:
> On Sun, Apr 24, 2016 at 4:00 AM, Jiri Benc <jbenc@redhat.com> wrote:
> > The IFLA_GRE_REMOTE attribute does not make sense together with collect
> > metadata and is ignored in such case. However, iproute2 always sets it; it
> > will be zero if there's no remote address specified on the command line.
> >
> > Remove the check for non-zero IFLA_GRE_REMOTE when collect medata flag is
> > set.
> >
> Rather than cover up in ip_gre kernel module, why not just fix
> iproute2 to set the attribute correctly?

Well, who defines "correctly"? :-)

I can do that, it really doesn't matter. One way or the other, the
result will be the same. I'll move this to iproute2 in v3.

 Jiri