[PATCH iproute2 0/2] ip/tunnel: Do not free() answer on rtnl_talk() error, correct ip6gre

[PATCH iproute2 0/2] ip/tunnel: Do not free() answer on rtnl_talk() error, correct ip6gre

[Serhey Popovych]

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

  1) If rtnl_talk(), called with answer != NULL fails it
     never returns with allocated answer: no need to
     initialize answer and free() it in the caller on error.

  2) Correct ip6gre print endpoint address printing with
     ll_addr_n2a(): endpoints are IPv6 addresses, not hex
     dump.

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (2):
  utils: ll_addr: Handle ARPHRD_IP6GRE in ll_addr_n2a()
  ip/tunnel: No need to free answer after rtnl_talk() on error

 ip/link_gre.c    |    3 +--
 ip/link_gre6.c   |    3 +--
 ip/link_ip6tnl.c |    3 +--
 ip/link_iptnl.c  |    3 +--
 ip/link_vti.c    |    3 +--
 ip/link_vti6.c   |    3 +--
 lib/ll_addr.c    |    3 ++-
 7 files changed, 8 insertions(+), 13 deletions(-)

-- 
1.7.10.4

[PATCH iproute2 1/2] utils: ll_addr: Handle ARPHRD_IP6GRE in ll_addr_n2a()

[Serhey Popovych]

ll_addr_n2a() correctly prints tunnel endpoints for gre, ipip, sit
and ip6tnl, but not for ip6gre. Fix this by adding ARPHRD_IP6GRE to
IPv6 tunnel endpoing address conversion.

Before:
-------

$ ip link show
...
18: ip6tnl0: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT group default
    link/tunnel6 :: brd ::
19: ip6gre0: <NOARP> mtu 1456 qdisc noop state DOWN mode DEFAULT group default
    link/gre6 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 brd \
00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00

After:
------

$ ip link show
...
18: ip6tnl0: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT group default
    link/tunnel6 :: brd ::
19: ip6gre0: <NOARP> mtu 1456 qdisc noop state DOWN mode DEFAULT group default
    link/gre6 :: brd ::

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 lib/ll_addr.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ll_addr.c b/lib/ll_addr.c
index c03523d..84de64e 100644
--- a/lib/ll_addr.c
+++ b/lib/ll_addr.c
@@ -36,7 +36,8 @@ const char *ll_addr_n2a(const unsigned char *addr, int alen, int type, char *buf
 	    (type == ARPHRD_TUNNEL || type == ARPHRD_SIT || type == ARPHRD_IPGRE)) {
 		return inet_ntop(AF_INET, addr, buf, blen);
 	}
-	if (alen == 16 && type == ARPHRD_TUNNEL6) {
+	if (alen == 16 &&
+	    (type == ARPHRD_TUNNEL6 || type == ARPHRD_IP6GRE)) {
 		return inet_ntop(AF_INET6, addr, buf, blen);
 	}
 	snprintf(buf, blen, "%02x", addr[0]);
-- 
1.7.10.4

[PATCH iproute2 2/2] ip/tunnel: No need to free answer after rtnl_talk() on error

[Serhey Popovych]

Since rtnl_talk() never returns with answer buffer allocated
on error we do not need to release it manually. After this
initializing answer with NULL before rtnl_talk() is useless.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c    |    3 +--
 ip/link_gre6.c   |    3 +--
 ip/link_ip6tnl.c |    3 +--
 ip/link_iptnl.c  |    3 +--
 ip/link_vti.c    |    3 +--
 ip/link_vti6.c   |    3 +--
 6 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 2397920..f55c40c 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -75,7 +75,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *greinfo[IFLA_GRE_MAX + 1];
@@ -104,7 +104,6 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index 7190ada..a3e8e08 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -86,7 +86,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *greinfo[IFLA_GRE_MAX + 1];
@@ -114,7 +114,6 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index f11ddd5..0a471c2 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -83,7 +83,7 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *iptuninfo[IFLA_IPTUN_MAX + 1];
@@ -108,7 +108,6 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index 5e4dffa..8a8f5dd 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -83,7 +83,7 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *iptuninfo[IFLA_IPTUN_MAX + 1];
@@ -112,7 +112,6 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
diff --git a/ip/link_vti.c b/ip/link_vti.c
index 6c5469f..2b0fab2 100644
--- a/ip/link_vti.c
+++ b/ip/link_vti.c
@@ -60,7 +60,7 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *vtiinfo[IFLA_VTI_MAX + 1];
@@ -77,7 +77,6 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
diff --git a/ip/link_vti6.c b/ip/link_vti6.c
index 4136b0e..74c246d 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -55,7 +55,7 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv,
 		.i.ifi_family = preferred_family,
 		.i.ifi_index = ifi->ifi_index,
 	};
-	struct nlmsghdr *answer = NULL;
+	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *vtiinfo[IFLA_VTI_MAX + 1];
@@ -72,7 +72,6 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv,
 get_failed:
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
-			free(answer);
 			return -1;
 		}
 
-- 
1.7.10.4

Re: [PATCH iproute2 0/2] ip/tunnel: Do not free() answer on rtnl_talk() error, correct ip6gre

[Stephen Hemminger]

On Wed, 20 Dec 2017 09:57:08 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> In this series I present next set of improvements/fixes:
> 
>   1) If rtnl_talk(), called with answer != NULL fails it
>      never returns with allocated answer: no need to
>      initialize answer and free() it in the caller on error.
> 
>   2) Correct ip6gre print endpoint address printing with
>      ll_addr_n2a(): endpoints are IPv6 addresses, not hex
>      dump.
> 
> See individual patch description message for details.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (2):
>   utils: ll_addr: Handle ARPHRD_IP6GRE in ll_addr_n2a()
>   ip/tunnel: No need to free answer after rtnl_talk() on error
> 
>  ip/link_gre.c    |    3 +--
>  ip/link_gre6.c   |    3 +--
>  ip/link_ip6tnl.c |    3 +--
>  ip/link_iptnl.c  |    3 +--
>  ip/link_vti.c    |    3 +--
>  ip/link_vti6.c   |    3 +--
>  lib/ll_addr.c    |    3 ++-
>  7 files changed, 8 insertions(+), 13 deletions(-)
> 

Applied, thanks.