Re: Proposal: kmem_valloc [was: Re: raspberry pi panic 7.0_BETA after install fs resize]

[Maxime Villard <max%M00nBSD.net@localhost>]

Le 09/11/2014 09:43, Michael van Elst a écrit :
> On Sat, Nov 08, 2014 at 07:46:08PM +0100, Maxime Villard wrote:
> > Le 08/11/2014 19:08, Michael van Elst a écrit :
> > > As a side effect, it removed the debugging features of malloc/free.
> >
> > As well as it allowed many bugs.
>
> I doubt that it allowed many bugs. It made code slightly more complex,
> but it also made people look at what the code does.

Yes; it made code slightly more complex, complex enough to introduce
bugs, bugs that then got fixed after looking at what the code does.

That's exactly what we saw in NetBSD-SA2014-014.

> For the most recent issue in ffs, it might even have exposed a bug
> as the change to the fs structure apparently did not cause a
> re-allocation of the csum structure, which is the real problem.
> Fixing only the de-allocation just hides that bug.

Yes, ffs_reload() is completely buggy: it re-reads and changes many
values without reallocating anything. You will also notice that the
whole thing is ready to collapse: bread() takes an int argument,
which can trigger a panic later; there are also several divisions
by zero, etc.

But what you say is I think a bit paradoxical. Yes, kmem_alloc might
have unearthed a bug in ffs. But why did it find this bug? Because I
enabled KMEM_SIZE. If I hadn't enabled it some months ago, kmem_alloc
wouldn't have detected a bug, and the situation would have been even
worse: there is a bug in the allocation, *and* a memory corruption
when freeing.

This is all the more paradoxical when we know that my proposal of
enabling KMEM_SIZE and KMEM_REDZONE was welcomed with scepticism.

So no, kmem_alloc hasn't been designed precisely to unroot lurking
bugs. Now it finds bugs only in architectures where DIAGNOSTIC
is enabled; when it is not, it adds a memory corruption.

> > > > That's what motivates my proposal: creating kmem_valloc and kmem_vfree,
> > > > to definitely get rid of malloc, and have another set of functions
> > > > that can perform allocations/deallocations without needing a size
> > > > argument. The 'v' stands for "variable".
> > >
> > > Looks like the worst of both worlds.
> >
> > Could you elaborate a bit?
>
> kmem_alloc/kmem_free
> - almost irrelevant optimization
> - requires tracking of allocations
> + could validate alloc vs. free size
>
> malloc/free
> - some overhead for tracking + tagging allocations
> + simpler code for fixed data structures
> -/+ not really a constructor/deconstructor
> + tagging allocations helps debugging (*)
>
> (*) Yes, I know that this is only a dummy parameter now.
>
> Your suggestion would
> - add another interface
> - add some overhead
> - be used for a few fringe cases, unless it replaces kmem_alloc/kmem_free
> -/+ would not help debugging like malloc/free

Help debugging? Tagging allocations?

Look at sys/sys/malloc.h:

	#define	malloc(size, type, flags)	kern_malloc(size, flags)

The tag is not even compiled in. Thus it can hardly help debugging
anything.

You understand why I say that malloc is dead.

> You can see that I consider the malloc/free API better than
> kmem_alloc/kmem_free. But going half-a-step to create yet
> another interface isn't the right thing.

It creates an interface, that is supposed to replace an old, dead one:
malloc.

> Greetings,