Date: Mon, 1 Feb 2016 16:01:14 -0700 From: Warner Losh <imp@bsdimp.com> To: Brooks Davis <brooks@freebsd.org> Cc: Mike Belopuhov <mike@belopuhov.com>, "freebsd-arch@freebsd.org" <arch@freebsd.org>, Ryan Stone <rysto32@gmail.com> Subject: Re: OpenBSD mallocarray Message-ID: <CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA@mail.gmail.com> In-Reply-To: <20160201224854.GB1747@spindle.one-eyed-alien.net> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAG6CVpWbaFOQ1GzE1qmZFodXg_xZafmCc0b1kUh=0%2BFAjLPRvA@mail.gmail.com> <CAFMmRNyNKOgDEY89dVB=dqYDq6XyQo=MQR%2BHPJ2=_0VdDKRvAw@mail.gmail.com> <20160201210256.GA29188@yamori.belopuhov.com> <1EA0ECF5-D7AC-430E-957D-C4D49F9A872B@bsdimp.com> <20160201224854.GB1747@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis <brooks@freebsd.org> wrote: > On Mon, Feb 01, 2016 at 02:12:20PM -0700, Warner Losh wrote: > > > > > On Feb 1, 2016, at 2:02 PM, Mike Belopuhov <mike@belopuhov.com> wrote: > > > > > > On Mon, Feb 01, 2016 at 15:56 -0500, Ryan Stone wrote: > > >> On Mon, Feb 1, 2016 at 3:16 PM, Conrad Meyer <cem@freebsd.org> 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. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA>