New talloc feature: memlimits

[David Disseldorp]

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.

>  
> > 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?

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

> > 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.
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.

Cheers, David