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 08/11/2014 23:28, Jean-Yves Migeon a écrit :
Le 08/11/2014 19:43, Maxime Villard a écrit :
Le 08/11/2014 18:06, Jean-Yves Migeon a écrit :
I agree that the size not being tracked by the allocator (well, it is,
but the API is ill-designed in this regard) leads to great bugs.

Two things come to mind:

- I think that KMEM_SIZE should become enabled by default instead (and
not reserved to DIAGNOSTIC kernels).

Then what's the point of giving a size, if the kernel already knows
what this size is?

It tracks it only when KMEM_SIZE is enabled which (given the code and
comments) was designed as a debug/diagnostic feature.

My suggestion was to enable the size setters/getters by default, and
possibly #ifdef part of the code that would be deemed unsuitable for
!diagnostic kernels.

Yes, but still, what's the point of giving a size then? If the kernel
knows the size, we can remove kmem_free's size argument, otherwise it
is inconsistent.

And then, as I said, we lose the memory optimisation: one more kmem page
is allocated to hold the size.

Given your patch we are not that far away from reimplemting KMEM_SIZE a
second time but without a diagnostic purpose.

Reimplementing the way it holds the size, yes.

My point is:
 - KMEM_SIZE is only enabled on DIAGNOSTIC.
 - We need a similar way of holding the size by default - and no size
   argument to _free.
 - Problem: if we do it by default in kmem_alloc - which, if I
   understand correctly, is your suggestion - we lose the memory
 - Solution: let's use malloc!
 - Problem: malloc is deprecated, and dead.
 - Solutions:
     - implement kmem_valloc/kmem_vfree
     - undeadify malloc

This looks like overkill.

It feels weird to have the size
field added twice when the option is enabled;

Yes it does...

IMHO if you want to keep riding that route, I would:
- enable by default the part of the code that tracks size allocations;
- rename KMEM_SIZE to something like KMEM_SIZE_DIAGNOSTIC and use it to
#ifdef checks that might have performance penalties (I can't see any but
maybe I overlooked something).

You know, I already struggled to get KMEM_SIZE enabled on DIAGNOSTIC.

- I still believe that allocating 0 byte of memory should end in
panic(). While standards make the result implementation-defined, to me
it indicates that something went wrong.

I would agree with you, but the fact is that kmem_alloc(0) is
necessarily triggered when a variable is passed - I mean it is not hard-
coded -, and when memory is allocated depending on a variable, it is
generally filled in and used depending on this variable.

If 0 bytes are allocated, 0 bytes will be written/read. If it's not the
case, then the caller is buggy, and KMEM_REDZONE will detect an
overflow, if any.

Not necessarily. See below.

Thus kmem_valloc(0) could return a valid address that must not be

IMHO this is still a bad idea. This reasoning holds when someone uses
buffer with loops where the evaluation of the exit condition happens at
the beginning and not the end of the block:

for (i = 0; i < len; i++) {...}
do {...} while (i < len)

In addition, if the variable is length it will typically be declared as
unsigned. Decrementing it will... bring interesting results, but I doubt
that red zone will catch the corruption upon free.

Having an empty memory region
serves no purpose in kernel and will put pressure on the "kmem-8" cache
for no benefit.

Yes; but remember that kmem_alloc(0) is rare - well, it should be rare -
so there won't be such a huge "pressure" on kmem-8.
Returning NULL would be even worse from a security standpoint and poses
problems for documentation. What does it mean to kmem_valloc(0,
KM_SLEEP)? A successful allocation but with an invalid pointer? Huh.

A successful allocation with a valid pointer, but a pointer that must
not be written to nor read from.

This asks for real troubles with 0. It is way too close to integer
underflow for unsigned values (nobody expects a negative sized buffer).

I understand what you say; but as your example shows, the allocation is
variable-sized, so the use will be variable-sized too.

Verily I would agree with you if I hadn't seen many bugs like:

	int count = SCARG(uap, misc);
	/* Make sure userland cannot exhaust kernel memory */
	if ((size_t)count > (size_t)uvmexp.nswapdev)
		count = uvmexp.nswapdev;
	ksep_len = sizeof(*ksep) * count;
	ksep = kmem_alloc(ksep_len, KM_SLEEP);

In this example, count could be zero, and it just crashed. But ksep was
used depending on count, it was not touched when count=0.

kmem_alloc(0) can uselessly panic the system while there's no bug at
all. kmem_valloc(0) wouldn't have crashed the system.

And finally, remember that malloc(0) does not panic precisely because
a header is allocated. malloc(0) returns a valid pointer that must not
be touched. And that's why there's never a zero check before
mallocating. Play a bit with NXR and you'll see what I mean.

If you think kmem_valloc(0) is a bad idea, then malloc(0) is a bad idea
too, and we would have to fix them all.

Home | Main Index | Thread Index | Old Index