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

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

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

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++) {...}
       vs
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