[PATCH iproute2 0/3] Improve tunnel local/remote endpoint params and gre link attribute handling

[PATCH iproute2 0/3] Improve tunnel local/remote endpoint params and gre link attribute handling

[Serhey Popovych]

In this series following problems addressed:

  1) Size of IFLA_GRE_LINK attribute is 32 bits long in , not 8 bit.

  2) Use get_addr() instead of get_prefix() to parse local/remote
     tunnel endpoints as IPADDR, not PREFIX as per ip-link(8).

  3) No need to check if local/remote endpoints are zero (e.g. INADDR_ANY):
     it is fully legal value, accepted by the kernel.

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (3):
  ip/tunnel: Unify setup and accept zero address for local/remote
    endpoints
  ip/tunnel: Use get_addr() instead of get_prefix() for local/remote
    endpoints
  ip: gre: fix IFLA_GRE_LINK attribute sizing

 ip/ip6tunnel.c   |    8 ++------
 ip/iptunnel.c    |   10 ++--------
 ip/link_gre.c    |    8 +++-----
 ip/link_gre6.c   |    8 ++------
 ip/link_ip6tnl.c |   12 ++++--------
 ip/link_iptnl.c  |   10 ++--------
 ip/link_vti.c    |   14 ++------------
 ip/link_vti6.c   |   26 ++++++++------------------
 8 files changed, 25 insertions(+), 71 deletions(-)

-- 
1.7.10.4

[PATCH iproute2 1/3] ip/tunnel: Unify setup and accept zero address for local/remote endpoints

[Serhey Popovych]

It is fully legal to submit zero (INADDR_ANY/IN6ADDR_ANY_INIT)
value for local and/or remote endpoints for all tunnel drivers:
no need additionally check this in userspace.

Note that all tunnel specific code already can pass zero address
to the kernel.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/iptunnel.c   |   10 ++--------
 ip/link_gre.c   |    6 ++----
 ip/link_iptnl.c |   10 ++--------
 ip/link_vti.c   |   14 ++------------
 ip/link_vti6.c  |   26 ++++++++------------------
 5 files changed, 16 insertions(+), 50 deletions(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 208a1f0..ce610f8 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -127,16 +127,10 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 			p->iph.frag_off = htons(IP_DF);
 		} else if (strcmp(*argv, "remote") == 0) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				p->iph.daddr = get_addr32(*argv);
-			else
-				p->iph.daddr = htonl(INADDR_ANY);
+			p->iph.daddr = get_addr32(*argv);
 		} else if (strcmp(*argv, "local") == 0) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				p->iph.saddr = get_addr32(*argv);
-			else
-				p->iph.saddr = htonl(INADDR_ANY);
+			p->iph.saddr = get_addr32(*argv);
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
 			medium = *argv;
diff --git a/ip/link_gre.c b/ip/link_gre.c
index 43cb1af..6f82fb4 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -251,12 +251,10 @@ get_failed:
 			pmtudisc = 1;
 		} else if (!matches(*argv, "remote")) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				daddr = get_addr32(*argv);
+			daddr = get_addr32(*argv);
 		} else if (!matches(*argv, "local")) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				saddr = get_addr32(*argv);
+			saddr = get_addr32(*argv);
 		} else if (!matches(*argv, "dev")) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index 4940b8b..521d6ba 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -195,16 +195,10 @@ get_failed:
 	while (argc > 0) {
 		if (strcmp(*argv, "remote") == 0) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				raddr = get_addr32(*argv);
-			else
-				raddr = 0;
+			raddr = get_addr32(*argv);
 		} else if (strcmp(*argv, "local") == 0) {
 			NEXT_ARG();
-			if (strcmp(*argv, "any"))
-				laddr = get_addr32(*argv);
-			else
-				laddr = 0;
+			laddr = get_addr32(*argv);
 		} else if (matches(*argv, "dev") == 0) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
diff --git a/ip/link_vti.c b/ip/link_vti.c
index 07ac94e..05aefa3 100644
--- a/ip/link_vti.c
+++ b/ip/link_vti.c
@@ -167,20 +167,10 @@ get_failed:
 			okey = uval;
 		} else if (!matches(*argv, "remote")) {
 			NEXT_ARG();
-			if (!strcmp(*argv, "any")) {
-				fprintf(stderr, "invalid value for \"remote\": \"%s\"\n", *argv);
-				exit(-1);
-			} else {
-				daddr = get_addr32(*argv);
-			}
+			daddr = get_addr32(*argv);
 		} else if (!matches(*argv, "local")) {
 			NEXT_ARG();
-			if (!strcmp(*argv, "any")) {
-				fprintf(stderr, "invalid value for \"local\": \"%s\"\n", *argv);
-				exit(-1);
-			} else {
-				saddr = get_addr32(*argv);
-			}
+			saddr = get_addr32(*argv);
 		} else if (!matches(*argv, "dev")) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
diff --git a/ip/link_vti6.c b/ip/link_vti6.c
index 6d08bfe..f665520 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -161,27 +161,17 @@ get_failed:
 			}
 			okey = uval;
 		} else if (!matches(*argv, "remote")) {
-			NEXT_ARG();
-			if (!strcmp(*argv, "any")) {
-				fprintf(stderr, "invalid value for \"remote\": \"%s\"\n", *argv);
-				exit(-1);
-			} else {
-				inet_prefix addr;
+			inet_prefix addr;
 
-				get_prefix(&addr, *argv, AF_INET6);
-				memcpy(&daddr, addr.data, addr.bytelen);
-			}
-		} else if (!matches(*argv, "local")) {
 			NEXT_ARG();
-			if (!strcmp(*argv, "any")) {
-				fprintf(stderr, "invalid value for \"local\": \"%s\"\n", *argv);
-				exit(-1);
-			} else {
-				inet_prefix addr;
+			get_prefix(&addr, *argv, AF_INET6);
+			memcpy(&daddr, addr.data, addr.bytelen);
+		} else if (!matches(*argv, "local")) {
+			inet_prefix addr;
 
-				get_prefix(&addr, *argv, AF_INET6);
-				memcpy(&saddr, addr.data, addr.bytelen);
-			}
+			NEXT_ARG();
+			get_prefix(&addr, *argv, AF_INET6);
+			memcpy(&saddr, addr.data, addr.bytelen);
 		} else if (!matches(*argv, "dev")) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
-- 
1.7.10.4

[PATCH iproute2 2/3] ip/tunnel: Use get_addr() instead of get_prefix() for local/remote endpoints

[Serhey Popovych]

Manual page ip-link(8) states that both local and remote accept
IPADDR not PREFIX. Use get_addr() instead of get_prefix() to
parse local/remote endpoint address correctly.

Force corresponding address family instead of using preferred_family
to catch weired cases as shown below.

Before this patch it is possible to create tunnel with commands:

  ip    li add dev ip6gre2 type ip6gre local fe80::1/64 remote fe80::2/64
  ip -4 li add dev ip6gre2 type ip6gre local 10.0.0.1/24 remote 10.0.0.2/24

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/ip6tunnel.c   |    8 ++------
 ip/link_gre6.c   |    8 ++------
 ip/link_ip6tnl.c |   12 ++++--------
 ip/link_vti6.c   |    8 ++++----
 4 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 4563e1e..b8db49c 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -170,17 +170,13 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 			inet_prefix raddr;
 
 			NEXT_ARG();
-			get_prefix(&raddr, *argv, preferred_family);
-			if (raddr.family == AF_UNSPEC)
-				invarg("\"remote\" address family is AF_UNSPEC", *argv);
+			get_addr(&raddr, *argv, AF_INET6);
 			memcpy(&p->raddr, &raddr.data, sizeof(p->raddr));
 		} else if (strcmp(*argv, "local") == 0) {
 			inet_prefix laddr;
 
 			NEXT_ARG();
-			get_prefix(&laddr, *argv, preferred_family);
-			if (laddr.family == AF_UNSPEC)
-				invarg("\"local\" address family is AF_UNSPEC", *argv);
+			get_addr(&laddr, *argv, AF_INET6);
 			memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index 0a82eae..c22fded 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -257,17 +257,13 @@ get_failed:
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, preferred_family);
-			if (addr.family == AF_UNSPEC)
-				invarg("\"remote\" address family is AF_UNSPEC", *argv);
+			get_addr(&addr, *argv, AF_INET6);
 			memcpy(&raddr, &addr.data, sizeof(raddr));
 		} else if (!matches(*argv, "local")) {
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, preferred_family);
-			if (addr.family == AF_UNSPEC)
-				invarg("\"local\" address family is AF_UNSPEC", *argv);
+			get_addr(&addr, *argv, AF_INET6);
 			memcpy(&laddr, &addr.data, sizeof(laddr));
 		} else if (!matches(*argv, "dev")) {
 			NEXT_ARG();
diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index 43287ab..4ab337f 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -184,18 +184,14 @@ get_failed:
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, preferred_family);
-			if (addr.family == AF_UNSPEC)
-				invarg("\"remote\" address family is AF_UNSPEC", *argv);
-			memcpy(&raddr, addr.data, addr.bytelen);
+			get_addr(&addr, *argv, AF_INET6);
+			memcpy(&raddr, addr.data, sizeof(raddr));
 		} else if (strcmp(*argv, "local") == 0) {
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, preferred_family);
-			if (addr.family == AF_UNSPEC)
-				invarg("\"local\" address family is AF_UNSPEC", *argv);
-			memcpy(&laddr, addr.data, addr.bytelen);
+			get_addr(&addr, *argv, AF_INET6);
+			memcpy(&laddr, addr.data, sizeof(laddr));
 		} else if (matches(*argv, "dev") == 0) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
diff --git a/ip/link_vti6.c b/ip/link_vti6.c
index f665520..84824a5 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -164,14 +164,14 @@ get_failed:
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, AF_INET6);
-			memcpy(&daddr, addr.data, addr.bytelen);
+			get_addr(&addr, *argv, AF_INET6);
+			memcpy(&daddr, addr.data, sizeof(daddr));
 		} else if (!matches(*argv, "local")) {
 			inet_prefix addr;
 
 			NEXT_ARG();
-			get_prefix(&addr, *argv, AF_INET6);
-			memcpy(&saddr, addr.data, addr.bytelen);
+			get_addr(&addr, *argv, AF_INET6);
+			memcpy(&saddr, addr.data, sizeof(saddr));
 		} else if (!matches(*argv, "dev")) {
 			NEXT_ARG();
 			link = if_nametoindex(*argv);
-- 
1.7.10.4

[PATCH iproute2 3/3] ip: gre: fix IFLA_GRE_LINK attribute sizing

[Serhey Popovych]

Attribute IFLA_GRE_LINK is 32 bit long, not 8 bit.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 6f82fb4..09f1e44 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -155,7 +155,7 @@ get_failed:
 			tos = rta_getattr_u8(greinfo[IFLA_GRE_TOS]);
 
 		if (greinfo[IFLA_GRE_LINK])
-			link = rta_getattr_u8(greinfo[IFLA_GRE_LINK]);
+			link = rta_getattr_u32(greinfo[IFLA_GRE_LINK]);
 
 		if (greinfo[IFLA_GRE_ENCAP_TYPE])
 			encaptype = rta_getattr_u16(greinfo[IFLA_GRE_ENCAP_TYPE]);
-- 
1.7.10.4

Re: [PATCH iproute2 0/3] Improve tunnel local/remote endpoint params and gre link attribute handling

[Stephen Hemminger]

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

> In this series following problems addressed:
> 
>   1) Size of IFLA_GRE_LINK attribute is 32 bits long in , not 8 bit.
> 
>   2) Use get_addr() instead of get_prefix() to parse local/remote
>      tunnel endpoints as IPADDR, not PREFIX as per ip-link(8).
> 
>   3) No need to check if local/remote endpoints are zero (e.g. INADDR_ANY):
>      it is fully legal value, accepted by the kernel.
> 
> See individual patch description message for details.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (3):
>   ip/tunnel: Unify setup and accept zero address for local/remote
>     endpoints
>   ip/tunnel: Use get_addr() instead of get_prefix() for local/remote
>     endpoints
>   ip: gre: fix IFLA_GRE_LINK attribute sizing
> 
>  ip/ip6tunnel.c   |    8 ++------
>  ip/iptunnel.c    |   10 ++--------
>  ip/link_gre.c    |    8 +++-----
>  ip/link_gre6.c   |    8 ++------
>  ip/link_ip6tnl.c |   12 ++++--------
>  ip/link_iptnl.c  |   10 ++--------
>  ip/link_vti.c    |   14 ++------------
>  ip/link_vti6.c   |   26 ++++++++------------------
>  8 files changed, 25 insertions(+), 71 deletions(-)
> 

Looks good, applied