From owner-freebsd-arch@freebsd.org Mon Feb 1 23:59:58 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D91ABA98472 for ; Mon, 1 Feb 2016 23:59:58 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id C4374FC0 for ; Mon, 1 Feb 2016 23:59:58 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by mailman.ysv.freebsd.org (Postfix) id C0531A98471; Mon, 1 Feb 2016 23:59:58 +0000 (UTC) Delivered-To: arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BFE98A98470 for ; Mon, 1 Feb 2016 23:59:58 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 997F8FBF for ; Mon, 1 Feb 2016 23:59:58 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 43F325A9F27; Mon, 1 Feb 2016 23:59:57 +0000 (UTC) Date: Mon, 1 Feb 2016 23:59:57 +0000 From: Brooks Davis To: Warner Losh Cc: Mike Belopuhov , "freebsd-arch@freebsd.org" , Ryan Stone Subject: Re: OpenBSD mallocarray Message-ID: <20160201235957.GC1747@spindle.one-eyed-alien.net> References: <20160201210256.GA29188@yamori.belopuhov.com> <1EA0ECF5-D7AC-430E-957D-C4D49F9A872B@bsdimp.com> <20160201224854.GB1747@spindle.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ADZbWkCsHQ7r3kzd" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Feb 2016 23:59:58 -0000 --ADZbWkCsHQ7r3kzd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 01, 2016 at 04:01:14PM -0700, Warner Losh wrote: > On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis wrote: >=20 > > On Mon, Feb 01, 2016 at 02:12:20PM -0700, Warner Losh wrote: > > > > > > > On Feb 1, 2016, at 2:02 PM, Mike Belopuhov wro= te: > > > > > > > > On Mon, Feb 01, 2016 at 15:56 -0500, Ryan Stone wrote: > > > >> On Mon, Feb 1, 2016 at 3:16 PM, Conrad Meyer wro= te: > > > >> > > > >>> > > > >>> Sure. +1 from me. I don't think we want the M_CANFAIL hack, tho= ugh. > > > >>> > > > >>> Best, > > > >>> Conrad > > > >>> > > > >>> > > > >> That may be the OpenBSD equivalent of M_NOWAIT. > > > > > > > > Not quite. From the man page: > > > > > > > > M_CANFAIL > > > > > > > > In the M_WAITOK case, if not enough memory is available, > > > > return NULL instead of calling panic(9). If mallocarray() > > > > detects an overflow or malloc() detects an excessive > > > > allocation, return NULL instead of calling panic(9). > > > > > > Yea, we don???t want it calling panic. Ever. That turns an overflow > > > into a DoS. Arguments should be properly checked so we can > > > properly return EINVAL for bat-**** crazy ones. FreeBSD???s malloc > > > doesn???t cave an excessive detector in it. > > > > > > My concern with this is that we have a number of different allocation > > > routines in FreeBSD. This only goes after the malloc() vector, and > > > even then it requires code changes. > > > > > > At best, CANFAIL is a kludge to fail with a panic instead of an > > > overflow. That???s got to be at most a transient thing until all the > > > code that it is kludged into with out proper thought is fixed. I???m = not > > > sure that???s something that we want to encourage. I???m all for > > > safety, but this flag seems both unsafe and unwise. > > > > Given that current code could be doing literally anything in the > > overflow case (and thanks to modern undefined behavior optimizations is > > likely doing something extraordinarily bizarre), I think turning overfl= ows > > into panics is a good thing. Yes, you can argue that means you've added > > a DoS vector, but best case you had an under allocation and bizarre > > memory corruption before. If the default or even only behavior is going > > to be that overflow fails then we need a static checker that ensure we > > check the return value even in the M_WAITOK. Otherwise there will be > > blind conversions of malloc to mallocarray that go unchecked. > > >=20 > Returning NULL should be sufficient. Blind conversion of malloc to > mallocarray in the kernel is also stupid. Intelligent conversion is > needed to ensure that the error conditions are handled correctly. > There's no need for a flag to say 'I am going to do the right thing > if you give me NULL back'. The conversion should do the right > thing when you get NULL back. A quick survey of the current kernel > shows that there's not very many that could be using user defined > values, but does show a large number of places where we could > use this API. On further consideration, I think returning NULL is sufficient since most of the time failure to check will result in a nearby fault. The main thing is eliminating the undefined behavior. -- Brooks --ADZbWkCsHQ7r3kzd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJWr/F8AAoJEKzQXbSebgfAJacH/RJ0Rvp/1/JRqguNmnsJgJ1e TzWHmqeir6d1aN2jEQ+bDWvVSAp8v7SB074o7cuz6dX6neyA5O1+qeTwCNFbemM7 6Ii5JB1UVOlTA0VCb9FM8CFhw816bg94phGL+RGLOO499P3uUq2ybVeSDD8ns54x D+0jifE+WGs94zdZhcFMr+CQIVqKGD8yGVa4xj1cgJLD6hOUlw/yxjfvXD1ZLezL y15H6F0hRC98lrBYRCaDNLEqhpLvBH5zq5B+cuADQzmpJYk40HrsSypLmXlTyrWF zpb9elq93ohk73uGdo5jXeafoJGiWSQtSKW7C1Gp3AZsMleHanrM0dWIVk/vSgg= =gN2r -----END PGP SIGNATURE----- --ADZbWkCsHQ7r3kzd--