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

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

[Serhey Popovych]

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

[PATCH iproute2 1/3] vxcan,veth: Forbid "type" for peer device

[Serhey Popovych]

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 <serhe.popovych@gmail.com>
---
 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

Re: [PATCH iproute2 1/3] vxcan,veth: Forbid "type" for peer device

[Stephen Hemminger]

On Wed, 20 Dec 2017 09:37:29 +0200
Serhey Popovych <serhe.popovych@gmail.com> 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 <serhe.popovych@gmail.com>

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.

Re: [PATCH iproute2 1/3] vxcan,veth: Forbid "type" for peer device

[Serhey Popovych]

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



Stephen Hemminger wrote:
> On Wed, 20 Dec 2017 09:37:29 +0200
> Serhey Popovych <serhe.popovych@gmail.com> 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 <serhe.popovych@gmail.com>
> 
> 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.
> 



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

[PATCH iproute2 1/3] vxcan,veth: Forbid "type" for peer device

[Serhey Popovych]

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 <serhe.popovych@gmail.com>
---
 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

Re: [PATCH iproute2 1/3] vxcan,veth: Forbid "type" for peer device

[Stephen Hemminger]

On Thu, 28 Dec 2017 13:01:04 +0200
Serhey Popovych <serhe.popovych@gmail.com> 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 <serhe.popovych@gmail.com>

Applied this one. The other util patches have some issues

[PATCH iproute2 2/3] utils: ll_map: Update name and type for existing entry

[Serhey Popovych]

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 <serhe.popovych@gmail.com>
---
 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

Re: [PATCH iproute2 2/3] utils: ll_map: Update name and type for existing entry

[Stephen Hemminger]

On Wed, 20 Dec 2017 09:37:30 +0200
Serhey Popovych <serhe.popovych@gmail.com> 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 <serhe.popovych@gmail.com>

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.

Re: [PATCH iproute2 2/3] utils: ll_map: Update name and type for existing entry

[Serhey Popovych]

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

Stephen Hemminger wrote:
> On Wed, 20 Dec 2017 09:37:30 +0200
> Serhey Popovych <serhe.popovych@gmail.com> 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 <serhe.popovych@gmail.com>
> 
> 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.



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

[PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char

[Serhey Popovych]

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 <serhe.popovych@gmail.com>
---
 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

Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char

[Stephen Hemminger]

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

> 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 <serhe.popovych@gmail.com>
> ---
>  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.
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.

Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char

[Serhey Popovych]

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

Stephen Hemminger wrote:
> On Wed, 20 Dec 2017 09:37:31 +0200
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> 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 <serhe.popovych@gmail.com>
>> ---
>>  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.


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