OpenBSD mallocarray

[Warner Losh]

On Wed, Feb 3, 2016 at 12:39 PM, John Baldwin <jhb at freebsd.org> wrote:

> On Monday, February 01, 2016 04:01:14 PM Warner Losh wrote:
> > On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis <brooks at 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 at 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 at 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.
>
> 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.
>

Exausting kmem isn't influenced by simple args. But I do stand corrected.


> 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. :)
>

I like Bruce's approach. And it works for more than just malloc.


> 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).
>

That would be fine. On its own, mallocarray() has all the issues we've
talked about.

Warner