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

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

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
   optimisation.
 - Solution: let's use malloc!
 - Problem: malloc is deprecated, and dead.
 - Solutions:
     - implement kmem_valloc/kmem_vfree
   or
     - 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
> > 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).

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.