New talloc feature: memlimits

[idra at samba.org]

On Mon, Sep 24, 2012 at 07:08:31PM +0200, David Disseldorp wrote:
> On Mon, 24 Sep 2012 10:36:07 -0600
> idra at samba.org wrote:
> 
> > On Mon, Sep 24, 2012 at 04:06:35PM +0200, David Disseldorp wrote:
> > > Hi Simo,
> ...
> > > This looks like a useful debugging feature, but I don't think it should
> > > be enabled by default.
> > 
> > As you can see 'enabling' it depends entirely on using tallos_set_memlimits(),
> > if you don't use it it is not enabled.
> 
> Sorry I wasn't clear, by enabled I was referring to a build time option,
> e.g. TALLOC_DEBUG.

Why should we ?
If you do not use the feature the only cost is testing if a pointer is null.  

> > > Firstly, it adds extra weight and complexity to one of the hottest code
> > > paths in Samba. The TALLOC VS MALLOC SPEED test should be able to
> > > provide data on the performance hit incurred under various limit
> > > conditions.
> > 
> > It is true that talloc is a hot path, but I am sure this feature will not cost much.
> > I will run some test later on so we have more data.
> > 
> > > As a measure against DOS attacks, it sits at the wrong layer IMO.
> > > Requests resulting in excessively large tallocs should be intercepted
> > > explicitly during unmarshalling.
> > 
> > Ok, I do not agree with this point.
> > This feature is not the primary defence for sure, but it is a useful tool in
> > that regard.
> > However my main idea was to use it to implement memory limits in the ldap server.
> > Most LDAP server implementations have both size and memory limits per connection,
> > and this feature allows us to retrofit those limits in easily w/o having to change
> > tons of code.
> 
> I'm not familiar with the LDAP server code, but wouldn't a retrofit
> still require auditing all connection code paths to ensure a correct
> talloc ancestor is used?

No, the LDAP server already is pretty hierarchical because we always need to
tear down all the memory if a connection is lost.

> Also, if the feature is LDAP server specific, perhaps a per-request loop
> talloc_total_size() check would be sufficient.

No it wouldn't. We want to prevent frowing memory, not just kill the connection
after-the-fact.

> > > cgroups / ulimit already provide the ability to restrict memory usage
> > > per-process.
> > 
> > I want to use this per (authenticated) connection :)
> 
> OK, fair enough.
> 
> > > Furthermore, functions called with talloc_ctx arguments require intimate
> > > knowledge of the memory context passed in by the caller to ensure they
> > > do not exceed any pre-imposed limits.
> > 
> > It requires to be careful, and should certainly not be adopted too liberally.
> > 
> > > Finally, increased reliance is placed on graceful memory allocation
> > > failure error handling, a code path which often receives little testing.
> > 
> > This is true, however what better way than incresing test coverage than having a
> > way to test with random limits to improve these error handling paths ? :-)
> 
> Sounds painful :-). We could also inject random failures at the malloc
> layer.

We could but a memory limit is more realistic and I think simulates more closely
a real situation.

> It's not our current convention, but my preference is to panic or abort
> on memory allocation failures, graceful handling is just too hard /
> time-consuming to test and maintain.

That may be fine in some applications, but I prefer handling it gracefully, especially
in samba where talloc gives us good ay to clean up. We have all the tools we need in
samba by way of taloc to gracefully handle cases where we are low on memory.

Simo.

-- 
Simo Sorce       idra at samba.org
-------------------------------
Samba Team http://www.samba.org