From mboxrd@z Thu Jan 1 00:00:00 1970 From: Serhey Popovych Subject: Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char Date: Thu, 28 Dec 2017 20:17:08 +0200 Message-ID: <6d62f27b-a9a1-0532-8569-c3eee7392698@gmail.com> References: <1513755451-9800-1-git-send-email-serhe.popovych@gmail.com> <1513755451-9800-4-git-send-email-serhe.popovych@gmail.com> <20171228094532.4a653cc0@xeon-e3> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="iapqV8SeBhxdKVsR6HVPiXVM8ie1yQQyi" Cc: netdev@vger.kernel.org To: Stephen Hemminger Return-path: Received: from mail-lf0-f68.google.com ([209.85.215.68]:33464 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752036AbdL1SRW (ORCPT ); Thu, 28 Dec 2017 13:17:22 -0500 Received: by mail-lf0-f68.google.com with SMTP id j143so782290lfg.0 for ; Thu, 28 Dec 2017 10:17:21 -0800 (PST) In-Reply-To: <20171228094532.4a653cc0@xeon-e3> Sender: netdev-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --iapqV8SeBhxdKVsR6HVPiXVM8ie1yQQyi Content-Type: multipart/mixed; boundary="qR9nX8Up1IXXvZNtW7M4PMkcQ6T5EHxVk"; protected-headers="v1" From: Serhey Popovych To: Stephen Hemminger Cc: netdev@vger.kernel.org Message-ID: <6d62f27b-a9a1-0532-8569-c3eee7392698@gmail.com> Subject: Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char References: <1513755451-9800-1-git-send-email-serhe.popovych@gmail.com> <1513755451-9800-4-git-send-email-serhe.popovych@gmail.com> <20171228094532.4a653cc0@xeon-e3> In-Reply-To: <20171228094532.4a653cc0@xeon-e3> --qR9nX8Up1IXXvZNtW7M4PMkcQ6T5EHxVk Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Stephen Hemminger wrote: > On Wed, 20 Dec 2017 09:37:31 +0200 > Serhey Popovych wrote: >=20 >> 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 >> --- >> 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]; >> }; >> =20 >> #define IDXMAP_SIZE 1024 >> @@ -71,7 +71,7 @@ static struct ll_cache *ll_get_by_name(const char *n= ame) >> struct ll_cache *im >> =3D container_of(n, struct ll_cache, name_hash); >> =20 >> - if (strncmp(im->name, name, IFNAMSIZ) =3D=3D 0) >> + if (!strcmp(im->name, name)) >> return im; >> } >> =20 >> @@ -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 =3D 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, >> } >> =20 >> parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), IFLA_PAYLOAD(n)); >> - ifname =3D rta_getattr_str(tb[IFLA_IFNAME]); >> - if (ifname =3D=3D NULL) >> + >> + if (!tb[IFLA_IFNAME]) >> + return 0; >> + strncpy(ifname, rta_getattr_str(tb[IFLA_IFNAME]), IFNAMSIZ); >> + if (!ifname[0]) >> return 0; >> + ifname[IFNAMSIZ - 1] =3D '\0'; >> =20 >> if (im) { >> /* change to existing entry */ >> - rehash =3D strcmp(im->name, ifname); >> + rehash =3D memcmp(im->name, ifname, IFNAMSIZ); >=20 > 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] =3D '\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 beginnin= g to null, > it will not overwrite after that. Then comparison with entry may not wo= rk 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. >=20 > I really doubt this is critical path on anything. Probably just having = a better > hash table implementation would solve that. >=20 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. --qR9nX8Up1IXXvZNtW7M4PMkcQ6T5EHxVk-- --iapqV8SeBhxdKVsR6HVPiXVM8ie1yQQyi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQEcBAEBAgAGBQJaRTUuAAoJEBTawMmQ61bBKf0H+gOcaTHucdI1q/G8gQ9lXUuZ WOTTEzxPQiVpolO73Y08ObytBIMqI9wERSaJZ8yxyQwyWFWs+wigzoF60bKeL1xn ZoZdLhNXuYa/bXSOl5czZNlcCWoSF90qJO9z45SrrRkPmPi8Ndt5YFpaXxGlnKVN pKWpz0ysK3BSk3NzYE0TtFWrefH2tMxDgMTXV0Pk4MRivVNM33GQgCw76kqBUrp7 GAFaV1BZDfGiC4pnaQ+Dnz/Ki2SW+kKlN8PEZjddEgoBiBKUdbIfRRLpzyrsztFH O7lz1me8yzu/vTLjF2OpbJ2LxFlhUlTQP0MRKcv1u8lWcs9NaENscuyMBAbbfMY= =k/Ym -----END PGP SIGNATURE----- --iapqV8SeBhxdKVsR6HVPiXVM8ie1yQQyi--