ipgre: fix lwtunnel support

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 <[email protected]>

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

net/ipv4/ip_gre.c +1 -1

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

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 <[email protected]>

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

To keep backwards compatibility and prevent any surprises, gretap interfaces have priority in receiving packets with Ethernet headers.

Signed-off-by Jiri Benc <[email protected]>

---
 include/net/ip_tunnels.h |  1 +
 net/ipv4/ip_gre.c        | 34 +++++++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

net/ipv4/ip_gre.c +9 -0

+
+static int ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi)
+{
+       struct net *net = dev_net(skb->dev);
+       struct ip_tunnel_net *itn;
+       int res;
+
+       if (tpi->proto == htons(ETH_P_TEB))
+               itn = net_generic(net, gre_tap_net_id);

I think we should we return error in case of such configuration rather than silently ignoring it.

Quoted 5 lines · Jiri Benc

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

How user is suppose to sort out the type of packet? whether it is L2 or L3.

Is it fine to receive L2 packet over L3 device?
At least ip_tunnel_rcv() is not ready to handle such packet.

Quoted 7 lines · 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 <[email protected]>

Acked-by Pravin B Shelar <[email protected]>

Quoted 2 lines · pravin shelar

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.

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

To keep backwards compatibility and prevent any surprises, gretap interfaces have priority in receiving packets with Ethernet headers.

Quoted 6 lines · pravin shelar

Quoted 5 lines · Jiri Benc

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

Quoted 1 line

How user is suppose to sort out the type of packet? whether it is L2 or L3.

By skb->protocol, of course. Will be ETH_P_TEB if the inner packet was Ethernet. There's no problem with this, for lwtunnels used with encap routes, such packets are just discarded (which is the expected behavior), as there's no protocol handler for ETH_P_TEB. More clever lwtunnel users (such as openvswitch) can process the packets. This is important in order to have a single tunnel interface for everything.

Quoted 2 lines

Is it fine to receive L2 packet over L3 device?
At least ip_tunnel_rcv() is not ready to handle such packet.

I don't see why. Could you please elaborate? Seems safe to me. The desired result is skb->protocol == htons(ETH_P_TEB), network header(!) pointing to the start of the Ethernet frame (because we're tunneling Ethernet via ARPHRD_IPGRE interface, there's no L2 header to have), mac header not being set.

Quoted 5 lines · Jiri Benc

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

Quoted 2 lines

To keep backwards compatibility and prevent any surprises, gretap interfaces have priority in receiving packets with Ethernet headers.

I may be missing some context. Is anyone using this already or is this preparing the stage for another user? It's not clear to me from the commit message.

Quoted 8 lines · Jiri Benc

Quoted 2 lines · pravin shelar

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.

Quoted 2 lines

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.

Quoted 3 lines · Thomas Graf

I may be missing some context. Is anyone using this already or is this preparing the stage for another user? It's not clear to me from the commit message.

Hi Thomas,

I'm not sure what use Jiri may have in mind but I plan to use this to allow OvS to support packets without an Ethernet header to be received from and sent to a GRE tunnel.

I am reasonably sure there are no existing users.

But skb->protocol is not set to ETH_P_TEB anywhere in ip-gre module. Am I missing something?

ip_tunnel_rcv() checks device type (tunnel->dev->type) to perform ethernet specific processing on packet. I think that should be changed to check packet type.

For ipgre interfaces in collect metadata mode, receive also traffic with encapsulated Ethernet headers. The lwtunnel users are supposed to sort this out correctly. This allows to have mixed Ethernet + L3-only traffic on the same lwtunnel interface.

To keep backwards compatibility and prevent any surprises, gretap interfaces have priority in receiving packets with Ethernet headers.

I have had some success wiring up Open vSwitch to use this patch for transmit. However, I am wondering if something more is needed to allow differentiation between packets with and without an L2 header present on receive.