From owner-freebsd-arch@FreeBSD.ORG Mon Apr 6 20:27:11 2015 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1783C64D; Mon, 6 Apr 2015 20:27:11 +0000 (UTC) Received: from na01-bn1-obe.outbound.protection.outlook.com (mail-bn1bn0108.outbound.protection.outlook.com [157.56.110.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (Client CN "mail.protection.outlook.com", Issuer "MSIT Machine Auth CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 73E39FC6; Mon, 6 Apr 2015 20:27:09 +0000 (UTC) Received: from CO2PR0501MB1063.namprd05.prod.outlook.com (25.160.7.20) by CY1PR0501MB1547.namprd05.prod.outlook.com (25.161.161.145) with Microsoft SMTP Server (TLS) id 15.1.130.23; Mon, 6 Apr 2015 20:27:03 +0000 Received: from CO2PR0501MB1063.namprd05.prod.outlook.com ([25.160.7.20]) by CO2PR0501MB1063.namprd05.prod.outlook.com ([25.160.7.20]) with mapi id 15.01.0125.002; Mon, 6 Apr 2015 20:27:02 +0000 From: Anuranjan Shukla To: John Baldwin , "freebsd-arch@freebsd.org" Subject: Re: RFC: chgsbsize doesn't handle -ve sbsize correctly Thread-Topic: RFC: chgsbsize doesn't handle -ve sbsize correctly Thread-Index: AQHQbeE/f9SgtMJNR02J2FpJ+QyWpp1AX0uA//+fsAA= Date: Mon, 6 Apr 2015 20:27:02 +0000 Message-ID: References: <476184366.nOLsX7PO1g@ralph.baldwin.cx> In-Reply-To: <476184366.nOLsX7PO1g@ralph.baldwin.cx> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Microsoft-MacOutlook/14.4.8.150116 x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [66.129.239.12] authentication-results: freebsd.org; dkim=none (message not signed) header.d=none; x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0501MB1547; x-microsoft-antispam-prvs: x-forefront-antispam-report: BMV:1; SFV:NSPM; SFS:(10019020)(6009001)(24454002)(51704005)(479174004)(377454003)(2501003)(19580405001)(2900100001)(46102003)(50986999)(76176999)(40100003)(54356999)(2950100001)(83506001)(450100001)(19580395003)(106116001)(102836002)(92566002)(77156002)(62966003)(86362001)(66066001)(122556002)(99286002)(36756003)(87936001)(2656002); DIR:OUT; SFP:1102; SCL:1; SRVR:CY1PR0501MB1547; H:CO2PR0501MB1063.namprd05.prod.outlook.com; FPR:; SPF:None; MLV:sfv; LANG:en; x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(601004)(5005006)(5002010); SRVR:CY1PR0501MB1547; BCL:0; PCL:0; RULEID:; SRVR:CY1PR0501MB1547; x-forefront-prvs: 0538A71254 Content-Type: text/plain; charset="us-ascii" Content-ID: <960BBF627E9C8A42BCA8FB358DAB6908@namprd05.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: juniper.net X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Apr 2015 20:27:02.2870 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: bea78b3c-4cdb-4130-854a-1d193232e5f4 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0501MB1547 Cc: "arch@FreeBSD.org" , Simon Gerraty X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Apr 2015 20:27:11 -0000 Hi John, Correct, I'd miswrote int. I'm curious as to what point there is in keeping sbsize signed in the first place. If we make it unsigned long, on 32 bit also you get 'more', and overflow could be detected someway like 'if (diff > 0 && new < ui_sbsize)'. I don't see much of a value with the signed sbsize. Any particular reason you would favor otherwise? Thanks for get back on this. Anu On 4/6/15, 12:11 PM, "John Baldwin" wrote: >On Friday, April 03, 2015 07:38:59 AM Anuranjan Shukla wrote: >> Hi, >> In struct uidinfo{}, ui_sbsize member is an int. In a system with large >> number of sockets owned by a user it's possible for ui_sbsize to roll >>over >> to a negative value. When that happens, further calls to _increase_ >>sbsize >> don't detect this condition as they should per design. But when the >> sockets start shutting down this condition would be detected and you'll >> see the console prints ("negative sbsize for uid =3D") happen. As a wors= t >> case the console being flooded with these prints can create a DoS >>scenario >> (specially on single CPU systems). >>=20 >> Regardless of the end result, the code itself needs to be fixed for >> correctness. There are two things to note: >> - Int doesn't seem to be the correct type for ui_sbsize. Should be a >>u_int >> atleast. >> - Since there's no real prevention of the overflow as it happens, the >> warning print isn't helpful and should be a DEBUG level log at best. If >>we >> change ui_sbsize to be a u_int, the negative check itself can go. >>=20 >>=20 >> int >> chgsbsize(uip, hiwat, to, max) >> { >> int diff; >>=20 >> diff =3D to - *hiwat; >> if (diff > 0) { >> if (atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) + >> diff > max) { >> <=3D=3D=3D=3D=3D=3D=3D -ve ui_sbsize goes undetected and we'll p= ass the above >> check=20 >> atomic_subtract_long(&uip->ui_sbsize, >>(long)diff); >> return (0); >> } >> } else { >> atomic_add_long(&uip->ui_sbsize, (long)diff); >> if (uip->ui_sbsize < 0) >> printf("negative sbsize for uid =3D %d\n", >> uip->ui_uid); >>=20 >> <=3D=3D=3D=3D Now we'll detect, far too late, that sbsiz= e went >>-ve > >Note that ui_sbsize is long, not int. That doesn't help you on 32-bit >platforms, but you are also stuck with 32 bits since you typically don't >have 64-bit atomics on 32-bit platforms. > >Making it unsigned just means you can't detect overflow anymore. Instead >I think it should be fixed to detect the overflow when increasing the size >and fail then. Several nearby functions would need the same fix btw: > >Index: kern_resource.c >=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >--- kern_resource.c (revision 281146) >+++ kern_resource.c (working copy) >@@ -1380,17 +1380,18 @@ chgproccnt(struct uidinfo *uip, int diff, rlim_t m > int > chgsbsize(struct uidinfo *uip, u_int *hiwat, u_int to, rlim_t max) > { >+ long new; > int diff; >=20 > diff =3D to - *hiwat; >+ new =3D atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) + diff; > if (diff > 0) { >- if (atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) + diff > max) { >+ if (new < 0 || new > max) { > atomic_subtract_long(&uip->ui_sbsize, (long)diff); > return (0); > } > } else { >- atomic_add_long(&uip->ui_sbsize, (long)diff); >- if (uip->ui_sbsize < 0) >+ if (new < 0) > printf("negative sbsize for uid =3D %d\n", uip->ui_uid); > } > *hiwat =3D to; > > >--=20 >John Baldwin