[PATCH iproute/master 0/3] lost mpls ip tunnel patches

[PATCH iproute/master 0/3] lost mpls ip tunnel patches

[Krister Johansen]

Hi Stephen,
I'm a bit unsure of the decorum in this particular situation. Kernel
support for mpls/ip tunnels was integrated back in July of 2016.  At the
time, the author of that feature sent out a RFC patch for the iproute
support but never followed up on subsequent code review comments.

The kernel support got merged, but the iproute support never made it in.
I wanted to run some tests with these features.  In the process, I
tracked down the author's original patches, merged them into current
iproute, and attempted to address the comments from code reviewers.

I've attached an 'Original-Author' label to each commit, and have CC'd
him and the code reviewer on this patch.  If any part of this is
improper, please let me know and I'll respin accordingly.  Mostly, I
wanted to close the loop here so the mpls in ip tunnel support is usable
through iproute.

The original threads for the 2016 patch are here:

http://marc.info/?l=linux-netdev&m=146782946216005&w=2
http://marc.info/?l=linux-netdev&m=146782941615977&w=2
http://marc.info/?l=linux-netdev&m=146782947016007&w=2
http://marc.info/?l=linux-netdev&m=146782942915988&w=2

Thanks,

-K

Krister Johansen (3):
  iptunnel: document mode parameter for sit tunnels
  iptunnel: add support for mpls/ip to sit tunnels
  iptunnel: add support for mpls/ip to ipip tunnels

 include/utils.h       |  3 +++
 ip/link_iptnl.c       | 30 ++++++++++++++++++++++++++----
 ip/tunnel.c           |  3 +++
 man/man8/ip-link.8.in | 12 +++++++++++-
 4 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH iproute/master 1/3] iptunnel: document mode parameter for sit tunnels

[Krister Johansen]

Original-Author: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 man/man8/ip-link.8.in | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 5d73538..3cc2f5d 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -660,7 +660,9 @@ the following additional arguments are supported:
 ] [
 .RB [ no ] encap-csum
 ] [
-.RB [ no ] encap-remcsum
+.I " [no]encap-remcsum "
+] [
+.I " mode " { ip6ip | ipip | any } "
 ]
 
 .in +8
@@ -697,6 +699,12 @@ encapsulation.
 - specifies if Remote Checksum Offload is enabled. This is only
 applicable for Generic UDP Encapsulation.
 
+.sp
+.BI mode " { ip6ip | ipip | any } "
+- specifies mode in which device should run. "ip6ip" indicates
+IPv6-Over-IPv4, "ipip" indicates "IPv4-Over-IPv4", "any" indicates either
+IPv6 or IPv4 Over IPv4. Only supported for SIT where the default is "ip6ip".
+
 .in -8
 
 .TP
-- 
2.7.4

[PATCH iproute/master 2/3] iptunnel: add support for mpls/ip to sit tunnels

[Krister Johansen]

Original-Author: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 include/utils.h       | 3 +++
 ip/link_iptnl.c       | 9 ++++++---
 ip/tunnel.c           | 3 +++
 man/man8/ip-link.8.in | 9 +++++----
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index bfbc9e6..60ffde4 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -87,6 +87,9 @@ struct ipx_addr {
 #ifndef AF_MPLS
 # define AF_MPLS 28
 #endif
+#ifndef IPPROTO_MPLS
+#define IPPROTO_MPLS	137
+#endif
 
 __u32 get_addr32(const char *name);
 int get_addr_1(inet_prefix *dst, const char *arg, int family);
diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index 2f74d9b..cf3a9ef 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -16,6 +16,7 @@
 #include <sys/socket.h>
 #include <arpa/inet.h>
 
+#include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/if_tunnel.h>
 #include "rt_names.h"
@@ -47,9 +48,8 @@ static void print_usage(FILE *f, int sit)
 		type
 	);
 	if (sit) {
-		fprintf(f,
-			"                [ mode { ip6ip | ipip | any } ]\n"
-			"                [ isatap ]\n");
+		fprintf(f, "          [ mode { ip6ip | ipip | mplsip | any } ]\n");
+		fprintf(f, "          [ isatap ]\n");
 	}
 	fprintf(f, "                [ external ]\n");
 	fprintf(f, "                [ fwmark MARK ]\n");
@@ -243,6 +243,9 @@ get_failed:
 				 strcmp(*argv, "ipip") == 0 ||
 				 strcmp(*argv, "ip4ip4") == 0)
 				proto = IPPROTO_IPIP;
+			else if (strcmp(*argv, "mpls/ipv4") == 0 ||
+				   strcmp(*argv, "mplsip") == 0)
+				proto = IPPROTO_MPLS;
 			else if (strcmp(*argv, "any/ipv4") == 0 ||
 				 strcmp(*argv, "any") == 0)
 				proto = 0;
diff --git a/ip/tunnel.c b/ip/tunnel.c
index 7956d71..d359eb9 100644
--- a/ip/tunnel.c
+++ b/ip/tunnel.c
@@ -54,6 +54,9 @@ const char *tnl_strproto(__u8 proto)
 	case IPPROTO_ESP:
 		strcpy(buf, "esp");
 		break;
+	case IPPROTO_MPLS:
+		strcpy(buf, "mpls");
+		break;
 	case 0:
 		strcpy(buf, "any");
 		break;
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 3cc2f5d..994b539 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -662,7 +662,7 @@ the following additional arguments are supported:
 ] [
 .I " [no]encap-remcsum "
 ] [
-.I " mode " { ip6ip | ipip | any } "
+.I " mode " { ip6ip | ipip | mplsip | any } "
 ]
 
 .in +8
@@ -700,10 +700,11 @@ encapsulation.
 applicable for Generic UDP Encapsulation.
 
 .sp
-.BI mode " { ip6ip | ipip | any } "
+.BI mode " { ip6ip | ipip | mplsip | any } "
 - specifies mode in which device should run. "ip6ip" indicates
-IPv6-Over-IPv4, "ipip" indicates "IPv4-Over-IPv4", "any" indicates either
-IPv6 or IPv4 Over IPv4. Only supported for SIT where the default is "ip6ip".
+IPv6-Over-IPv4, "ipip" indicates "IPv4-Over-IPv4", "mplsip" indicates
+MPLS-Over-IPv4, "any" indicates IPv6, IPv4 or MPLS Over IPv4. Only
+supported for SIT where the default is "ip6ip".
 
 .in -8
 
-- 
2.7.4

Re: [PATCH iproute/master 2/3] iptunnel: add support for mpls/ip to sit tunnels

[Stephen Hemminger]

On Fri,  9 Jun 2017 18:31:31 -0700
Krister Johansen <kjlx@templeofstupid.com> wrote:

> diff --git a/include/utils.h b/include/utils.h
> index bfbc9e6..60ffde4 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -87,6 +87,9 @@ struct ipx_addr {
>  #ifndef AF_MPLS
>  # define AF_MPLS 28
>  #endif
> +#ifndef IPPROTO_MPLS
> +#define IPPROTO_MPLS	137
> +#endif
>  

I am a little concerned that this definition may end up being different
between kernel and iproute2.  It looks like utils.h already has lots of duplicate
definitions of things that are in standard include directory.

Most of these like IPSEC and DECNET are old, but MPLS might get changed
in the process of going into glibc.

Is there anyway to get this from the kernel headers which are cloned
into include/linux/ already?

Re: [PATCH iproute/master 2/3] iptunnel: add support for mpls/ip to sit tunnels

[Krister Johansen]

On Wed, Jun 14, 2017 at 10:02:11AM -0700, Stephen Hemminger wrote:
> On Fri,  9 Jun 2017 18:31:31 -0700
> Krister Johansen <kjlx@templeofstupid.com> wrote:
> 
> > diff --git a/include/utils.h b/include/utils.h
> > index bfbc9e6..60ffde4 100644
> > --- a/include/utils.h
> > +++ b/include/utils.h
> > @@ -87,6 +87,9 @@ struct ipx_addr {
> >  #ifndef AF_MPLS
> >  # define AF_MPLS 28
> >  #endif
> > +#ifndef IPPROTO_MPLS
> > +#define IPPROTO_MPLS	137
> > +#endif
> >  
> 
> I am a little concerned that this definition may end up being different
> between kernel and iproute2.  It looks like utils.h already has lots of duplicate
> definitions of things that are in standard include directory.
> 
> Most of these like IPSEC and DECNET are old, but MPLS might get changed
> in the process of going into glibc.
> 
> Is there anyway to get this from the kernel headers which are cloned
> into include/linux/ already?

I did try to fix this up as part of bringing this patch up to date,
since it was one of the concerns that David raised too.  I believe the
problem that I ran into was that IPPROTO_MPLS wasn't defined in all
versions of the headers where I tried to include them, and by bringing
in in.h, I also managed to get a bunch of errors around re-definition of
other symbols.

That said, I don't believe that 137 as the IPPROTO_MPLS value should
change anytime soon.  It's defined in RFC 4023.

	https://tools.ietf.org/html/rfc4023

However, if this is still seems problematic, I can take another shot at
attempting to clean this up further.

-K

Re: [PATCH iproute/master 2/3] iptunnel: add support for mpls/ip to sit tunnels

[David Ahern]

On 6/14/17 11:11 AM, Krister Johansen wrote:
> I did try to fix this up as part of bringing this patch up to date,
> since it was one of the concerns that David raised too.  I believe the
> problem that I ran into was that IPPROTO_MPLS wasn't defined in all
> versions of the headers where I tried to include them, and by bringing
> in in.h, I also managed to get a bunch of errors around re-definition of
> other symbols.
> 
> That said, I don't believe that 137 as the IPPROTO_MPLS value should
> change anytime soon.  It's defined in RFC 4023.
> 
> 	https://tools.ietf.org/html/rfc4023
> 
> However, if this is still seems problematic, I can take another shot at
> attempting to clean this up further.

IPPROTO_MPLS is defined in include/linux/in.h in iproute2.

Re: [PATCH iproute/master 2/3] iptunnel: add support for mpls/ip to sit tunnels

[Krister Johansen]

On Wed, Jun 14, 2017 at 11:16:20AM -0600, David Ahern wrote:
> On 6/14/17 11:11 AM, Krister Johansen wrote:
> > I did try to fix this up as part of bringing this patch up to date,
> > since it was one of the concerns that David raised too.  I believe the
> > problem that I ran into was that IPPROTO_MPLS wasn't defined in all
> > versions of the headers where I tried to include them, and by bringing
> > in in.h, I also managed to get a bunch of errors around re-definition of
> > other symbols.
> > 
> > That said, I don't believe that 137 as the IPPROTO_MPLS value should
> > change anytime soon.  It's defined in RFC 4023.
> > 
> > 	https://tools.ietf.org/html/rfc4023
> > 
> > However, if this is still seems problematic, I can take another shot at
> > attempting to clean this up further.
> 
> IPPROTO_MPLS is defined in include/linux/in.h in iproute2.

Right.  I poked at this some more yesterday afternoon.

The IPPROTO enum in include/linux/in.h is wrapped in an #if
__UAPI_DEF_IN_IPPROTO.  That's defined in include/linux/libc-compat.h.
If _NETINET_IN_H is defined, then __UAPI_DEF_IN_IPPROTO is undefined.
In that case, we pick up the the IPPROTO defs from netinet/in.h.  The
addition of IPPROTO_MPLS to utils.h is to cover the case where we're
using a system netinet/in.h that's older and doesn't yet have an
IPPROTO_MPLS defintion.  There seems to be prescedent for this, since
utils.h also includes IPPROTO_ESP, IPPROTO_AH, and IPPROTO_COMP.

I spent some time trying to remove includes of netinet/in.h from the
code in this patch, however, it's also included by other system headers
that are included throughout iproute2.  I'm not saying removing this is
impossible, but it certainly seems like it's beyond the scope of this
particular patch.

-K

[PATCH iproute/master 3/3] iptunnel: add support for mpls/ip to ipip tunnels

[Krister Johansen]

Original-Author: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 ip/link_iptnl.c       | 21 ++++++++++++++++++++-
 man/man8/ip-link.8.in |  5 +++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index cf3a9ef..d24e737 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -50,6 +50,8 @@ static void print_usage(FILE *f, int sit)
 	if (sit) {
 		fprintf(f, "          [ mode { ip6ip | ipip | mplsip | any } ]\n");
 		fprintf(f, "          [ isatap ]\n");
+	} else {
+		fprintf(f, "          [ mode { ipip | mplsip | any } ]\n");
 	}
 	fprintf(f, "                [ external ]\n");
 	fprintf(f, "                [ fwmark MARK ]\n");
@@ -251,6 +253,21 @@ get_failed:
 				proto = 0;
 			else
 				invarg("Cannot guess tunnel mode.", *argv);
+		} else if (strcmp(lu->id, "ipip") == 0 &&
+			   strcmp(*argv, "mode") == 0) {
+			NEXT_ARG();
+			if (strcmp(*argv, "ipv4/ipv4") == 0 ||
+				 strcmp(*argv, "ipip") == 0 ||
+				 strcmp(*argv, "ip4ip4") == 0)
+				proto = IPPROTO_IPIP;
+			else if (strcmp(*argv, "mpls/ipv4") == 0 ||
+				   strcmp(*argv, "mplsip") == 0)
+				proto = IPPROTO_MPLS;
+			else if (strcmp(*argv, "any/ipv4") == 0 ||
+				 strcmp(*argv, "any") == 0)
+				proto = 0;
+			else
+				invarg("Cannot guess tunnel mode.", *argv);
 		} else if (strcmp(*argv, "noencap") == 0) {
 			encaptype = TUNNEL_ENCAP_NONE;
 		} else if (strcmp(*argv, "encap") == 0) {
@@ -343,9 +360,11 @@ get_failed:
 	addattr16(n, 1024, IFLA_IPTUN_ENCAP_SPORT, htons(encapsport));
 	addattr16(n, 1024, IFLA_IPTUN_ENCAP_DPORT, htons(encapdport));
 
+	if (strcmp(lu->id, "ipip") == 0 || strcmp(lu->id, "sit") == 0)
+		addattr8(n, 1024, IFLA_IPTUN_PROTO, proto);
+
 	if (strcmp(lu->id, "sit") == 0) {
 		addattr16(n, 1024, IFLA_IPTUN_FLAGS, iflags);
-		addattr8(n, 1024, IFLA_IPTUN_PROTO, proto);
 		if (ip6rdprefixlen) {
 			addattr_l(n, 1024, IFLA_IPTUN_6RD_PREFIX,
 				  &ip6rdprefix, sizeof(ip6rdprefix));
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 994b539..a782712 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -703,8 +703,9 @@ applicable for Generic UDP Encapsulation.
 .BI mode " { ip6ip | ipip | mplsip | any } "
 - specifies mode in which device should run. "ip6ip" indicates
 IPv6-Over-IPv4, "ipip" indicates "IPv4-Over-IPv4", "mplsip" indicates
-MPLS-Over-IPv4, "any" indicates IPv6, IPv4 or MPLS Over IPv4. Only
-supported for SIT where the default is "ip6ip".
+MPLS-Over-IPv4, "any" indicates IPv6, IPv4 or MPLS Over IPv4. Supported for
+SIT where the default is "ip6ip" and IPIP where the default is "ipip".
+IPv6-Over-IPv4 is not supported for IPIP.
 
 .in -8
 
-- 
2.7.4

Re: [PATCH iproute/master 0/3] lost mpls ip tunnel patches

[Stephen Hemminger]

On Fri,  9 Jun 2017 18:31:29 -0700
Krister Johansen <kjlx@templeofstupid.com> wrote:

> Hi Stephen,
> I'm a bit unsure of the decorum in this particular situation. Kernel
> support for mpls/ip tunnels was integrated back in July of 2016.  At the
> time, the author of that feature sent out a RFC patch for the iproute
> support but never followed up on subsequent code review comments.
> 
> The kernel support got merged, but the iproute support never made it in.
> I wanted to run some tests with these features.  In the process, I
> tracked down the author's original patches, merged them into current
> iproute, and attempted to address the comments from code reviewers.
>

In order to get merged, an RFC message should be followed by a final
non RFC version. I think you will find the original's were marked
in patchwork as RFC.

Re: [PATCH iproute/master 0/3] lost mpls ip tunnel patches

[Krister Johansen]

On Wed, Jun 14, 2017 at 10:24:51AM -0700, Stephen Hemminger wrote:
> On Fri,  9 Jun 2017 18:31:29 -0700
> Krister Johansen <kjlx@templeofstupid.com> wrote:
> 
> > Hi Stephen,
> > I'm a bit unsure of the decorum in this particular situation. Kernel
> > support for mpls/ip tunnels was integrated back in July of 2016.  At the
> > time, the author of that feature sent out a RFC patch for the iproute
> > support but never followed up on subsequent code review comments.
> > 
> > The kernel support got merged, but the iproute support never made it in.
> > I wanted to run some tests with these features.  In the process, I
> > tracked down the author's original patches, merged them into current
> > iproute, and attempted to address the comments from code reviewers.
> >
> 
> In order to get merged, an RFC message should be followed by a final
> non RFC version. I think you will find the original's were marked
> in patchwork as RFC.

Yes, absolutely true.  I was just unsure of the protocol for getting
these merged when I'm not the original author.  Trying to ensure that
credit is given where it is due, and I'm following the appropriate
procedures.

Thanks,

-K

Re: [PATCH iproute/master 0/3] lost mpls ip tunnel patches

[Stephen Hemminger]

On Fri,  9 Jun 2017 18:31:29 -0700
Krister Johansen <kjlx@templeofstupid.com> wrote:

> Hi Stephen,
> I'm a bit unsure of the decorum in this particular situation. Kernel
> support for mpls/ip tunnels was integrated back in July of 2016.  At the
> time, the author of that feature sent out a RFC patch for the iproute
> support but never followed up on subsequent code review comments.
> 
> The kernel support got merged, but the iproute support never made it in.
> I wanted to run some tests with these features.  In the process, I
> tracked down the author's original patches, merged them into current
> iproute, and attempted to address the comments from code reviewers.
> 
> I've attached an 'Original-Author' label to each commit, and have CC'd
> him and the code reviewer on this patch.  If any part of this is
> improper, please let me know and I'll respin accordingly.  Mostly, I
> wanted to close the loop here so the mpls in ip tunnel support is usable
> through iproute.
> 
> The original threads for the 2016 patch are here:
> 
> http://marc.info/?l=linux-netdev&m=146782946216005&w=2
> http://marc.info/?l=linux-netdev&m=146782941615977&w=2
> http://marc.info/?l=linux-netdev&m=146782947016007&w=2
> http://marc.info/?l=linux-netdev&m=146782942915988&w=2
> 
> Thanks,
> 
> -K
> 
> Krister Johansen (3):
>   iptunnel: document mode parameter for sit tunnels
>   iptunnel: add support for mpls/ip to sit tunnels
>   iptunnel: add support for mpls/ip to ipip tunnels
> 
>  include/utils.h       |  3 +++
>  ip/link_iptnl.c       | 30 ++++++++++++++++++++++++++----
>  ip/tunnel.c           |  3 +++
>  man/man8/ip-link.8.in | 12 +++++++++++-
>  4 files changed, 43 insertions(+), 5 deletions(-)
> 

I went ahead and merged these in. Would be better to get the headers
stuff straightened out but that is probably an endless battle.