| Message ID | 1513755451-9800-4-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | Forbid "type" for peer, update ifname and make it array in ll_cache | expand |
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.
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.
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;
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(-)