From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling Date: Tue, 19 Dec 2017 07:59:51 -0800 Message-ID: <20171219075951.7aca0d53@xeon-e3> References: <1513623248-7689-1-git-send-email-serhe.popovych@gmail.com> <1513623248-7689-2-git-send-email-serhe.popovych@gmail.com> <20171218112359.1ef8db23@xeon-e3> <7f83992a-90e0-4f15-2be4-7348a6742e6c@gmail.com> <20171218132231.6dcf7b54@xeon-e3> <18e98fc6-4145-0bb9-143d-d4305d22bdc8@gmail.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; boundary="Sig_/Bys+DcpB=FOmyHSr3WXF5=A"; protocol="application/pgp-signature" Cc: netdev@vger.kernel.org To: Serhey Popovich Return-path: Received: from mail-pl0-f65.google.com ([209.85.160.65]:46362 "EHLO mail-pl0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751166AbdLSP75 (ORCPT ); Tue, 19 Dec 2017 10:59:57 -0500 Received: by mail-pl0-f65.google.com with SMTP id i6so7209707plt.13 for ; Tue, 19 Dec 2017 07:59:57 -0800 (PST) In-Reply-To: <18e98fc6-4145-0bb9-143d-d4305d22bdc8@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: --Sig_/Bys+DcpB=FOmyHSr3WXF5=A Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Mon, 18 Dec 2017 23:37:09 +0200 Serhey Popovich wrote: > Stephen Hemminger wrote: > > On Mon, 18 Dec 2017 23:02:07 +0200 > > Serhey Popovich wrote: > > =20 > >> Stephen Hemminger wrote: =20 > >>> On Mon, 18 Dec 2017 20:54:06 +0200 > >>> Serhey Popovych wrote: > >>> =20 > >>>> diff --git a/ip/iplink.c b/ip/iplink.c > >>>> index 1e685cc..4f9c169 100644 > >>>> --- a/ip/iplink.c > >>>> +++ b/ip/iplink.c > >>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct = iplink_req *req, > >>>> *name =3D *argv; > >>>> } else if (strcmp(*argv, "index") =3D=3D 0) { > >>>> NEXT_ARG(); > >>>> + if (*index) > >>>> + duparg("index", *argv); > >>>> *index =3D atoi(*argv); > >>>> - if (*index < 0) > >>>> + if (*index <=3D 0) =20 > >>> > >>> Why not use strtoul instead of atoi? =20 > >> Do not see reason for strtoul() instead atoi(): > >> > >> 1) main arg: indexes in kernel represented as "int", which is > >> signed. <=3D 0 values are reserved for various special purposes > >> (see net/core/fib_rules.c on how device matching implemented). > >> > >> Configuring network device manually with index <=3D 0 is not corr= ect > >> (however possible). Kernel itself never chooses ifindex <=3D 0. > >> > >> Having unsigned int > 0x7fffffff actually means index <=3D 0. > >> > >> 2) this is not single place in iproute2 where it is used: not > >> going to remove last user. > >> > >> 3) make changes clear and transparent for review. =20 > >=20 > > I would rather all of iproute2 correctly handles unsigned values. > > Too much code is old K&R style C "the world is an int" and "who needs > > to check for negative". =20 >=20 > You are right :(. I'm just trying to improve things a bit. >=20 > >=20 > > There already is get_unsigned() in iproute2 util functions. =20 > This is good one based on strtoul(). But do we want to submit say > index =3D (unsigned int)2147483648(0x7fffffff) to the kernel that is > illegal from it's perspective? >=20 > Or do you mean I can prepare treewide change to replace atoi() with > get_unsigned()/get_integer() where appropriate? >=20 > We already check if (*index < 0) since commit 3c682146aeff > (iplink: forbid negative ifindex and modifying ifindex), and I just > put index =3D=3D 0 in the same range of invalid indexes. >=20 The legacy BSD ABI for interfaces uses int, so that sets the upper bound for kernel. The netlink ABI limit is u32 for ifindex so technically 1..UINT32_MAX are possible values but kernel is bound by BSD mistake. I will take the original patch. --Sig_/Bys+DcpB=FOmyHSr3WXF5=A Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEn2/DRbBb5+dmuDyPgKd/YJXN5H4FAlo5N3cACgkQgKd/YJXN 5H4vwQ//R5clGWF8+pxB8+CekfRP66u7t7lKmoKKz0GtDaoJLNMWz+WOE4BpOMOl eOJ7si+d5xSHqUPWvoW7x2pRnLkEDQhW/eQr0U6L5x847y5BQrKK3oYDhONY7Izk GBAgyiygNwqyM0BCz0RL8PIyOnekcbUEs96H5D0LiBVjh5Pt3Ju+W7J6Jgduuxjx A52OAxyYbqHFRqLGVvDqqSOfHV/JcKjux5208TeLiQKbu3dH63syen0fKZjIklyn 0JWUNG1gf6BBNoHFot7klXJZwYDojvX8Lc3va2OLwXb/s+Q8MxOVkthVGvG0+U6O UaeKNpDYYmjirkYLqpays2oAsgFNzXcqeuFtPL2GlSuHZGD4/YmbJZBbpZ8ZR2kg 7MQGYw3h1vCMb3+z6efywhiHelQK4MBiAb/aeWEX3X2nxE1hE2dQb41cBUfG66vZ hrKYu3BfJqGaXN9G8MVbt4G1ZxMzAD+eYrU5HG/Yd2kd9mOxa4N5hJWGyEufG8Bq mlww74ToDjajktNHpV5YM5mwCrXPP9C5jJYiqMwUag5rgSAZq1lbkB5XEy+Elf3U wMSQGcSIMtg30Hk+VpMb0eA0xhpvmXWVkB/moDTTgjg1dbMlznfJBDYZCmoHMsUk 95ColkCUsBPhhnUnMJPmw3yUOuPBYXB4u/nwjZVTzNFSvIP7mFE= =9S/K -----END PGP SIGNATURE----- --Sig_/Bys+DcpB=FOmyHSr3WXF5=A--