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

Submitted by Krister Johansen on June 10, 2017, 1:31 a.m.

Details

Message ID 1497058292-10099-3-git-send-email-kjlx@templeofstupid.com
State New
Delegated to: stephen hemminger
Headers show

Commit Message

Krister Johansen June 10, 2017, 1:31 a.m.
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(-)

Comments

stephen hemminger June 14, 2017, 5:02 p.m.
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?
Krister Johansen June 14, 2017, 5:11 p.m.
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
David Ahern June 14, 2017, 5:16 p.m.
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.
Krister Johansen June 15, 2017, 6:31 p.m.
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 hide | download patch | download mbox

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