Re: xt_connlimit 20070625

[Patrick McHardy <kaber@trash.net>]

Jan Engelhardt wrote:
> On Jun 25 2007 13:41, Patrick McHardy wrote:
>   
>>> +config NETFILTER_XT_MATCH_CONNLIMIT
>>> +	tristate '"connlimit" match support"'
>>> +	depends on NETFILTER_XTABLES && NF_CONNTRACK_IPV4
>>>   
>>>       
>> This doesn't seem right for an x_tables target. Adding IPv6 looks
>> trivial, the only thing that really depends on the address length
>> seems to be the masked comparison.
>>     
>
> Indeed - leftovers from its POM ascendant. But do I have to add ipv6 now?
>
> (It would also be quite a duplicate of count_them() and connlimit_match() -
> is there a better way without too many if()s?)
>   
I'd use a conditional in count_them and add an connlimit_match6 function.


>   
>>> +static inline unsigned int connlimit_iphash(u_int32_t addr)
>>> +{
>>> +	/*
>>> +	 * Since iphash (in struct xt_connlimit_data) has 256 entries, we need
>>> +	 * to "% 256" it. "& mask" works too, but _only_ if the iphash array
>>> +	 * size is exactly a {power of two} minus 1.
>>>   
>>>       
>> Incorrect comment, it does use &.
>>     
>
> I reword it.
>
>
>   
>> Can't be NULL here.
>> OK it can. But you should probably just continue for found == NULL above.
>>     
>
> I doubt I can do that. Is is part of the destruction.
> A little more below, there is /* this one is gone */
>   
Right.

>>> +	for (i = 0; i < ARRAY_SIZE(info->data->iphash); ++i)
>>>   
>>>       
>> I prefer post-increment in all cases but those where pre-increment
>> is really needed.
>>     
>
> Can I ignore that?
>   

Yes.
>   
>>> +static struct xt_match connlimit_reg = {
>>>   
>>>       
>> __read_mostly?
>>     
>
> No other xt_ modules have it, but I can add it.
>   

They don't? I'll add it to the others.
>   
>>> +static int __init xt_connlimit_init(void)
>>> +{
>>> +	get_random_bytes(&connlimit_iphash_rnd, sizeof(connlimit_iphash_rnd));
>>>   
>>>       
>> We usually initialize jhash random values at the time the first
>> hash is calculated because there might be not enough entropy
>> available during boot.
>>     
>
> The routing code (where Eric pointed me to) does it on boot-up.
>   

It does not use get_random_bytes during boot and reinitizalizes the
random value after a flush.

> I have xt_connlimit loaded at boot (in runlevel B), there does not seem to be a
> problem.
>   

Think of a diskless system.