[PATCH iproute2 0/3] Forbid "type" for peer, update ifname and make it array in ll_cache

In this series I present following improvements and fixes:

  1) Forbid "type" parameter when parsing command line
     for peer in iplink_vxcan.c and link_veth.c using
     iplink_parse(): we already known it.

  2) In ll_remember_index() update ifname, not only rehash
     it. It might be changed for same ifindex since last
     run (i.e. in cache "eth0" during the dump "ppp0").

  3) Make ifname fixed size array of chars in @struct ll_cache:
     names are never exceed IFNAMSIZ (16 bytes). Replace
     strcmp()/strcpy() with memcmp()/memcpy() to possibly
     benefit from compiler call inlining.

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (3):
  vxcan,veth: Forbid "type" for peer device
  utils: ll_map: Update name and type for existing entry
  utils: ll_map: Make network device name fixed size array of char

 ip/iplink_vxcan.c |    3 +++
 ip/link_veth.c    |    3 +++
 lib/ll_map.c      |   47 +++++++++++++++++++++++++++--------------------
 3 files changed, 33 insertions(+), 20 deletions(-)

-- 
1.7.10.4

It is already given for original device we configure this
peer for.

Results from following command before/after change applied
are shown below:

  $ ip link add dev veth1a type veth peer name veth1b \
                           type veth peer name veth1c

Before:
-------

<no output, no netdevs created>

After:
------

Error: argument "type" is wrong: not supported for peer

Signed-off-by: Serhey Popovych <redacted>
---
 ip/iplink_vxcan.c |    3 +++
 ip/link_veth.c    |    3 +++
 2 files changed, 6 insertions(+)

diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c
index c13224c..13f2577 100644
--- a/ip/iplink_vxcan.c
+++ b/ip/iplink_vxcan.c

@@ -65,6 +65,9 @@ static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (err < 0)
 		return err;
 
+	if (type)
+		invarg("not supported for peer", "type");
+
 	if (name) {
 		addattr_l(hdr, 1024,
 			  IFLA_IFNAME, name, strlen(name) + 1);

diff --git a/ip/link_veth.c b/ip/link_veth.c
index fcfd1ef..cc43198 100644
--- a/ip/link_veth.c
+++ b/ip/link_veth.c

@@ -63,6 +63,9 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (err < 0)
 		return err;
 
+	if (type)
+		invarg("not supported for peer", "type");
+
 	if (name) {
 		addattr_l(hdr, 1024,
 			  IFLA_IFNAME, name, strlen(name) + 1);

-- 
1.7.10.4

In case of we update existing entry we need not only rehash
but also update name in existing entry.

Need to update device type too since cached interface might
be deleted and new with same index, but different type
added (e.g. eth0 and ppp0).

Reuse new entry initialization path to avoid duplications.

Signed-off-by: Serhey Popovych <redacted>
---
 lib/ll_map.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/lib/ll_map.c b/lib/ll_map.c
index f65614f..abe7bdc 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c

@@ -10,6 +10,7 @@
  *
  */
 
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>

@@ -85,6 +86,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
 	struct ll_cache *im;
 	struct rtattr *tb[IFLA_MAX+1];
+	bool rehash;
 
 	if (n->nlmsg_type != RTM_NEWLINK && n->nlmsg_type != RTM_DELLINK)
 		return 0;

@@ -109,29 +111,30 @@ int ll_remember_index(const struct sockaddr_nl *who,
 
 	if (im) {
 		/* change to existing entry */
-		if (strcmp(im->name, ifname) != 0) {
+		rehash = strcmp(im->name, ifname);
+		if (rehash)
 			hlist_del(&im->name_hash);
-			h = namehash(ifname) & (IDXMAP_SIZE - 1);
-			hlist_add_head(&im->name_hash, &name_head[h]);
-		}
+	} else {
+		im = malloc(sizeof(*im) + strlen(ifname) + 1);
+		if (!im)
+			return 0;
+		im->index = ifi->ifi_index;
 
-		im->flags = ifi->ifi_flags;
-		return 0;
+		h = ifi->ifi_index & (IDXMAP_SIZE - 1);
+		hlist_add_head(&im->idx_hash, &idx_head[h]);
+
+		rehash = true;
 	}
 
-	im = malloc(sizeof(*im) + strlen(ifname) + 1);
-	if (im == NULL)
-		return 0;
-	im->index = ifi->ifi_index;
-	strcpy(im->name, ifname);
 	im->type = ifi->ifi_type;
 	im->flags = ifi->ifi_flags;
 
-	h = ifi->ifi_index & (IDXMAP_SIZE - 1);
-	hlist_add_head(&im->idx_hash, &idx_head[h]);
+	if (rehash) {
+		h = namehash(ifname) & (IDXMAP_SIZE - 1);
+		hlist_add_head(&im->name_hash, &name_head[h]);
 
-	h = namehash(ifname) & (IDXMAP_SIZE - 1);
-	hlist_add_head(&im->name_hash, &name_head[h]);
+		strcpy(im->name, ifname);
+	}
 
 	return 0;
 }

-- 
1.7.10.4

Network device names are fixed in size and never exceed
IFNAMSIZ (16 bytes).

Make name fixed size array to always malloc() same size chunk
of memory and use memcpy()/memcmp() with constant IFNAMSIZ
to benefit from possible compiler optimizations replacing
call to a function with two/four load/store instructions
on 64/32 bit systems.

Check if IFLA_IFNAME attribute present in netlink message
(should always) and use strncpy() to pad name with zeros.

Signed-off-by: Serhey Popovych <redacted>
---
 lib/ll_map.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/ll_map.c b/lib/ll_map.c
index abe7bdc..fcbf0fb 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c

@@ -30,7 +30,7 @@ struct ll_cache {
 	unsigned	flags;
 	unsigned 	index;
 	unsigned short	type;
-	char		name[];
+	char		name[IFNAMSIZ];
 };
 
 #define IDXMAP_SIZE	1024

@@ -71,7 +71,7 @@ static struct ll_cache *ll_get_by_name(const char *name)
 		struct ll_cache *im
 			= container_of(n, struct ll_cache, name_hash);
 
-		if (strncmp(im->name, name, IFNAMSIZ) == 0)
+		if (!strcmp(im->name, name))
 			return im;
 	}
 

@@ -82,7 +82,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
 		      struct nlmsghdr *n, void *arg)
 {
 	unsigned int h;
-	const char *ifname;
+	char ifname[IFNAMSIZ];
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
 	struct ll_cache *im;
 	struct rtattr *tb[IFLA_MAX+1];

@@ -105,17 +105,21 @@ int ll_remember_index(const struct sockaddr_nl *who,
 	}
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), IFLA_PAYLOAD(n));
-	ifname = rta_getattr_str(tb[IFLA_IFNAME]);
-	if (ifname == NULL)
+
+	if (!tb[IFLA_IFNAME])
+		return 0;
+	strncpy(ifname, rta_getattr_str(tb[IFLA_IFNAME]), IFNAMSIZ);
+	if (!ifname[0])
 		return 0;
+	ifname[IFNAMSIZ - 1] = '\0';
 
 	if (im) {
 		/* change to existing entry */
-		rehash = strcmp(im->name, ifname);
+		rehash = memcmp(im->name, ifname, IFNAMSIZ);
 		if (rehash)
 			hlist_del(&im->name_hash);
 	} else {
-		im = malloc(sizeof(*im) + strlen(ifname) + 1);
+		im = malloc(sizeof(*im));
 		if (!im)
 			return 0;
 		im->index = ifi->ifi_index;

@@ -133,7 +137,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
 		h = namehash(ifname) & (IDXMAP_SIZE - 1);
 		hlist_add_head(&im->name_hash, &name_head[h]);
 
-		strcpy(im->name, ifname);
+		memcpy(im->name, ifname, IFNAMSIZ);
 	}
 
 	return 0;

-- 
1.7.10.4

On Wed, 20 Dec 2017 09:37:29 +0200
Serhey Popovych [off-list ref] wrote:

It is already given for original device we configure this
peer for.

Results from following command before/after change applied
are shown below:

  $ ip link add dev veth1a type veth peer name veth1b \
                           type veth peer name veth1c

Before:
-------

<no output, no netdevs created>

After:
------

Error: argument "type" is wrong: not supported for peer

Signed-off-by: Serhey Popovych <redacted>

This makes a lot of sense.
Maybe the better error message would be to use duparg() since
that is the most likely cause of the user error.

Stephen Hemminger wrote:

On Wed, 20 Dec 2017 09:37:29 +0200
Serhey Popovych [off-list ref] wrote:

quoted

It is already given for original device we configure this
peer for.

Results from following command before/after change applied
are shown below:

  $ ip link add dev veth1a type veth peer name veth1b \
                           type veth peer name veth1c

Before:
-------

<no output, no netdevs created>

After:
------

Error: argument "type" is wrong: not supported for peer

Signed-off-by: Serhey Popovych <redacted>

This makes a lot of sense.
Maybe the better error message would be to use duparg() since
that is the most likely cause of the user error.

Not sure Stephen. With duparg() I get following error message for
test case described in above.

Error: duplicate "type": "veth" is the second value.

It looks good for me, but we have two "type veth" in command and
which one is wrong might not be very clear to user.

Anyway I send v2 for this particular change to get and option
to pick the best one.

Thanks.

It is already given for original device we configure this
peer for.

Results from following command before/after change applied
are shown below:

  $ ip link add dev veth1a type veth peer name veth1b \
                           type veth peer name veth1c

Before:
-------

<no output, no netdevs created>

After:
------

Error: duplicate "type": "veth" is the second value.

Signed-off-by: Serhey Popovych <redacted>
---
 ip/iplink_vxcan.c |    3 +++
 ip/link_veth.c    |    3 +++
 2 files changed, 6 insertions(+)

diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c
index c13224c..ed0ad8b 100644
--- a/ip/iplink_vxcan.c
+++ b/ip/iplink_vxcan.c

@@ -65,6 +65,9 @@ static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (err < 0)
 		return err;
 
+	if (type)
+		duparg("type", argv[err]);
+
 	if (name) {
 		addattr_l(hdr, 1024,
 			  IFLA_IFNAME, name, strlen(name) + 1);

diff --git a/ip/link_veth.c b/ip/link_veth.c
index fcfd1ef..fddb7ac 100644
--- a/ip/link_veth.c
+++ b/ip/link_veth.c

@@ -63,6 +63,9 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (err < 0)
 		return err;
 
+	if (type)
+		duparg("type", argv[err]);
+
 	if (name) {
 		addattr_l(hdr, 1024,
 			  IFLA_IFNAME, name, strlen(name) + 1);

-- 
1.7.10.4

On Thu, 28 Dec 2017 13:01:04 +0200
Serhey Popovych [off-list ref] wrote:

It is already given for original device we configure this
peer for.

Results from following command before/after change applied
are shown below:

  $ ip link add dev veth1a type veth peer name veth1b \
                           type veth peer name veth1c

Before:
-------

<no output, no netdevs created>

After:
------

Error: duplicate "type": "veth" is the second value.

Signed-off-by: Serhey Popovych <redacted>

Applied this one. The other util patches have some issues

Stephen Hemminger wrote:

On Wed, 20 Dec 2017 09:37:31 +0200
Serhey Popovych [off-list ref] wrote:

quoted

Network device names are fixed in size and never exceed
IFNAMSIZ (16 bytes).

Make name fixed size array to always malloc() same size chunk
of memory and use memcpy()/memcmp() with constant IFNAMSIZ
to benefit from possible compiler optimizations replacing
call to a function with two/four load/store instructions
on 64/32 bit systems.

Check if IFLA_IFNAME attribute present in netlink message
(should always) and use strncpy() to pad name with zeros.

Signed-off-by: Serhey Popovych <redacted>
---
 lib/ll_map.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/ll_map.c b/lib/ll_map.c
index abe7bdc..fcbf0fb 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c

@@ -30,7 +30,7 @@ struct ll_cache {
 	unsigned	flags;
 	unsigned 	index;
 	unsigned short	type;
-	char		name[];
+	char		name[IFNAMSIZ];
 };
 
 #define IDXMAP_SIZE	1024

@@ -71,7 +71,7 @@ static struct ll_cache *ll_get_by_name(const char *name)
 		struct ll_cache *im
 			= container_of(n, struct ll_cache, name_hash);
 
-		if (strncmp(im->name, name, IFNAMSIZ) == 0)
+		if (!strcmp(im->name, name))
 			return im;
 	}
 

@@ -82,7 +82,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
 		      struct nlmsghdr *n, void *arg)
 {
 	unsigned int h;
-	const char *ifname;
+	char ifname[IFNAMSIZ];
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
 	struct ll_cache *im;
 	struct rtattr *tb[IFLA_MAX+1];

@@ -105,17 +105,21 @@ int ll_remember_index(const struct sockaddr_nl *who,
 	}
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), IFLA_PAYLOAD(n));
-	ifname = rta_getattr_str(tb[IFLA_IFNAME]);
-	if (ifname == NULL)
+
+	if (!tb[IFLA_IFNAME])
+		return 0;
+	strncpy(ifname, rta_getattr_str(tb[IFLA_IFNAME]), IFNAMSIZ);
+	if (!ifname[0])
 		return 0;
+	ifname[IFNAMSIZ - 1] = '\0';
 
 	if (im) {
 		/* change to existing entry */
-		rehash = strcmp(im->name, ifname);
+		rehash = memcmp(im->name, ifname, IFNAMSIZ);

This is not safe. There is not guarantee that bytes after end of string are zero.

Sorry Stephen, correct if my assumptions are wrong:

  1. struct ll_cache entries are only modified in ll_remember_index().
     There are no places where we may modify ll_cache entries.

  2. strncpy() always pad with zeroes to the end of IFNAMSIZ sized
     buffer.

  3. strncpy() may not return null terminated string: this addressed
     with ifname[IFNAMSIZ - 1] = '\0' in the code above.

Assuming 1 and 2 we always have im->name[] initialized with string and
zero pads up to IFNAMSIZ. We prepare ifname using strncpy() to, so it
is zero padded and we can safely use memcmp() to compare byte by byte.

And in your code, strncpy() will overwrite characters from the beginning to null,
it will not overwrite after that. Then comparison with entry may not work because
of the data after that.

strncpy() will not pad with zeroes up to IFNAMSIZ? I get from strncpy(3)
it will pad to the end of buf, so IFNAMSIZ is initialized.

Please correct me if I'm wrong at some point.

Thanks.

I really doubt this is critical path on anything. Probably just having a better
hash table implementation would solve that.

Well this is critical with thousands of interfaces. I guess. Didn't go
with tests, but can do that. Of course difference might not be so big as
I expect, but anyway.

On Wed, 20 Dec 2017 09:37:31 +0200
Serhey Popovych [off-list ref] wrote:

This is not safe. There is not guarantee that bytes after end of string are zero.
And in your code, strncpy() will overwrite characters from the beginning to null,
it will not overwrite after that. Then comparison with entry may not work because
of the data after that.

I really doubt this is critical path on anything. Probably just having a better
hash table implementation would solve that.

On Wed, 20 Dec 2017 09:37:30 +0200
Serhey Popovych [off-list ref] wrote:

In case of we update existing entry we need not only rehash
but also update name in existing entry.

Need to update device type too since cached interface might
be deleted and new with same index, but different type
added (e.g. eth0 and ppp0).

Reuse new entry initialization path to avoid duplications.

Signed-off-by: Serhey Popovych <redacted>

Can you provide an example where this is an observed bug?
I suspect that unless you use a batch mode command the reload
of the cache on next invocation is solving this.

Stephen Hemminger wrote:

On Wed, 20 Dec 2017 09:37:30 +0200
Serhey Popovych [off-list ref] wrote:

quoted

In case of we update existing entry we need not only rehash
but also update name in existing entry.

Need to update device type too since cached interface might
be deleted and new with same index, but different type
added (e.g. eth0 and ppp0).

Reuse new entry initialization path to avoid duplications.

Signed-off-by: Serhey Popovych <redacted>

Can you provide an example where this is an observed bug?
I suspect that unless you use a batch mode command the reload
of the cache on next invocation is solving this.

From my side example from description is pretty clear: eth0 -> ppp0
or rename eth0 -> eth1, etc.

According to ll_remember_index() code we might be called with
non-empty cache. If ll_get_by_index() returns an entry with
name that differs from current we need:

  1. Rehash in ->name_hash (done with current code)
  2. Update entry name     (not done with current code)

That's my point of view.

Thanks.