New talloc feature: memlimits

[simo]

On Fri, 2012-09-28 at 22:27 -0700, Matthieu Patou wrote:
> On 09/28/2012 09:45 PM, 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.
> Let me insist that we need a solution for 4.0 it's a huge blocker for 
> this release as you can very easily take down the ldap server.
> So I'm not against any idea but we'd better come up with a solution that 
> fix the bug.

Hi Matthieu,
I totally agree we need to fix the LDAP bug we found last week, the
memlimit stuff is an idea that came out of the discussion around that
bug but I am not proposing it as a way to paper over that bug.

Simo.

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