gre: fix lwtunnel support

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

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.

---
 net/ipv4/ip_gre.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index af5d1f38217f..c035b43b1d4b 100644
@@ -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

Hello.

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

[...]

MBR, Sergei

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

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.

---
v2: unchanged

---
 net/ipv4/ip_gre.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index c035b43b1d4b..02b12db59437 100644
@@ -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

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

---
New in v2.

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

index 02b12db59437..27716c72b8e2 100644
@@ -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

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