From owner-freebsd-arch@freebsd.org Tue Feb 2 06:05:22 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 55BA0A9824B for ; Tue, 2 Feb 2016 06:05:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) 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 3D57A15C7 for ; Tue, 2 Feb 2016 06:05:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 3BFBCA98249; Tue, 2 Feb 2016 06:05:22 +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 3B871A98248 for ; Tue, 2 Feb 2016 06:05:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id DEEFD15C6; Tue, 2 Feb 2016 06:05:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 43BF17848BB; Tue, 2 Feb 2016 17:05:18 +1100 (AEDT) Date: Tue, 2 Feb 2016 17:05:18 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Warner Losh cc: cem@freebsd.org, Mike Belopuhov , Ryan Stone , "freebsd-arch@freebsd.org" Subject: Re: OpenBSD mallocarray In-Reply-To: Message-ID: <20160202160137.M916@besplex.bde.org> References: <20160201210256.GA29188@yamori.belopuhov.com> <1EA0ECF5-D7AC-430E-957D-C4D49F9A872B@bsdimp.com> MIME-Version: 1.0 X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=PfoC/XVd c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=nlC_4_pT8q9DhB4Ho9EA:9 a=6I5d2MoRAAAA:8 a=7Qk2ozbKAAAA:8 a=zHAF4flPilWhpgNLD_wA:9 a=pFdfRmNuPZVy2IBl:21 a=gRaiNp1KNaO4jvP0:21 a=45ClL6m2LaAA:10 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE X-Content-Filtered-By: Mailman/MimeDel 2.1.20 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: Tue, 02 Feb 2016 06:05:22 -0000 On Mon, 1 Feb 2016, Warner Losh wrote: > On Mon, Feb 1, 2016 at 3:19 PM, Conrad Meyer wrote: > >> On Mon, Feb 1, 2016 at 1:12 PM, Warner Losh wrote: > ... >>> That=E2=80=99s got to be at most a transient thing until all the >>> code that it is kludged into with out proper thought is fixed. >> >> You mean the panic? What fallback behavior would you prefer? If the >> caller requested an overflowing allocation, there really isn't >> anything sane to do besides immediately panic (again, for M_WAITOK). >> Even M_NOWAIT callers may or may not do something sane. > > I'd prefer that we return NULL. Let the caller decide the policy, > not some stupid thing down in the bowls of malloc because > I forgot to include some stupid new flag. > > M_NOWAIT callers know to check for NULL, or they have a bug. > It would be the same for mallocarray: you must check for NULL and > unwind if you get that back. There's no need for the CANFAIL flag > and it's a horrible API. The whole API is horrible. It is a wrapper that is intended to make things simpler, but actually makes them more complicated. It is even worse than calloc(3) since normal use of malloc(9) is to never fail, while both malloc(3) and calloc(3) always have failure modes. It is the array size calculation that can overflow, not the allocation. First I note how using calloc(3) is only slightly more complicated unless sloppy error handling is acceptable. Good version that does checks up front like most malloc(9) users already do, but for malloc(3). =09if (size !=3D 0 && SIZE_MAX / size < num) =09=09return (EINVAL); =09if (malloc((size_t)size * num) =3D=3D NULL) =09=09laboriously_handle_error_usually_worse_than_null_ptr_core(); The error "can't happen", and we can make that even more likely by changing SIZE_MAX to a small value. We should usually change SIZE_MAX to a small value anyway to limit denials of service to other caleers. The overflow check can be hidden in a macro (not in an inline function without expanding everything to uintmax_t), but using the macro isn't much easier: #define=09MALLOC_ARRAY_CHECK(num, size, limit) \ =09((size) =3D=3D 0 || limit / (size) >=3D (num)) =2E.. =09if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX)) =09=09return (EINVAL); =09if (malloc((size_t)size * num) =3D=3D NULL) =09=09laboriously_handle_error_usually_worse_than_null_ptr_core(); Sloppy code using calloc(): =09if (calloc(num, size) =3D=3D NULL) =09=09laboriously_handle_error_usually_worse_than_null_ptr_core(); Equivalent code using calloc(): =09if (calloc(num, size) =3D=3D NULL) { =09=09/* =09=09 * calloc() doesn't tell us the precise reason for failure, =09=09 * and decoding errno wouldn't be much better than the =09=09 * following, =09=09 * so if our API is not as bad as that then we must check =09=09 * for overflow like we should have done up front. =09=09 */ =09=09if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX)) =09=09=09return (EINVAL); =09=09laboriously_handle_error_usually_worse_than_null_ptr_core(); =09} Using malloc(9) with M_NOWAIT is similar except the error handling must be even more laborious to be correct. It usually isn't correct, but is more needed and is sometimes better than a null pointer panic. Using M_WAITOK, we lose the simpler code that is possible by not having error handling in every caller. Good version: =09if (!MALLOC_ARRAY_CHECK(num, size, my_limit)) =09=09return (EINVAL); =09ptr =3D malloc((size_t)size * num, M_FOO, M_WAITOK); The error handling is obviously neither neither laborious or incorrect. It is just to handle a DOS from callers. Worse version: put all the args in a function. Add flags to control the error handling. I forget the details of mallocarray(). The above can be done by adding 2 args: =09ptr =3D mymallocarray(num, size, sizelimit, M_FOO, M_WAITOK); =09if (ptr =3D=3D NULL) =09=09return (EINVAL); This is actually a couple of words less complicated, but also less clear. It hides the limit check. Don't forget to expand the types taken by the function to uintmax_t. Otherwise, overflow may occur when the function is called when the type of num, size of sizelimit is larger than size_t (most likely if it is a 64-bit type on a 32-bit arch). The macro handles this automatically provided size_max <=3D SIZE_MAX. Bruce From owner-freebsd-arch@freebsd.org Wed Feb 3 19:39:29 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 79613A9AE53; Wed, 3 Feb 2016 19:39:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5351B1F3E; Wed, 3 Feb 2016 19:39:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id DB9B7B917; Wed, 3 Feb 2016 14:39:27 -0500 (EST) From: John Baldwin To: freebsd-arch@freebsd.org Cc: Warner Losh , Brooks Davis , Mike Belopuhov , Ryan Stone , "freebsd-arch@freebsd.org" Subject: Re: OpenBSD mallocarray Date: Wed, 03 Feb 2016 11:39:11 -0800 Message-ID: <1955470.jNCaThvui8@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: References: <20160201224854.GB1747@spindle.one-eyed-alien.net> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 03 Feb 2016 14:39:28 -0500 (EST) 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: Wed, 03 Feb 2016 19:39:29 -0000 On Monday, February 01, 2016 04:01:14 PM Warner Losh wrote: > On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis wrote: > > > On Mon, Feb 01, 2016 at 02:12:20PM -0700, Warner Losh wrote: > > > > > > > On Feb 1, 2016, at 2:02 PM, Mike Belopuhov wrote: > > > > > > > > On Mon, Feb 01, 2016 at 15:56 -0500, Ryan Stone wrote: > > > >> On Mon, Feb 1, 2016 at 3:16 PM, Conrad Meyer wrote: > > > >> > > > >>> > > > >>> Sure. +1 from me. I don't think we want the M_CANFAIL hack, though. > > > >>> > > > >>> 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 overflows > > 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. > > > > 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. > > I guess this comes down to 'why is it an unreasonable burden to > test the return value in code that's converted?' We're already changing > the code. > > If you absolutely must have a flag, I'd prefer M_CANPANIC or something > that is also easy to add for the 'mindless' case that we can easily > grep for so we know when we're removed all the stupid 'mindless' > cases from the tree. Having M_WAITOK-anything return NULL will be a POLA violation. It doesn't return NULL for anything else. I think having a separate M_CANFAIL flag is also rather pointless. If we want to have this, I think it should work similar to malloc(). M_WAITOK panics if you do stupid things (malloc(9) does this for sufficiently large overflow when it exhausts kmem contrary to Warner's earlier claim), M_NOWAIT returns NULL. In general I think I most prefer Bruce's approach of having a separate macro to check for overflow explicitly so it can be handled as a separate error. In particular, if mallocarry(..., M_NOWAIT) fails, is it because of memory shortage (in which case retrying in the future might be sensible) or is it due to overflow (in which case it will fail no matter how many times you retry)? You'd then have to have the macro anyway to differentiate and handle this case. Warner also seems to be assuming that we should do check for overflow explicitly for any user-supplied values before calling malloc() or malloc()-like things. This means N hand-rolled (and possibly buggy) checks, or a shared macro to do the check. I think this is another argument in favor of Bruce's approach. :) If you go that route, then mallocarray() is really just an assertion checker. It should only fail because a programmer ommitted an explicit overflow check for a user-supplied value or screwed up an in-kernel value. In that case I think panic'ing sooner when the overflow is obvious is more useful for debugging the error than a NULL pointer deference some time later (or requests that get retried forever and go possibly unnoticed). -- John Baldwin