Re: [PATCH] fix mem-leak in netfilter

[Stephen Frost]

* Patrick McHardy (kaber@xxxxxxxxx) wrote:
> Anyway, here goes the first shot at a replacement, it should be fully
> compatible. Comments and testing welcome.

This patch didn't apply cleanly against 2.6.16; I didn't think there had
been other changes since then.  As it was an entire replacement I just
pulled out the '[+ ]' lines from the patch.  Hopefully this doesn't lead
to problems in my review.

It probably would have been better to integrate it with ipset, as I've
mentioned previously.  Other comments:

recent_entry_init() appears to just look for something to delete when
the maximum number of entries has been reached, starting from the hash
position of the address.  The original ipt_recent, quite intentionally,
looked for the *oldest* address to replace.  This meant that the list
only had to be large enough to cover the number of addresses seen in a
given time-period.  This change would mean that the list would need to
be large enough to hold all addresses seen always, to be able to enforce
the time-based rules ipt_recent was written for.

ie: List of 100 addresses.  Highest timeout value in the ruleset is 60
seconds.  Average of 100 individual addresses in a 60-second timeframe.
The old ipt_recent would correctly enforce the 60-second requirement in
the ruleset.  With the new version, as soon as the list was full the
next address could replace any address in the list, even if that address
was only 15 seconds old.

One way to handle this would be to track the highest time value in the
rulesets but as the ruleset is dynamic you could end up throwing away an
address which would have been caught by a rule that was about to be
added.  The old module was written with the expectation of the list
always being full and that it would only be less-than-full shortly after
booting.  By then only removing the oldest entry in the table for each
new address seen the maximum amount of time possible for the given table
size and distinct addresses seen is achieved.

The rest looks good, thanks.

	Stephen

Attachment: signature.asc
Description: Digital signature