OpenBSD mallocarray

[John Baldwin]

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.

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