talloc: talloc_set_memlimit causes all reallocs to fail when used on pools. talloc_set_memlimit not enforced correctly on pools.

[Arran Cudbard-Bell]

> On Oct 16, 2020, at 2:51 PM, Arran Cudbard-Bell via samba-technical <samba-technical at lists.samba.org> wrote:
> 
> 
> 
>> On Oct 16, 2020, at 2:44 PM, Jeremy Allison <jra at samba.org> wrote:
>> 
>> On Fri, Oct 16, 2020 at 01:59:11PM -0500, Arran Cudbard-Bell via samba-technical wrote:
>>> Reviewing the talloc code shows provisions (and test cases) for applying memlimits to pools, unfortunately in practice, memlimit functionality on pools seems fairly broken.
>>> 
>>> #include <talloc.h>
>>> #include <stdio.h>
>>> 
>>> int main(int argc, char **argv)
>>> {
>>>      TALLOC_CTX *pool = talloc_pool(NULL, 1024);
>>>      TALLOC_CTX *chunk, *fail_chunk;
>>> 
>>>      if (pool) {
>>>              printf("Pool allocated\n");
>>>      } else {
>>>              printf("Pool allocation failed\n");
>>>      }
>>> 
>>>      if (talloc_set_memlimit(pool, 512) < 0) {
>>>              printf("talloc_set_memlimit failed\n");
>>>      } else {
>>>             printf("talloc_set_memlimit success\n");
>>>      }
>> 
>> OK, looking at this more closely, I think this is
>> a fundemental misunderstanding of what a talloc_pool
>> is.
>> 
>> A talloc_pool() is pre-allocated memory, that can
>> then be further divided without having to call
>> into malloc.
> 
> Yes, we use them for slab allocation and for creating read only regions of memory with mprotect.  We have a special wrapper that ensures the first child allocated in the pool is page aligned.
> 
> When we do this, we don't want to allow allocations to occur outside of the pool so set a memlimit on the pool.  I know this is a really weird use case, but it's ended up being quite a useful debugging tool.
> 
>> It's ALREADY allocated memory, so setting a memlimit size
>> smaller than the pool size makes no sense - once you've
>> done that all allocations should fail.
> 
> In addition to preventing allocations outside of the pool, It would still be useful to be able to apply limits to pools so that a pool passed into a function cannot be entirely consumed by allocations by that function. i.e. if you want to reserve some space in the pool.

As for what the semantics should be, I was thinking that, yes, a pool in its entirety should be included in the memlimit calculation of its parent chunk, but if a memlimit is set on a pool directly, only the memory used by the children should be included in the memlimit check.  I didn't get far enough into the code to see how many assumptions would be broken by this.

An alternative that'd still satisfy our immediate need would be to have talloc_set_memlimit simply fail when someone tried to apply it to a pool (as you suggested), and add an optional flag that'd prevent allocations from occurring outside of the pool.

-Arran