tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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?

- almost irrelevant optimization
- requires tracking of allocations
+ could validate alloc vs. free size

- 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

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:


Home | Main Index | Thread Index | Old Index