Re: [PATCH] Add IPv6 support to TCP SYN cookies

[Eric Dumazet <dada1@cosmosbay.com>]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

Evgeniy Polyakov a écrit :
> On Wed, Feb 06, 2008 at 10:30:24AM -0800, Glenn Griffin (ggriffin.kernel@gmail.com) wrote:
>   
>>>> +static u32 cookie_hash(struct in6_addr *saddr, struct in6_addr *daddr,
>>>> +		       __be16 sport, __be16 dport, u32 count, int c)
>>>> +{
>>>> +	__u32 tmp[16 + 5 + SHA_WORKSPACE_WORDS];
>>>>         
>>> This huge buffer should not be allocated on stack.
>>>       
>> I can replace it will a kmalloc, but for my benefit what's the practical
>> size we try and limit the stack to?  It seemed at first glance to me
>> that 404 bytes plus the arguments, etc. was not such a large buffer for
>> a non-recursive function.  Plus the alternative with a kmalloc requires
>>     
>
> Well, maybe for connection establishment path it is not, but it is
> absolutely the case in the sending and sometimes receiving pathes for 4k
> stacks. The main problem is that bugs which happen because of stack
> overflow are so much obscure, that it is virtually impossible to detect
> where overflow happend. 'Debug stack overflow' somehow does not help to
> detect it.
>
> Usually there is about 1-1.5 kb of free stack for each process, so this
> change will cut one third of the free stack, getting into account that
> something can store ipv6 addresses on stack too, this can end up badly.
>
>   
>> propogating the possible error status back up to tcp_ipv6.c in the event
>> we are unable to allocate enough memory, so it can simply drop the
>> connection.  Not an impossible task by any means but it does
>> significantly complicate things and I would like to know it's worth the
>> effort.  Also would it be worth it to provide a supplemental patch for
>> the ipv4 implementation as it allocates the same buffer?
>>     
>
> One can reorganize syncookie support to work with request hash tables
> too, so that we could allocate per hash-bucket space and use it as a
> scratchpad for cookies.
>
>   
Or maybe use percpu storage for that...

I am not sure if cookie_hash() is always called with preemption disabled.
(If not, we have to use get_cpu_var()/put_cpu_var())

[NET] IPV4: lower stack usage in cookie_hash() function

400 bytes allocated on stack might be a litle bit too much. Using a 
per_cpu var is more friendly.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>



[-- Attachment #2: cookie_scratch.patch --]
[-- Type: text/plain, Size: 681 bytes --]

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index f470fe4..177da14 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -35,10 +35,12 @@ module_init(init_syncookies);
 #define COOKIEBITS 24	/* Upper bits store count */
 #define COOKIEMASK (((__u32)1 << COOKIEBITS) - 1)
 
+static DEFINE_PER_CPU(__u32, cookie_scratch)[16 + 5 + SHA_WORKSPACE_WORDS];
+
 static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16 dport,
 		       u32 count, int c)
 {
-	__u32 tmp[16 + 5 + SHA_WORKSPACE_WORDS];
+	__u32 *tmp = __get_cpu_var(cookie_scratch);
 
 	memcpy(tmp + 3, syncookie_secret[c], sizeof(syncookie_secret[c]));
 	tmp[0] = (__force u32)saddr;