Messages in this thread Patch in this message | | | Date | Thu, 07 Feb 2008 10:40:19 +0100 | From | Eric Dumazet <> | Subject | Re: [PATCH] Add IPv6 support to TCP SYN cookies |
| |
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>
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; | |