[PATCH iproute2 0/3] ip/tunnel: Fix noencap- and document "external" parameter handling.

[PATCH iproute2 0/3] ip/tunnel: Fix noencap- and document "external" parameter handling.

[Serhey Popovych]

In this series I present next set of improvements/fixes:

  1) Fix noencap- option handling: we need to clear bit, instead
     of seting all, but one we expect to clear.

  2) Document "external" parameter both in ip-link(8) and help
     output for link_gre.c. Add "noexternal" option variant to
     bring inline with GENEVE and VXLAN.

  3) Trivial: clear flowlabel/tclass from flowinfo in case of
     inherit to stop sending garbage to the kernel. It has no
     functional change, but follows similar behaviour in link_ip6tnl.c

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (3):
  gre,ip6tnl/tunnel: Fix noencap- support
  gre6/tunnel: Do not submit garbage in flowinfo
  ip/tunnel: Document "external" parameter

 ip/link_gre.c         |    7 +++++--
 ip/link_gre6.c        |    4 ++--
 ip/link_ip6tnl.c      |    6 ++++--
 ip/link_iptnl.c       |    4 +++-
 man/man8/ip-link.8.in |    6 ++++++
 5 files changed, 20 insertions(+), 7 deletions(-)

-- 
1.7.10.4

[PATCH iproute2 1/3] gre,ip6tnl/tunnel: Fix noencap- support

[Serhey Popovych]

We must clear bit, not set all but given bit.

Fixes: 858dbb208e39 ("ip link: Add support for remote checksum offload to IP tunnels")
Fixes: 73516e128a5a ("ip6tnl: Support for fou encapsulation"
Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c    |    4 ++--
 ip/link_ip6tnl.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index f55c40c..896bb19 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -281,11 +281,11 @@ get_failed:
 		} else if (strcmp(*argv, "encap-udp6-csum") == 0) {
 			encapflags |= TUNNEL_ENCAP_FLAG_CSUM6;
 		} else if (strcmp(*argv, "noencap-udp6-csum") == 0) {
-			encapflags |= ~TUNNEL_ENCAP_FLAG_CSUM6;
+			encapflags &= ~TUNNEL_ENCAP_FLAG_CSUM6;
 		} else if (strcmp(*argv, "encap-remcsum") == 0) {
 			encapflags |= TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "noencap-remcsum") == 0) {
-			encapflags |= ~TUNNEL_ENCAP_FLAG_REMCSUM;
+			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "external") == 0) {
 			metadata = 1;
 		} else if (strcmp(*argv, "ignore-df") == 0) {
diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index 0a471c2..84205b1 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -301,7 +301,7 @@ get_failed:
 		} else if (strcmp(*argv, "encap-remcsum") == 0) {
 			encapflags |= TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "noencap-remcsum") == 0) {
-			encapflags |= ~TUNNEL_ENCAP_FLAG_REMCSUM;
+			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "external") == 0) {
 			metadata = 1;
 		} else
-- 
1.7.10.4

[PATCH iproute2 2/3] gre6/tunnel: Do not submit garbage in flowinfo

[Serhey Popovych]

We always send flowinfo to the kernel. If flowlabel/tclass
was set first to non-inherit value and then reset to
inherit we do not clear flowlabel/tclass part in flowinfo,
send it to kernel and can get from the kernel back.

Even if we check for IP6_TNL_F_USE_ORIG_TCLASS and
IP6_TNL_F_USE_ORIG_FLOWLABEL when printing options
sending invalid flowlabel/tclass to the kernel seems
bad idea.

Note that ip6tnl always clean corresponding flowinfo
parts on inherit.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre6.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index a3e8e08..7ae4b49 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -251,12 +251,12 @@ get_failed:
 			__u8 uval;
 
 			NEXT_ARG();
+			flowinfo &= ~IP6_FLOWINFO_TCLASS;
 			if (strcmp(*argv, "inherit") == 0)
 				flags |= IP6_TNL_F_USE_ORIG_TCLASS;
 			else {
 				if (get_u8(&uval, *argv, 16))
 					invarg("invalid TClass", *argv);
-				flowinfo &= ~IP6_FLOWINFO_TCLASS;
 				flowinfo |= htonl((__u32)uval << 20) & IP6_FLOWINFO_TCLASS;
 				flags &= ~IP6_TNL_F_USE_ORIG_TCLASS;
 			}
@@ -265,6 +265,7 @@ get_failed:
 			__u32 uval;
 
 			NEXT_ARG();
+			flowinfo &= ~IP6_FLOWINFO_FLOWLABEL;
 			if (strcmp(*argv, "inherit") == 0)
 				flags |= IP6_TNL_F_USE_ORIG_FLOWLABEL;
 			else {
@@ -272,7 +273,6 @@ get_failed:
 					invarg("invalid Flowlabel", *argv);
 				if (uval > 0xFFFFF)
 					invarg("invalid Flowlabel", *argv);
-				flowinfo &= ~IP6_FLOWINFO_FLOWLABEL;
 				flowinfo |= htonl(uval) & IP6_FLOWINFO_FLOWLABEL;
 				flags &= ~IP6_TNL_F_USE_ORIG_FLOWLABEL;
 			}
-- 
1.7.10.4

[PATCH iproute2 3/3] ip/tunnel: Document "external" parameter

[Serhey Popovych]

Also add "noexternal" variant to be inline
with geneve and vxlan tunnel types.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c         |    3 +++
 ip/link_ip6tnl.c      |    4 +++-
 ip/link_iptnl.c       |    4 +++-
 man/man8/ip-link.8.in |    6 ++++++
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 896bb19..b383061 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -43,6 +43,7 @@ static void print_usage(FILE *f)
 		"                            [ [no]encap-csum ]\n"
 		"                            [ [no]encap-csum6 ]\n"
 		"                            [ [no]encap-remcsum ]\n"
+		"                            [ [no]external ]\n"
 		"                            [ fwmark MARK ]\n"
 		"                            [ erspan IDX ]\n"
 		"\n"
@@ -288,6 +289,8 @@ get_failed:
 			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "external") == 0) {
 			metadata = 1;
+		} else if (strcmp(*argv, "noexternal") == 0) {
+			metadata = 0;
 		} else if (strcmp(*argv, "ignore-df") == 0) {
 			ignore_df = 1;
 		} else if (strcmp(*argv, "noignore-df") == 0) {
diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index 84205b1..9efe8a1 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -50,7 +50,7 @@ static void print_usage(FILE *f)
 		"                  [ [no]encap-csum ]\n"
 		"                  [ [no]encap-csum6 ]\n"
 		"                  [ [no]encap-remcsum ]\n"
-		"                  [ external ]\n"
+		"                  [ [no]external ]\n"
 		"\n"
 		"Where: ADDR      := IPV6_ADDRESS\n"
 		"       ELIM      := { none | 0..255 }(default=%d)\n"
@@ -304,6 +304,8 @@ get_failed:
 			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "external") == 0) {
 			metadata = 1;
+		} else if (strcmp(*argv, "noexternal") == 0) {
+			metadata = 0;
 		} else
 			usage();
 		argc--, argv++;
diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index 8a8f5dd..6a76721 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -53,7 +53,7 @@ static void print_usage(FILE *f, int sit)
 	} else {
 		fprintf(f, "          [ mode { ipip | mplsip | any } ]\n");
 	}
-	fprintf(f, "                [ external ]\n");
+	fprintf(f, "                [ [no]external ]\n");
 	fprintf(f, "                [ fwmark MARK ]\n");
 	fprintf(f, "\n");
 	fprintf(f, "Where: ADDR := { IP_ADDRESS | any }\n");
@@ -299,6 +299,8 @@ get_failed:
 			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "external") == 0) {
 			metadata = 1;
+		} else if (strcmp(*argv, "noexternal") == 0) {
+			metadata = 0;
 		} else if (strcmp(*argv, "6rd-prefix") == 0) {
 			inet_prefix prefix;
 
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 94ecbec..d7aa45d 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -698,6 +698,8 @@ the following additional arguments are supported:
 .I " mode " { ip6ip | ipip | mplsip | any } "
 ] [
 .BR erspan " \fIIDX "
+] [
+.RB [ no ] external
 ]
 
 .in +8
@@ -749,6 +751,10 @@ IPv6-Over-IPv4 is not supported for IPIP.
 indicates a 20 bit index/port number associated with the ERSPAN
 traffic's source port and direction.
 
+.sp
+.RB [ no ] external
+- make this tunnel externally controlled (or not, which is the default).
+
 .in -8
 
 .TP
-- 
1.7.10.4

Re: [PATCH iproute2 3/3] ip/tunnel: Document "external" parameter

[William Tu]

Hi Serhey,

On Wed, Dec 27, 2017 at 3:28 AM, Serhey Popovych
<serhe.popovych@gmail.com> wrote:
> Also add "noexternal" variant to be inline
> with geneve and vxlan tunnel types.
>
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
> ---
>  ip/link_gre.c         |    3 +++
>  ip/link_ip6tnl.c      |    4 +++-
>  ip/link_iptnl.c       |    4 +++-
>  man/man8/ip-link.8.in |    6 ++++++
>  4 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/ip/link_gre.c b/ip/link_gre.c
> index 896bb19..b383061 100644
> --- a/ip/link_gre.c
> +++ b/ip/link_gre.c
> @@ -43,6 +43,7 @@ static void print_usage(FILE *f)
>                 "                            [ [no]encap-csum ]\n"
>                 "                            [ [no]encap-csum6 ]\n"
>                 "                            [ [no]encap-remcsum ]\n"
> +               "                            [ [no]external ]\n"
>                 "                            [ fwmark MARK ]\n"
>                 "                            [ erspan IDX ]\n"
>                 "\n"
> @@ -288,6 +289,8 @@ get_failed:
>                         encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
>                 } else if (strcmp(*argv, "external") == 0) {
>                         metadata = 1;
> +               } else if (strcmp(*argv, "noexternal") == 0) {
> +                       metadata = 0;
>                 } else if (strcmp(*argv, "ignore-df") == 0) {
>                         ignore_df = 1;
>                 } else if (strcmp(*argv, "noignore-df") == 0) {

What's the use case of noexternal?
AFAIK, setting 'noexternal' to the existing external device won't
change it to noexternal.
And if the device is created first time, then the default is noexternal.

William

Re: [PATCH iproute2 0/3] ip/tunnel: Fix noencap- and document "external" parameter handling.

[Stephen Hemminger]

On Wed, 27 Dec 2017 13:28:13 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> In this series I present next set of improvements/fixes:
> 
>   1) Fix noencap- option handling: we need to clear bit, instead
>      of seting all, but one we expect to clear.
> 
>   2) Document "external" parameter both in ip-link(8) and help
>      output for link_gre.c. Add "noexternal" option variant to
>      bring inline with GENEVE and VXLAN.
> 
>   3) Trivial: clear flowlabel/tclass from flowinfo in case of
>      inherit to stop sending garbage to the kernel. It has no
>      functional change, but follows similar behaviour in link_ip6tnl.c
> 
> See individual patch description message for details.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (3):
>   gre,ip6tnl/tunnel: Fix noencap- support
>   gre6/tunnel: Do not submit garbage in flowinfo
>   ip/tunnel: Document "external" parameter
> 
>  ip/link_gre.c         |    7 +++++--
>  ip/link_gre6.c        |    4 ++--
>  ip/link_ip6tnl.c      |    6 ++++--
>  ip/link_iptnl.c       |    4 +++-
>  man/man8/ip-link.8.in |    6 ++++++
>  5 files changed, 20 insertions(+), 7 deletions(-)
> 

These are really disjoint. I applied the noencap and the flowinfo patch.
Agree with William that having noexternal which does nothing useful is of little value now.

Re: [PATCH iproute2 0/3] ip/tunnel: Fix noencap- and document "external" parameter handling.

[Serhey Popovych]

[-- Attachment #1.1: Type: text/plain, Size: 1987 bytes --]



Stephen Hemminger wrote:
> On Wed, 27 Dec 2017 13:28:13 +0200
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> In this series I present next set of improvements/fixes:
>>
>>   1) Fix noencap- option handling: we need to clear bit, instead
>>      of seting all, but one we expect to clear.
>>
>>   2) Document "external" parameter both in ip-link(8) and help
>>      output for link_gre.c. Add "noexternal" option variant to
>>      bring inline with GENEVE and VXLAN.
>>
>>   3) Trivial: clear flowlabel/tclass from flowinfo in case of
>>      inherit to stop sending garbage to the kernel. It has no
>>      functional change, but follows similar behaviour in link_ip6tnl.c
>>
>> See individual patch description message for details.
>>
>> Thanks,
>> Serhii
>>
>> Serhey Popovych (3):
>>   gre,ip6tnl/tunnel: Fix noencap- support
>>   gre6/tunnel: Do not submit garbage in flowinfo
>>   ip/tunnel: Document "external" parameter
>>
>>  ip/link_gre.c         |    7 +++++--
>>  ip/link_gre6.c        |    4 ++--
>>  ip/link_ip6tnl.c      |    6 ++++--
>>  ip/link_iptnl.c       |    4 +++-
>>  man/man8/ip-link.8.in |    6 ++++++
>>  5 files changed, 20 insertions(+), 7 deletions(-)
>>
> 
> These are really disjoint. I applied the noencap and the flowinfo patch.
> Agree with William that having noexternal which does nothing useful is of little value now.

Agree with you guys, just added it to match GENEVE/VXLAN implementation
which is has "noexternal".

Will send v2 to just document "external" and clear flowlabel/tclass in
flowinfo in case of inherit.

I have plans for iproute2-next to merge link_{gre,vti}{,6}.c and
link_ip{6,}tnl.c functionality together and just want to do things
as clear as possible before doing merge.

This is to reduce size of ip(8) binary to help openwrt like solutions to
survive JSON print support that increases size of binary.

Thank you for quick review and good feed back!

> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH iproute2 3/3] ip/tunnel: Document "external" parameter

[Serhey Popovych]

Add it to ip-link(8) "type gre" output help message
as well as to ip-link(8) page.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c         |    1 +
 man/man8/ip-link.8.in |    7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 896bb19..3c0b6d6 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -43,6 +43,7 @@ static void print_usage(FILE *f)
 		"                            [ [no]encap-csum ]\n"
 		"                            [ [no]encap-csum6 ]\n"
 		"                            [ [no]encap-remcsum ]\n"
+		"                            [ external ]\n"
 		"                            [ fwmark MARK ]\n"
 		"                            [ erspan IDX ]\n"
 		"\n"
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 94ecbec..9ddf0ff 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -698,6 +698,8 @@ the following additional arguments are supported:
 .I " mode " { ip6ip | ipip | mplsip | any } "
 ] [
 .BR erspan " \fIIDX "
+] [
+.BR external
 ]
 
 .in +8
@@ -749,6 +751,11 @@ IPv6-Over-IPv4 is not supported for IPIP.
 indicates a 20 bit index/port number associated with the ERSPAN
 traffic's source port and direction.
 
+.sp
+.BR external
+- make this tunnel externally controlled
+.RB "(e.g. " "ip route encap" ).
+
 .in -8
 
 .TP
-- 
1.7.10.4

Re: [PATCH iproute2 3/3] ip/tunnel: Document "external" parameter

[Stephen Hemminger]

On Thu, 28 Dec 2017 13:11:42 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> Add it to ip-link(8) "type gre" output help message
> as well as to ip-link(8) page.
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>

Applied. Thanks