ip_tunnel: specify tunnel type via template

This new field allows you to restrict the metadata template for a given tunnel driver. This is convenient in scenarios that combine different tunneling drivers, to deal with possible misconfigurations given that the template can be interpreted by any target tunnel driver. Default value is IP_TUNNEL_TYPE_UNSPEC, to retain the existing behaviour. This also implicitly exposes what drivers are currently supported in the IP_TUNNEL_INFO_TX mode.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 drivers/net/geneve.c           |  3 ++-
 drivers/net/vxlan.c            | 13 +++++++------
 include/net/dst_metadata.h     |  1 +
 include/net/ip_tunnels.h       | 16 ++++++++++++++++
 net/ipv4/ip_gre.c              |  2 ++
 net/ipv6/ip6_gre.c             |  2 ++
 net/openvswitch/flow_netlink.c |  1 +
 7 files changed, 31 insertions(+), 7 deletions(-)

drivers/net/geneve.c +2 -1

index 6625fabe2c88..c383c394f0d2 100644
@@ -920,7 +920,8 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (geneve->collect_md) {
 		info = skb_tunnel_info(skb);
-		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX))) {
+		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(info, IP_TUNNEL_TYPE_GENEVE))) {
 			err = -EINVAL;
 			netdev_dbg(dev, "no tunnel metadata\n");
 			goto tx_error;

drivers/net/vxlan.c +7 -6

index e5d236595206..8cca91b572bd 100644
@@ -2296,14 +2296,15 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_reset_mac_header(skb);
 
 	if (vxlan->cfg.flags & VXLAN_F_COLLECT_METADATA) {
-		if (info && info->mode & IP_TUNNEL_INFO_BRIDGE &&
-		    info->mode & IP_TUNNEL_INFO_TX) {
+		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(info, IP_TUNNEL_TYPE_VXLAN))) {
+			kfree_skb(skb);
+			return NETDEV_TX_OK;
+		}
+		if (info->mode & IP_TUNNEL_INFO_BRIDGE) {
 			vni = tunnel_id_to_key32(info->key.tun_id);
 		} else {
-			if (info && info->mode & IP_TUNNEL_INFO_TX)
-				vxlan_xmit_one(skb, dev, vni, NULL, false);
-			else
-				kfree_skb(skb);
+			vxlan_xmit_one(skb, dev, vni, NULL, false);
 			return NETDEV_TX_OK;
 		}
 	}

include/net/dst_metadata.h +1 -0

index 56cb3c38569a..674116f7fa4a 100644
@@ -100,6 +100,7 @@ static inline struct metadata_dst *tun_rx_dst(int md_size)
 	if (!tun_dst)
 		return NULL;
 
+	tun_dst->u.tun_info.type = IP_TUNNEL_TYPE_UNSPEC;
 	tun_dst->u.tun_info.options_len = 0;
 	tun_dst->u.tun_info.mode = 0;
 	return tun_dst;

include/net/ip_tunnels.h +16 -0

index b0d022ff6ea1..985d24b6a102 100644
@@ -66,7 +66,16 @@ struct ip_tunnel_key {
 	GENMASK((FIELD_SIZEOF(struct ip_tunnel_info,		\
 			      options_len) * BITS_PER_BYTE) - 1, 0)
 
+enum ip_tunnel_type {
+	IP_TUNNEL_TYPE_UNSPEC	= 0,
+	IP_TUNNEL_TYPE_GRE,
+	IP_TUNNEL_TYPE_VXLAN,
+	IP_TUNNEL_TYPE_GENEVE,
+	IP_TUNNEL_TYPE_ERSPAN,
+};
+
 struct ip_tunnel_info {
+	enum ip_tunnel_type	type;
 	struct ip_tunnel_key	key;
 #ifdef CONFIG_DST_CACHE
 	struct dst_cache	dst_cache;

@@ -75,6 +84,13 @@ struct ip_tunnel_info {
 	u8			mode;
 };
 
+static inline bool ip_tunnel_type(const struct ip_tunnel_info *tun_info,
+				  enum ip_tunnel_type type)
+{
+	return tun_info->type == IP_TUNNEL_TYPE_UNSPEC ||
+	       tun_info->type == type;
+}
+
 /* 6rd prefix/relay information */
 #ifdef CONFIG_IPV6_SIT_6RD
 struct ip_tunnel_6rd_parm {

net/ipv4/ip_gre.c +2 -0

index c3385a84f8ff..3ad12135c3d3 100644
@@ -534,6 +534,7 @@ static void gre_fb_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	tun_info = skb_tunnel_info(skb);
 	if (unlikely(!tun_info || !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+		     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_GRE) ||
 		     ip_tunnel_info_af(tun_info) != AF_INET))
 		goto err_free_skb;
 

@@ -585,6 +586,7 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	tun_info = skb_tunnel_info(skb);
 	if (unlikely(!tun_info || !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+		     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_ERSPAN) ||
 		     ip_tunnel_info_af(tun_info) != AF_INET))
 		goto err_free_skb;
 

net/ipv6/ip6_gre.c +2 -0

index 515adbdba1d2..675f373809ee 100644
@@ -732,6 +732,7 @@ static netdev_tx_t __gre6_xmit(struct sk_buff *skb,
 		tun_info = skb_tunnel_info(skb);
 		if (unlikely(!tun_info ||
 			     !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_GRE) ||
 			     ip_tunnel_info_af(tun_info) != AF_INET6))
 			return -EINVAL;
 

@@ -960,6 +961,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		tun_info = skb_tunnel_info(skb);
 		if (unlikely(!tun_info ||
 			     !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_ERSPAN) ||
 			     ip_tunnel_info_af(tun_info) != AF_INET6))
 			return -EINVAL;
 

net/openvswitch/flow_netlink.c +0 -0

index a70097ecf33c..1ee2509534df 100644

Quoted 153 lines · Pablo Neira Ayuso

This new field allows you to restrict the metadata template for a given tunnel driver. This is convenient in scenarios that combine different tunneling drivers, to deal with possible misconfigurations given that the template can be interpreted by any target tunnel driver. Default value is IP_TUNNEL_TYPE_UNSPEC, to retain the existing behaviour. This also implicitly exposes what drivers are currently supported in the IP_TUNNEL_INFO_TX mode.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 drivers/net/geneve.c           |  3 ++-
 drivers/net/vxlan.c            | 13 +++++++------
 include/net/dst_metadata.h     |  1 +
 include/net/ip_tunnels.h       | 16 ++++++++++++++++
 net/ipv4/ip_gre.c              |  2 ++
 net/ipv6/ip6_gre.c             |  2 ++
 net/openvswitch/flow_netlink.c |  1 +
 7 files changed, 31 insertions(+), 7 deletions(-)

drivers/net/geneve.c +2 -1

index 6625fabe2c88..c383c394f0d2 100644
@@ -920,7 +920,8 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (geneve->collect_md) {
 		info = skb_tunnel_info(skb);
-		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX))) {
+		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(info, IP_TUNNEL_TYPE_GENEVE))) {
 			err = -EINVAL;
 			netdev_dbg(dev, "no tunnel metadata\n");
 			goto tx_error;

drivers/net/vxlan.c +7 -6

index e5d236595206..8cca91b572bd 100644
@@ -2296,14 +2296,15 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_reset_mac_header(skb);
 
 	if (vxlan->cfg.flags & VXLAN_F_COLLECT_METADATA) {
-		if (info && info->mode & IP_TUNNEL_INFO_BRIDGE &&
-		    info->mode & IP_TUNNEL_INFO_TX) {
+		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(info, IP_TUNNEL_TYPE_VXLAN))) {
+			kfree_skb(skb);
+			return NETDEV_TX_OK;
+		}
+		if (info->mode & IP_TUNNEL_INFO_BRIDGE) {
 			vni = tunnel_id_to_key32(info->key.tun_id);
 		} else {
-			if (info && info->mode & IP_TUNNEL_INFO_TX)
-				vxlan_xmit_one(skb, dev, vni, NULL, false);
-			else
-				kfree_skb(skb);
+			vxlan_xmit_one(skb, dev, vni, NULL, false);
 			return NETDEV_TX_OK;
 		}
 	}

include/net/dst_metadata.h +1 -0

index 56cb3c38569a..674116f7fa4a 100644
@@ -100,6 +100,7 @@ static inline struct metadata_dst *tun_rx_dst(int md_size)
 	if (!tun_dst)
 		return NULL;
 
+	tun_dst->u.tun_info.type = IP_TUNNEL_TYPE_UNSPEC;
 	tun_dst->u.tun_info.options_len = 0;
 	tun_dst->u.tun_info.mode = 0;
 	return tun_dst;

include/net/ip_tunnels.h +16 -0

index b0d022ff6ea1..985d24b6a102 100644
@@ -66,7 +66,16 @@ struct ip_tunnel_key {
 	GENMASK((FIELD_SIZEOF(struct ip_tunnel_info,		\
 			      options_len) * BITS_PER_BYTE) - 1, 0)
 
+enum ip_tunnel_type {
+	IP_TUNNEL_TYPE_UNSPEC	= 0,
+	IP_TUNNEL_TYPE_GRE,
+	IP_TUNNEL_TYPE_VXLAN,
+	IP_TUNNEL_TYPE_GENEVE,
+	IP_TUNNEL_TYPE_ERSPAN,
+};
+
 struct ip_tunnel_info {
+	enum ip_tunnel_type	type;
 	struct ip_tunnel_key	key;
 #ifdef CONFIG_DST_CACHE
 	struct dst_cache	dst_cache;

@@ -75,6 +84,13 @@ struct ip_tunnel_info {
 	u8			mode;
 };
 
+static inline bool ip_tunnel_type(const struct ip_tunnel_info *tun_info,
+				  enum ip_tunnel_type type)
+{
+	return tun_info->type == IP_TUNNEL_TYPE_UNSPEC ||
+	       tun_info->type == type;
+}
+
 /* 6rd prefix/relay information */
 #ifdef CONFIG_IPV6_SIT_6RD
 struct ip_tunnel_6rd_parm {

net/ipv4/ip_gre.c +2 -0

index c3385a84f8ff..3ad12135c3d3 100644
@@ -534,6 +534,7 @@ static void gre_fb_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	tun_info = skb_tunnel_info(skb);
 	if (unlikely(!tun_info || !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+		     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_GRE) ||
 		     ip_tunnel_info_af(tun_info) != AF_INET))
 		goto err_free_skb;
 

@@ -585,6 +586,7 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	tun_info = skb_tunnel_info(skb);
 	if (unlikely(!tun_info || !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+		     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_ERSPAN) ||
 		     ip_tunnel_info_af(tun_info) != AF_INET))
 		goto err_free_skb;
 

net/ipv6/ip6_gre.c +2 -0

index 515adbdba1d2..675f373809ee 100644
@@ -732,6 +732,7 @@ static netdev_tx_t __gre6_xmit(struct sk_buff *skb,
 		tun_info = skb_tunnel_info(skb);
 		if (unlikely(!tun_info ||
 			     !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_GRE) ||
 			     ip_tunnel_info_af(tun_info) != AF_INET6))
 			return -EINVAL;
 

@@ -960,6 +961,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		tun_info = skb_tunnel_info(skb);
 		if (unlikely(!tun_info ||
 			     !(tun_info->mode & IP_TUNNEL_INFO_TX) ||
+			     !ip_tunnel_type(tun_info, IP_TUNNEL_TYPE_ERSPAN) ||
 			     ip_tunnel_info_af(tun_info) != AF_INET6))
 			return -EINVAL;
 

net/openvswitch/flow_netlink.c +0 -0

index a70097ecf33c..1ee2509534df 100644
@@ -2602,6 +2602,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
 	ovs_tun->tun_dst = tun_dst;
 

net/openvswitch/flow_netlink.c static int validate_and_copy_set_tun(const struct nlattr *attr, · line 2608

Quoted 6 lines

net/openvswitch/flow_netlink.c +1 -0

 	tun_info = &tun_dst->u.tun_info;
+	tun_info->type = IP_TUNNEL_TYPE_UNSPEC;
 	tun_info->mode = IP_TUNNEL_INFO_TX;
 	if (key.tun_proto == AF_INET6)
 		tun_info->mode |= IP_TUNNEL_INFO_IPV6;

If so then this should also be made explicit IP_TUNNEL_TYPE_UNSPEC in BPF code since all these tunnel types are supported there as well.

Quoted 1 line · Daniel Borkmann

On 10/04/2018 02:03 AM, Pablo Neira Ayuso wrote:

Quoted 7 lines

Quoted 7 lines

net/openvswitch/flow_netlink.c +0 -0

index a70097ecf33c..1ee2509534df 100644
@@ -2602,6 +2602,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
 	ovs_tun->tun_dst = tun_dst;
 

Quoted 9 lines

net/openvswitch/flow_netlink.c static int validate_and_copy_set_tun(const struct nlattr *attr, · line 2608

Quoted 6 lines

net/openvswitch/flow_netlink.c +1 -0

 	tun_info = &tun_dst->u.tun_info;
+	tun_info->type = IP_TUNNEL_TYPE_UNSPEC;
 	tun_info->mode = IP_TUNNEL_INFO_TX;
 	if (key.tun_proto == AF_INET6)
 		tun_info->mode |= IP_TUNNEL_INFO_IPV6;

If so then this should also be made explicit IP_TUNNEL_TYPE_UNSPEC in BPF code since all these tunnel types are supported there as well.

Are you refering to proper initialization? I can see a memset() there for the ip_tunnel_info structure, which is implicitly setting tun_info->type to zero, ie. IP_TUNNEL_TYPE_UNSPEC.

I can also make it explicit there if you prefer.

Hi,

The following patchset adds a new field to the tunnel metadata template to restrict the configuration to a given tunnel driver. Currently, a misconfiguration may result in packets going to the wrong tunnel driver.

Although we have the tunnel option flags, they are not mandatory for some tunnel drivers, eg. vxlan, which may use it or not; and gre which does not use them.

This patch updates tc's tunnel action and netfilter's tunnel extension to use this new field. OVS netlink interface has been left unset, although they could be updated to use this.

By extending the existing tc action to support the IP_TUNNEL_INFO_BRIDGE mode, I think it should be possible to expose IP_TUNNEL_TYPE_VLAN too, although this patchset doesn't address this scenario.

The field is initialized to zero, which maps to IP_TUNNEL_TYPE_UNSPEC to retain the existing behaviour, so the existing flexibility is still in place while this new feature is added.

Cc'ing people that git annotate show as dealing with these bits more recently.

Quoted 9 lines · Pablo Neira Ayuso

Hi,

The following patchset adds a new field to the tunnel metadata template to restrict the configuration to a given tunnel driver. Currently, a misconfiguration may result in packets going to the wrong tunnel driver.

Although we have the tunnel option flags, they are not mandatory for some tunnel drivers, eg. vxlan, which may use it or not; and gre which does not use them.

Option flags are necessary because interpretation of option blob is entirely protocol-specific.

Quoted 8 lines

This patch updates tc's tunnel action and netfilter's tunnel extension to use this new field. OVS netlink interface has been left unset, although they could be updated to use this.

By extending the existing tc action to support the IP_TUNNEL_INFO_BRIDGE mode, I think it should be possible to expose IP_TUNNEL_TYPE_VLAN too, although this patchset doesn't address this scenario.

Quoted 6 lines

The field is initialized to zero, which maps to IP_TUNNEL_TYPE_UNSPEC to retain the existing behaviour, so the existing flexibility is still in place while this new feature is added.

Cc'ing people that git annotate show as dealing with these bits more recently.

What practical scenario are you trying to address here?

Have you seen
https://www.mail-archive.com/[email protected]/msg250705.html
?

Hi Jakub,

Quoted 21 lines · Jakub Kicinski

Quoted 9 lines · Pablo Neira Ayuso

Hi,

The following patchset adds a new field to the tunnel metadata template to restrict the configuration to a given tunnel driver. Currently, a misconfiguration may result in packets going to the wrong tunnel driver.

Although we have the tunnel option flags, they are not mandatory for some tunnel drivers, eg. vxlan, which may use it or not; and gre which does not use them.

Option flags are necessary because interpretation of option blob is entirely protocol-specific.

Quoted 7 lines

This patch updates tc's tunnel action and netfilter's tunnel extension to use this new field. OVS netlink interface has been left unset, although they could be updated to use this.

By extending the existing tc action to support the IP_TUNNEL_INFO_BRIDGE mode, I think it should be possible to expose IP_TUNNEL_TYPE_VLAN too, although this patchset doesn't address this scenario.

Quoted 8 lines

Quoted 6 lines

The field is initialized to zero, which maps to IP_TUNNEL_TYPE_UNSPEC to retain the existing behaviour, so the existing flexibility is still in place while this new feature is added.

Cc'ing people that git annotate show as dealing with these bits more recently.

What practical scenario are you trying to address here?

Incorrect configuration. The tunnel template defines an ID field, this ID means vni in vxlan, but it also means session in erspan. If a packet that should go to vxlan tunnel device (to be encapsulated using vni 5) ends up in a gre/erspan device, you will get an erspan packet with session 5. With this new tunnel type field, you can restrict the tunnel template to work _only_ for a given tunnel device driver. Hence, if the packet ends up in the wrong tunnel device driver due to incorrect configuration, packet gets dropped.

Quoted 2 lines

Have you seen
https://www.mail-archive.com/[email protected]/msg250705.html ?

Hm, I remember to have seen some hw offload driver code that is making assumptions on the destination ports to pick the tunnel protocol, based on what the act_key_tunnel is passing.

On Fri, Oct 5, 2018 at 12:58 AM Pablo Neira Ayuso <[email protected]> wrote:

Quoted 19 lines · Jakub Kicinski

Quoted 18 lines · Pablo Neira Ayuso

Quoted 9 lines

Hi,

The following patchset adds a new field to the tunnel metadata template to restrict the configuration to a given tunnel driver. Currently, a misconfiguration may result in packets going to the wrong tunnel driver.

Although we have the tunnel option flags, they are not mandatory for some tunnel drivers, eg. vxlan, which may use it or not; and gre which does not use them.

Option flags are necessary because interpretation of option blob is entirely protocol-specific.

Quoted 4 lines

This patch updates tc's tunnel action and netfilter's tunnel extension to use this new field. OVS netlink interface has been left unset, although they could be updated to use this.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

By extending the existing tc action to support the IP_TUNNEL_INFO_BRIDGE mode, I think it should be possible to expose IP_TUNNEL_TYPE_VLAN too, although this patchset doesn't address this scenario.

not following... can you elaborate further please?

Quoted 24 lines

Quoted 8 lines

Quoted 6 lines

The field is initialized to zero, which maps to IP_TUNNEL_TYPE_UNSPEC to retain the existing behaviour, so the existing flexibility is still in place while this new feature is added.

Cc'ing people that git annotate show as dealing with these bits more recently.

What practical scenario are you trying to address here?

Incorrect configuration. The tunnel template defines an ID field, this ID means vni in vxlan, but it also means session in erspan. If a packet that should go to vxlan tunnel device (to be encapsulated using vni 5) ends up in a gre/erspan device, you will get an erspan packet with session 5. With this new tunnel type field, you can restrict the tunnel template to work _only_ for a given tunnel device driver. Hence, if the packet ends up in the wrong tunnel device driver due to incorrect configuration, packet gets dropped.

Quoted 2 lines

Have you seen
https://www.mail-archive.com/[email protected]/msg250705.html ?

Hm, I remember to have seen some hw offload driver code that is making assumptions on the destination ports to pick the tunnel protocol, based on what the act_key_tunnel is passing.

[..]

for udp based tunneling this is valid practice, b/c a driver can get the tunnel type <--> udp dest port relation from the stack through the udp tunnel ndo.

HW offloading wise, I think it would be better first pursue Januk and Co proposal which deals with the problematic part of tc/tunnels -- ingress rules set on tunnel devices (decap rules). The RFC looks very promising and I understand this is going to be reposed as non-rfc early in the next cycle

Quoted 1 line · Or Gerlitz

On Fri, Oct 5, 2018 at 12:58 AM Pablo Neira Ayuso <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 19 lines · Jakub Kicinski

Quoted 18 lines · Pablo Neira Ayuso

Quoted 9 lines

Hi,

The following patchset adds a new field to the tunnel metadata template to restrict the configuration to a given tunnel driver. Currently, a misconfiguration may result in packets going to the wrong tunnel driver.

Although we have the tunnel option flags, they are not mandatory for some tunnel drivers, eg. vxlan, which may use it or not; and gre which does not use them.

Option flags are necessary because interpretation of option blob is entirely protocol-specific.

Quoted 4 lines

This patch updates tc's tunnel action and netfilter's tunnel extension to use this new field. OVS netlink interface has been left unset, although they could be updated to use this.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

By extending the existing tc action to support the IP_TUNNEL_INFO_BRIDGE mode, I think it should be possible to expose IP_TUNNEL_TYPE_VLAN too, although this patchset doesn't address this scenario.

not following... can you elaborate further please?

It should be possible to extend act_key_tunnel to support the IP_TUNNEL_INFO_BRIDGE flag, but this is just a suggestion.

Quoted 26 lines

Quoted 24 lines

Quoted 8 lines

Quoted 6 lines

The field is initialized to zero, which maps to IP_TUNNEL_TYPE_UNSPEC to retain the existing behaviour, so the existing flexibility is still in place while this new feature is added.

Cc'ing people that git annotate show as dealing with these bits more recently.

What practical scenario are you trying to address here?

Incorrect configuration. The tunnel template defines an ID field, this ID means vni in vxlan, but it also means session in erspan. If a packet that should go to vxlan tunnel device (to be encapsulated using vni 5) ends up in a gre/erspan device, you will get an erspan packet with session 5. With this new tunnel type field, you can restrict the tunnel template to work _only_ for a given tunnel device driver. Hence, if the packet ends up in the wrong tunnel device driver due to incorrect configuration, packet gets dropped.

Quoted 2 lines

Have you seen
https://www.mail-archive.com/[email protected]/msg250705.html ?

Hm, I remember to have seen some hw offload driver code that is making assumptions on the destination ports to pick the tunnel protocol, based on what the act_key_tunnel is passing.

[..]

Quoted 7 lines

for udp based tunneling this is valid practice, b/c a driver can get the tunnel type <--> udp dest port relation from the stack through the udp tunnel ndo.

HW offloading wise, I think it would be better first pursue Januk and Co proposal which deals with the problematic part of tc/tunnels -- ingress rules set on tunnel devices (decap rules). The RFC looks very promising and I understand this is going to be reposed as non-rfc early in the next cycle

Sorry, I think there's a misunderstanding here.

My patchset is of benefit for pure software/control plane approach:
This allows users to narrow down tunnel configurations, existing approach is very flexible - probably a bit too much for people willing to validate things are correct - since it is allowing too loose configurations. This is leaving room to the user to make configuration mistakes that result in bad things, such as packets being tunneled through the wrong encapsulation type.

through the wrong encapsulation type. With this patchset, packets going to the wrong tunnel devices will be simply dropped - and we could even do more specific validation from control plane after this. This patchset is backward compatible, so it doesn't restrict for the existing flexibility that users may want for this.

This patchset _never_ meant to replace Jakub's work nor any HW offload infrastructure. After reading Jakub's email, I was just suggesting that this may (probably) still help drivers too, since this would provide more hints to the driver.

provide more hints to the driver. Please, let me know if this is causing any interference with your ongoing HW driver development in some way, and if so, in what way.

Quoted 1 line · Pablo Neira Ayuso

On Sun, Oct 14, 2018 at 09:42:42AM +0300, Or Gerlitz wrote:

Quoted 10 lines

Quoted 7 lines

for udp based tunneling this is valid practice, b/c a driver can get the tunnel type <--> udp dest port relation from the stack through the udp tunnel ndo.

HW offloading wise, I think it would be better first pursue Januk and Co proposal which deals with the problematic part of tc/tunnels -- ingress rules set on tunnel devices (decap rules). The RFC looks very promising and I understand this is going to be reposed as non-rfc early in the next cycle

Sorry, I think there's a misunderstanding here.

Quoted 7 lines

My patchset is of benefit for pure software/control plane approach:
This allows users to narrow down tunnel configurations, existing approach is very flexible - probably a bit too much for people willing to validate things are correct - since it is allowing too loose configurations. This is leaving room to the user to make configuration mistakes that result in bad things, such as packets being tunneled through the wrong encapsulation type.

I see your point, but I think that the fact that effectively all or most of the IP based tunnels in the kernel conform to struct ip_tunnel_info is a plus and not a minus.. control planes can do their mistakes if they don't read the man, this wrong tunneling that you mentioned is not going to live far beyond the next networking hop or even dropped earlier, within the stack of this node.

Quoted 10 lines

through the wrong encapsulation type. With this patchset, packets going to the wrong tunnel devices will be simply dropped - and we could even do more specific validation from control plane after this. This patchset is backward compatible, so it doesn't restrict for the existing flexibility that users may want for this.

This patchset _never_ meant to replace Jakub's work nor any HW offload infrastructure. After reading Jakub's email, I was just suggesting that this may (probably) still help drivers too, since this would provide more hints to the driver.

well, yes, more hints and sort of no (see next)

Quoted 3 lines

provide more hints to the driver. Please, let me know if this is causing any interference with your ongoing HW driver development in some way, and if so, in what way.

for example, if newer controller wants to work over older kernel that doesn't have the new flag, they have to write code that can go both ways

, this is doable
-- but do we want to do that just for the sake of reacting to user mis-configurations?

I am open / will be happy to hear more opinions here.