netdev - Re: [PATCH v2] netfilter: save the hash of the tuple in the original direction for latter use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 19 Aug 2010 23:35:23 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	Patrick McHardy <kaber@...sh.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <eric.dumazet@...il.com>,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2] netfilter: save the hash of the tuple in the original
 direction for latter use

On Thu, Aug 19, 2010 at 11:21 PM, Mathieu Desnoyers
<mathieu.desnoyers@...ymtl.ca> wrote:
> * Changli Gao (xiaosuo@...il.com) wrote:
>> Since we don't change the tuple in the original direction, we can save it
>> in ct->tuplehash[IP_CT_DIR_REPLY].hnode.pprev for __nf_conntrack_confirm()
>> use.
>>
>> __hash_conntrack() is split into two steps: ____hash_conntrack() is used
>> to get the raw hash, and __hash_bucket() is used to get the bucket id.
>>
>> In SYN-flood case, early_drop() doesn't need to recompute the hash again.
>>
>> Signed-off-by: Changli Gao <xiaosuo@...il.com>
>> ---
>> v2: use cmpxchg() to save 2 variables.
>>  net/netfilter/nf_conntrack_core.c |  114 ++++++++++++++++++++++++++------------
>>  1 file changed, 79 insertions(+), 35 deletions(-)
>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>> index df3eedb..0e02205 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -65,14 +65,20 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max);
>>  DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
>>  EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
>>
>> -static int nf_conntrack_hash_rnd_initted;
>> -static unsigned int nf_conntrack_hash_rnd;
>> -
>> -static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
>> -                               u16 zone, unsigned int size, unsigned int rnd)
>> +static u32 ____hash_conntrack(const struct nf_conntrack_tuple *tuple, u16 zone)
>>  {
>>       unsigned int n;
>>       u_int32_t h;
>> +     static unsigned int rnd;
>
> Why are you putting a hidden static variable here ? It should probably
> be declared outside of the function scope.

It is only used in this function.

>
>> +
>> +     if (unlikely(!rnd)) {
>> +             unsigned int rand;
>> +
>> +             get_random_bytes(&rand, sizeof(rand));
>> +             if (!rand)
>> +                     rand = 1;
>> +             cmpxchg(&rnd, 0, rand);
>
> This really belongs to a "init()" function, not in a dynamic check in
> the __hash_conntrack hot path. If you do that a init time, then you
> don't even need the cmpxchg.
>
> Or maybe I completely misunderstand you goal here; maybe you are really
> trying to re-calculate random bytes. But more explanation is called for,
> because I really don't see where the value is brought back to 0.
>

In fact, I don't know the reason clearly. This code is derived from
the older one. Maybe there isn't enough entropy when initializing.

-- 
Regards,
Changli Gao(xiaosuo@...il.com)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html