All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/3] ipgre: fix lwtunnel support
@ 2016-04-22 17:44 Jiri Benc
  2016-04-22 17:44 ` [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode Jiri Benc
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 17:44 UTC (permalink / raw)
  To: netdev; +Cc: Pravin B Shelar, Thomas Graf, Simon Horman

lwtunnels currently work only with gretap. This patchset fixes the bugs in
ipgre metadata mode implementation.

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.

Jiri Benc (3):
  gre: do not assign header_ops in collect metadata mode
  gre: build header correctly for collect metadata tunnels
  gre: receive also TEB packets for lwtunnels

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

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode
  2016-04-22 17:44 [PATCH net 0/3] ipgre: fix lwtunnel support Jiri Benc
@ 2016-04-22 17:44 ` Jiri Benc
  2016-04-22 21:04   ` pravin shelar
  2016-04-22 17:44 ` [PATCH net 2/3] gre: build header correctly for collect metadata tunnels Jiri Benc
  2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
  2 siblings, 1 reply; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 17:44 UTC (permalink / raw)
  To: netdev; +Cc: Pravin B Shelar, Thomas Graf, Simon Horman

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
-	} else
+	} else if (!tunnel->collect_md) {
 		dev->header_ops = &ipgre_header_ops;
+	}
 
 	return ip_tunnel_init(dev);
 }
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net 2/3] gre: build header correctly for collect metadata tunnels
  2016-04-22 17:44 [PATCH net 0/3] ipgre: fix lwtunnel support Jiri Benc
  2016-04-22 17:44 ` [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode Jiri Benc
@ 2016-04-22 17:44 ` Jiri Benc
  2016-04-22 21:05   ` pravin shelar
  2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
  2 siblings, 1 reply; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 17:44 UTC (permalink / raw)
  To: netdev; +Cc: Pravin B Shelar, Thomas Graf, Simon Horman

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>
---
 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 d0abde4236af..f973e0a58993 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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 17:44 [PATCH net 0/3] ipgre: fix lwtunnel support Jiri Benc
  2016-04-22 17:44 ` [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode Jiri Benc
  2016-04-22 17:44 ` [PATCH net 2/3] gre: build header correctly for collect metadata tunnels Jiri Benc
@ 2016-04-22 17:44 ` Jiri Benc
  2016-04-22 21:07   ` pravin shelar
                     ` (2 more replies)
  2 siblings, 3 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 17:44 UTC (permalink / raw)
  To: netdev; +Cc: Pravin B Shelar, Thomas Graf, Simon Horman

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 <jbenc@redhat.com>
---
 include/net/ip_tunnels.h |  1 +
 net/ipv4/ip_gre.c        | 34 +++++++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 56050f913339..ac8d6822072e 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -161,6 +161,7 @@ struct tnl_ptk_info {
 
 #define PACKET_RCVD	0
 #define PACKET_REJECT	1
+#define PACKET_NEXT	2
 
 #define IP_TNL_HASH_BITS   7
 #define IP_TNL_HASH_SIZE   (1 << IP_TNL_HASH_BITS)
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index f973e0a58993..ccd6b098928d 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -379,19 +379,13 @@ static __be32 tunnel_id_to_key(__be64 x)
 #endif
 }
 
-static int ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi)
+static int __ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi,
+		       struct ip_tunnel_net *itn)
 {
-	struct net *net = dev_net(skb->dev);
 	struct metadata_dst *tun_dst = NULL;
-	struct ip_tunnel_net *itn;
 	const struct iphdr *iph;
 	struct ip_tunnel *tunnel;
 
-	if (tpi->proto == htons(ETH_P_TEB))
-		itn = net_generic(net, gre_tap_net_id);
-	else
-		itn = net_generic(net, ipgre_net_id);
-
 	iph = ip_hdr(skb);
 	tunnel = ip_tunnel_lookup(itn, skb->dev->ifindex, tpi->flags,
 				  iph->saddr, iph->daddr, tpi->key);
@@ -412,7 +406,29 @@ static int ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi)
 		ip_tunnel_rcv(tunnel, skb, tpi, tun_dst, log_ecn_error);
 		return PACKET_RCVD;
 	}
-	return PACKET_REJECT;
+	return PACKET_NEXT;
+}
+
+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);
+	else
+		itn = net_generic(net, ipgre_net_id);
+
+	res = __ipgre_rcv(skb, tpi, itn);
+	if (res == PACKET_NEXT && tpi->proto == htons(ETH_P_TEB)) {
+		/* ipgre tunnels in collect metadata mode should receive
+		 * also ETH_P_TEB traffic
+		 */
+		itn = net_generic(net, ipgre_net_id);
+		res = __ipgre_rcv(skb, tpi, itn);
+	}
+	return res;
 }
 
 static int gre_rcv(struct sk_buff *skb)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode
  2016-04-22 17:44 ` [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode Jiri Benc
@ 2016-04-22 21:04   ` pravin shelar
  2016-04-22 21:20     ` Jiri Benc
  0 siblings, 1 reply; 19+ messages in thread
From: pravin shelar @ 2016-04-22 21:04 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 2/3] gre: build header correctly for collect metadata tunnels
  2016-04-22 17:44 ` [PATCH net 2/3] gre: build header correctly for collect metadata tunnels Jiri Benc
@ 2016-04-22 21:05   ` pravin shelar
  2016-04-23  0:03     ` Simon Horman
  0 siblings, 1 reply; 19+ messages in thread
From: pravin shelar @ 2016-04-22 21:05 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
@ 2016-04-22 21:07   ` pravin shelar
  2016-04-22 21:27     ` Jiri Benc
  2016-04-23  1:49   ` Thomas Graf
  2016-04-28  6:49   ` Simon Horman
  2 siblings, 1 reply; 19+ messages in thread
From: pravin shelar @ 2016-04-22 21:07 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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.

> To keep backwards compatibility and prevent any surprises, gretap interfaces
> have priority in receiving packets with Ethernet headers.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  include/net/ip_tunnels.h |  1 +
>  net/ipv4/ip_gre.c        | 34 +++++++++++++++++++++++++---------
>  2 files changed, 26 insertions(+), 9 deletions(-)
>

...

> +
> +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);
> +       else
> +               itn = net_generic(net, ipgre_net_id);
> +
> +       res = __ipgre_rcv(skb, tpi, itn);
> +       if (res == PACKET_NEXT && tpi->proto == htons(ETH_P_TEB)) {
> +               /* ipgre tunnels in collect metadata mode should receive
> +                * also ETH_P_TEB traffic
> +                */
> +               itn = net_generic(net, ipgre_net_id);
> +               res = __ipgre_rcv(skb, tpi, int);

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode
  2016-04-22 21:04   ` pravin shelar
@ 2016-04-22 21:20     ` Jiri Benc
  2016-04-23  1:41       ` Thomas Graf
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 21:20 UTC (permalink / raw)
  To: pravin shelar
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 21:07   ` pravin shelar
@ 2016-04-22 21:27     ` Jiri Benc
  2016-04-23  3:40       ` pravin shelar
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Benc @ 2016-04-22 21:27 UTC (permalink / raw)
  To: pravin shelar
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

On Fri, 22 Apr 2016 14:07:01 -0700, pravin shelar wrote:
> On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote:
> > 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.

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.

> 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.

 Jiri

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 2/3] gre: build header correctly for collect metadata tunnels
  2016-04-22 21:05   ` pravin shelar
@ 2016-04-23  0:03     ` Simon Horman
  0 siblings, 0 replies; 19+ messages in thread
From: Simon Horman @ 2016-04-23  0:03 UTC (permalink / raw)
  To: pravin shelar
  Cc: Jiri Benc, Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf

On Fri, Apr 22, 2016 at 02:05:06PM -0700, pravin shelar wrote:
> On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote:
> > 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>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode
  2016-04-22 21:20     ` Jiri Benc
@ 2016-04-23  1:41       ` Thomas Graf
  2016-04-24  9:31         ` Jiri Benc
  0 siblings, 1 reply; 19+ messages in thread
From: Thomas Graf @ 2016-04-23  1:41 UTC (permalink / raw)
  To: Jiri Benc
  Cc: pravin shelar, Linux Kernel Network Developers, Pravin B Shelar,
	Simon Horman

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
  2016-04-22 21:07   ` pravin shelar
@ 2016-04-23  1:49   ` Thomas Graf
  2016-04-23  5:09     ` Simon Horman
  2016-04-28  6:49   ` Simon Horman
  2 siblings, 1 reply; 19+ messages in thread
From: Thomas Graf @ 2016-04-23  1:49 UTC (permalink / raw)
  To: Jiri Benc; +Cc: netdev, Pravin B Shelar, Simon Horman

On 04/22/16 at 07:44pm, Jiri Benc wrote:
> 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 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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 21:27     ` Jiri Benc
@ 2016-04-23  3:40       ` pravin shelar
  2016-04-24  9:54         ` Jiri Benc
  0 siblings, 1 reply; 19+ messages in thread
From: pravin shelar @ 2016-04-23  3:40 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

On Fri, Apr 22, 2016 at 2:27 PM, Jiri Benc <jbenc@redhat.com> wrote:
> On Fri, 22 Apr 2016 14:07:01 -0700, pravin shelar wrote:
>> On Fri, Apr 22, 2016 at 10:44 AM, Jiri Benc <jbenc@redhat.com> wrote:
>> > 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.
>
> 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.
>

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


>> 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.
>

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-23  1:49   ` Thomas Graf
@ 2016-04-23  5:09     ` Simon Horman
  2016-04-24  9:38       ` Jiri Benc
  0 siblings, 1 reply; 19+ messages in thread
From: Simon Horman @ 2016-04-23  5:09 UTC (permalink / raw)
  To: Thomas Graf; +Cc: Jiri Benc, netdev, Pravin B Shelar

On Sat, Apr 23, 2016 at 03:49:38AM +0200, Thomas Graf wrote:
> On 04/22/16 at 07:44pm, Jiri Benc wrote:
> > 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 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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode
  2016-04-23  1:41       ` Thomas Graf
@ 2016-04-24  9:31         ` Jiri Benc
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-24  9:31 UTC (permalink / raw)
  To: Thomas Graf
  Cc: pravin shelar, Linux Kernel Network Developers, Pravin B Shelar,
	Simon Horman

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-23  5:09     ` Simon Horman
@ 2016-04-24  9:38       ` Jiri Benc
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-24  9:38 UTC (permalink / raw)
  To: Simon Horman; +Cc: Thomas Graf, netdev, Pravin B Shelar

On Sat, 23 Apr 2016 15:09:16 +1000, Simon Horman wrote:
> On Sat, Apr 23, 2016 at 03:49:38AM +0200, Thomas Graf wrote:
> > 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.

Yes, that's the main use case as far as I can see. Sorry for not being
clear.

The intention is to support tunnels that can have mixed L2+L3 traffic
over a single lwtunnel interface. VXLAN-GPE already does this, this
patch makes GRE behave identically, while preserving all current GRE
users.

The reason I sent this for net and not net-next is that I wanted to
prevent the situation when we have a kernel released that supports
ipgre lwtunnels but incompletely - i.e., ETH_P_TEB frames are
discarded. But thinking about it more, it's probably non-issue. I'll
retarget this patch for net-next and resend the first two for net.

Thanks,

 Jiri

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-23  3:40       ` pravin shelar
@ 2016-04-24  9:54         ` Jiri Benc
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-24  9:54 UTC (permalink / raw)
  To: pravin shelar
  Cc: Linux Kernel Network Developers, Pravin B Shelar, Thomas Graf,
	Simon Horman

On Fri, 22 Apr 2016 20:40:13 -0700, pravin shelar wrote:
> But skb->protocol is not set to ETH_P_TEB anywhere in ip-gre module.
> Am I missing something?

Ah, I see your point. It needs to be solved a bit differently, though,
we need to call __iptunnel_pull_header instead of iptunnel_pull_header
for these packets. I'll rework the patch.

> 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.

The current behavior is correct. The Ethernet processing depends on
the interface type, ARPHRD_IPGRE interfaces can't treat Ethernet
headers as L2 headers, that wouldn't match the interface type.

 Jiri

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
  2016-04-22 21:07   ` pravin shelar
  2016-04-23  1:49   ` Thomas Graf
@ 2016-04-28  6:49   ` Simon Horman
  2016-04-28  8:23     ` Jiri Benc
  2 siblings, 1 reply; 19+ messages in thread
From: Simon Horman @ 2016-04-28  6:49 UTC (permalink / raw)
  To: Jiri Benc; +Cc: netdev, Pravin B Shelar, Thomas Graf

On Fri, Apr 22, 2016 at 07:44:08PM +0200, Jiri Benc wrote:
> 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.

Hi Jiri,

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.

I had luck getting receive working with the following:

From: Simon Horman <simon.horman@netronome.com>
Date: Mon, 18 Apr 2016 17:48:47 +1000
Subject: [PATCH] gre: mark presense of l2 when recieving TEB packets for lwtunnels

There seems to be some way for receivers to differentiate between
packets recieved with and without an l2 header. The approach taken here
is to use a new mode bit in struct ip_tunnel_key.

Another approach might be to store tpi->proto in tunnel metadata,
though that would consume 16 bits somewhere and seems like overkill
at this point.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/ip_tunnels.h | 6 ++++++
 net/ipv4/ip_gre.c        | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index d916b4315903..cdf71ced429e 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -58,6 +58,12 @@ struct ip_tunnel_key {
 /* Flags for ip_tunnel_info mode. */
 #define IP_TUNNEL_INFO_TX	0x01	/* represents tx tunnel parameters */
 #define IP_TUNNEL_INFO_IPV6	0x02	/* key contains IPv6 addresses */
+#define IP_TUNNEL_INFO_L2_PRESENT  0x04	/* Set on receive by tunnels that
+					 * may receive packets both with
+					 * and without an L2 header present
+					 * when a packet is received with
+					 * L2 header present.
+					 */
 
 /* Maximum tunnel options length. */
 #define IP_TUNNEL_OPTS_MAX					\
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 0a4af2896a15..1290695fbc95 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -401,6 +401,9 @@ static int __ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi,
 			tun_dst = ip_tun_rx_dst(skb, flags, tun_id, 0);
 			if (!tun_dst)
 				return PACKET_REJECT;
+			if (tpi->proto == htons(ETH_P_TEB))
+				tun_dst->u.tun_info.mode |=
+					IP_TUNNEL_INFO_L2_PRESENT;
 		}
 
 		ip_tunnel_rcv(tunnel, skb, tpi, tun_dst, log_ecn_error);
-- 
2.7.0.rc3.207.g0ac5344

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net 3/3] gre: receive also TEB packets for lwtunnels
  2016-04-28  6:49   ` Simon Horman
@ 2016-04-28  8:23     ` Jiri Benc
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Benc @ 2016-04-28  8:23 UTC (permalink / raw)
  To: Simon Horman; +Cc: netdev, Pravin B Shelar, Thomas Graf

On Thu, 28 Apr 2016 16:49:19 +1000, Simon Horman wrote:
> 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.

The problem, as Pravin pointed out, is the patch does not correctly set
skb->protocol to ETH_P_TEB because of special handling of ETH_P_TEB in
iptunnel_pull_header. This will be fixed in v2 of the patch.

No flag will be needed then, just use skb->protocol.

 Jiri

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-04-28  8:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-22 17:44 [PATCH net 0/3] ipgre: fix lwtunnel support Jiri Benc
2016-04-22 17:44 ` [PATCH net 1/3] gre: do not assign header_ops in collect metadata mode Jiri Benc
2016-04-22 21:04   ` pravin shelar
2016-04-22 21:20     ` Jiri Benc
2016-04-23  1:41       ` Thomas Graf
2016-04-24  9:31         ` Jiri Benc
2016-04-22 17:44 ` [PATCH net 2/3] gre: build header correctly for collect metadata tunnels Jiri Benc
2016-04-22 21:05   ` pravin shelar
2016-04-23  0:03     ` Simon Horman
2016-04-22 17:44 ` [PATCH net 3/3] gre: receive also TEB packets for lwtunnels Jiri Benc
2016-04-22 21:07   ` pravin shelar
2016-04-22 21:27     ` Jiri Benc
2016-04-23  3:40       ` pravin shelar
2016-04-24  9:54         ` Jiri Benc
2016-04-23  1:49   ` Thomas Graf
2016-04-23  5:09     ` Simon Horman
2016-04-24  9:38       ` Jiri Benc
2016-04-28  6:49   ` Simon Horman
2016-04-28  8:23     ` Jiri Benc

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.