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

Given your patch we are not that far away from reimplemting KMEM_SIZE a second time but without a diagnostic purpose. 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).

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

Jean-Yves Migeon

Home | Main Index | Thread Index | Old Index