New talloc feature: memlimits

[simo]

On Sat, 2012-09-29 at 15:29 +1000, Andrew Bartlett wrote:
> On Sat, 2012-09-29 at 00:45 -0400, simo wrote:
> > On Sat, 2012-09-29 at 08:27 +1000, Andrew Bartlett wrote:
> > > On Sun, 2012-09-23 at 00:10 -0600, idra at samba.org wrote:
> > > > Hello list,
> > > > 
> > > > during the recent SDC Conference we had the Samba4 LDAP server hammered by
> > > > the Codenomicon guys. A few bugs were found where we ended up allocating huge
> > > > amounts of memory.
> > > > 
> > > > These bugs will need fixing, but the situation reminded me that we still have
> > > > little or no control on what users can do over ldap. In particular we have no
> > > > good way to limit resources, and it is relatively easy to DoS the LDAP server
> > > > by making it allocate huge amounts of memory.
> > > > 
> > > > So I had the idea of limiting memory allocation to arbitrarily settable sizes
> > > > based on talloc contextes.
> > > > 
> > > > Attached you can find an initial implementation of this feature with basic
> > > > tests.
> > > > 
> > > > By using talloc_set_memlimit() on a context we can decide the maximum amount
> > > > of memory that can be used by any alloction on that context or any of its
> > > > children. Attempting to allocate more memory than allowed results in a failed
> > > > allocation.
> > > > Stealing memory under a memlimited hierarchy does not fail even if the new
> > > > total use exceed the limit, but any further allocation on the context will
> > > > fail. This means we'll need to be careful on how we create temporary contexts
> > > > and then steal data.
> > > > 
> > > > Memory limits can nest and any allocation will reflect in the parents memory
> > > > limits as well. This allows for a context to have larger limits and then
> > > > have individual smaller limits for childrens down the hierarchy.
> > > > 
> > > > Well, enough said, if there are any objections on committin gthis change please
> > > > speak up, otherwise I will push by the end of the week.
> > > 
> > > This certainly fits well with the memory model used in the AD DC, where
> > > most memory is allocated on a parent context, that eventually ends up at
> > > the connecting socket.  It also helps that in most cases we prefer to
> > > use this pattern:
> > > 
> > > TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
> > > 
> > > (do work)
> > > 
> > > talloc_steal(mem_ctx, ret)
> > > TALLOC_FREE(tmp_ctx)
> > > 
> > > (rather than steal from a talloc_stackframe() or from a context built on
> > > NULL). 
> > > 
> > > It also helps that this isn't a new idea - I remember a discussion with
> > > tridge about this early in the new talloc.  (This was in a context of
> > > discussions about if we should gracefully handle out of memory at all
> > > under a modern unix VM system).
> > > 
> > > The challenge of course is that finding out what code doesn't deal with
> > > memory limits cleanly, and working out what the runtime cost is.  It
> > > seems that it would make talloc_steal() a much more intensive operation
> > > than it currently is.
> > > 
> > > Finally, when we do a talloc_steal(), it seems the limit pointer is not
> > > updated on the child chunks.  How do we know these limit pointers will
> > > remain valid? 
> > > 
> > > I understand your rationale, but I think this needs more work and some
> > > very careful positive review rather than a 'push at the end of the
> > > week. 
> > 
> > Indeed I went through it with metze and I have a much improved version
> > in one of my trees. It passed tests last night but haven't pushed yet.
> > 
> > You can find it here:
> > https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=shortlog;h=refs/heads/talloc_memlimit
> > 
> > This version deals with realloc too which was left out (oops :), it also
> > properly propagates limit contexts to children when needed.
> > 
> > I am quite confident we can start pushing this version, but if you find
> > any bug it would be awesome to be able to push an even cleaner patchset.
> 
> I'm still very confused by how talloc_set_memlimit() is meant to work.
> I can't see where it allocates the new limit structure.  I take it that
> unless we call that function, we don't get a limit, and nothing is
> changed?

Exactly, the whole exercise is meant to impact as little as possible any
user that do not want the feature.

> I think _talloc_total_mem_internal() handles all that, but it looks like
> a query function, not a modification function.  Even getting that, if I
> have a parent/child:
> 
>  t1 -> t2
> 
> and I set a limit on t1
> 
> and then I steal onto an unrelated context:
> 
>  talloc_steal(t3, t2)
>  talloc_free(t1)
> 
> Is the limit on t2 still valid? (I expected we would need to either
> re-curse on the whole tree in either limit checking on talloc_steal).

If you steal away to a different (limitless) context the old limit
should not apply anymore.
I think my code may be failing to remove the pointer to the original
limit.
Well spotted, I will rev up a fix n the next few days. (bed time now and
busy w/e follows).

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>