[PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread Eric Dumazet 
Hi David

While browsing include/net/request_sock.h I found this suspicious locking 
protecting the SYN table hash table. I think this patch is necessary.

Thank you

Signed-off-by: Eric Dumazet [EMAIL PROTECTED]
--- linux-2.6.18/include/net/request_sock.h.orig2006-10-16 
10:53:11.0 +0200
+++ linux-2.6.18-ed/include/net/request_sock.h  2006-10-16 10:53:24.0 
+0200
@@ -251,9 +251,9 @@
req-expires = jiffies + timeout;
req-retrans = 0;
req-sk = NULL;
-   req-dl_next = lopt-syn_table[hash];
 
write_lock(queue-syn_wait_lock);
+   req-dl_next = lopt-syn_table[hash];
lopt-syn_table[hash] = req;
write_unlock(queue-syn_wait_lock);
 }

[PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread Eric Dumazet 
(Sorry, patch inlined this time)

Hi David

While browsing include/net/request_sock.h I found this suspicious locking
protecting the SYN table hash table. I think this patch is necessary.

Thank you

Signed-off-by: Eric Dumazet [EMAIL PROTECTED]
--- linux-2.6.18/include/net/request_sock.h.orig2006-10-16 
10:53:11.0 +0200
+++ linux-2.6.18-ed/include/net/request_sock.h  2006-10-16 10:53:24.0 
+0200
@@ -251,9 +251,9 @@
req-expires = jiffies + timeout;
req-retrans = 0;
req-sk = NULL;
-   req-dl_next = lopt-syn_table[hash];
 
write_lock(queue-syn_wait_lock);
+   req-dl_next = lopt-syn_table[hash];
lopt-syn_table[hash] = req;
write_unlock(queue-syn_wait_lock);
 }

Re: [PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread Arnaldo Carvalho de Melo 

On 10/16/06, Eric Dumazet [EMAIL PROTECTED] wrote:

(Sorry, patch inlined this time)

Hi David

While browsing include/net/request_sock.h I found this suspicious locking
protecting the SYN table hash table. I think this patch is necessary.

Thank you


Interesting, just checked and it was there before I moved this out of tcp land:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=0e87506fcc734647c7b2497eee4eb81e785c857a

@@ -898,18 +898,10 @@ static struct request_sock *tcp_v4_searc
static void tcp_v4_synq_add(struct sock *sk, struct request_sock *req)
{
 struct tcp_sock *tp = tcp_sk(sk);
-struct tcp_listen_opt *lopt = tp-listen_opt;
+   struct tcp_listen_opt *lopt = tp-accept_queue.listen_opt;
u32 h = tcp_v4_synq_hash(inet_rsk(req)-rmt_addr,
inet_rsk(req)-rmt_port, lopt-hash_rnd);
-req-expires = jiffies + TCP_TIMEOUT_INIT;
-req-retrans = 0;
-req-sk = NULL;
-req-dl_next = lopt-syn_table[h];
-
-write_lock(tp-syn_wait_lock);
-lopt-syn_table[h] = req;
-write_unlock(tp-syn_wait_lock);
-
+reqsk_queue_hash_req(tp-accept_queue, h, req, TCP_TIMEOUT_INIT);
 tcp_synq_added(sk);
}



Signed-off-by: Eric Dumazet [EMAIL PROTECTED]


--- linux-2.6.18/include/net/request_sock.h.orig2006-10-16 
10:53:11.0 +0200
+++ linux-2.6.18-ed/include/net/request_sock.h  2006-10-16 10:53:24.0 
+0200
@@ -251,9 +251,9 @@
req-expires = jiffies + timeout;
req-retrans = 0;
req-sk = NULL;
-   req-dl_next = lopt-syn_table[hash];

write_lock(queue-syn_wait_lock);
+   req-dl_next = lopt-syn_table[hash];
lopt-syn_table[hash] = req;
write_unlock(queue-syn_wait_lock);
 }

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread Eric Dumazet 
On Monday 16 October 2006 18:16, Arnaldo Carvalho de Melo wrote:
 On 10/16/06, Eric Dumazet [EMAIL PROTECTED] wrote:
  (Sorry, patch inlined this time)
 
  Hi David
 
  While browsing include/net/request_sock.h I found this suspicious locking
  protecting the SYN table hash table. I think this patch is necessary.
 
  Thank you

 Interesting, just checked and it was there before I moved this out of tcp
 land:

Well, the bug was there before you put your hands on the code (I checked 
linux-2.4.33  linux-2.4.1 , bug present on both versions)

:)

Eric
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread Eric Dumazet 
On Monday 16 October 2006 18:56, Eric Dumazet wrote:
 On Monday 16 October 2006 18:16, Arnaldo Carvalho de Melo wrote:
  On 10/16/06, Eric Dumazet [EMAIL PROTECTED] wrote:
   (Sorry, patch inlined this time)
  
   Hi David
  
   While browsing include/net/request_sock.h I found this suspicious
   locking protecting the SYN table hash table. I think this patch is
   necessary.
  
   Thank you
 
  Interesting, just checked and it was there before I moved this out of tcp
  land:

 Well, the bug was there before you put your hands on the code (I checked
 linux-2.4.33  linux-2.4.1 , bug present on both versions)

Well, 'bug' is not appropriate in fact. Overkill maybe ? 

The comment from include/net/request_sock.h explain the thing...

 * %syn_wait_lock is necessary only to avoid proc interface having to grab the 
main
 * lock sock while browsing the listening hash (otherwise it's deadlock 
prone).
 *
 * This lock is acquired in read mode only from listening_get_next() seq_file
 * op and it's acquired in write mode _only_ from code that is actively
 * changing rskq_accept_head. All readers that are holding the master sock 
lock
 * don't need to grab this lock in read mode too as rskq_accept_head. writes
 * are always protected from the main sock lock.

I bet a more appropriate code (and less prone to reading errors for kernel 
gurus/newbies) would be :

What do you think ?

Signed-off-by: Eric Dumazet [EMAIL PROTECTED]
--- linux-2.6.19-rc2/include/net/request_sock.h 2006-10-13 18:25:04.0 
+0200
+++ linux-2.6.19-rc2-ed/include/net/request_sock.h  2006-10-16 
19:34:19.0 +0200
@@ -254,9 +254,13 @@
req-sk = NULL;
req-dl_next = lopt-syn_table[hash];
 
-   write_lock(queue-syn_wait_lock);
+   /*
+* We want previous writes being commited before doing this change,
+* so that readers of the chain are not confused.
+*/
+   smp_mb();
+
lopt-syn_table[hash] = req;
-   write_unlock(queue-syn_wait_lock);
 }
 
 #endif /* _REQUEST_SOCK_H */

Re: [PATCH] NET : Suspicious locking in reqsk_queue_hash_req()

2006-10-16 Thread David Miller 
From: Eric Dumazet [EMAIL PROTECTED]
Date: Mon, 16 Oct 2006 11:00:22 +0200

 While browsing include/net/request_sock.h I found this suspicious locking 
 protecting the SYN table hash table. I think this patch is necessary.
 
 Signed-off-by: Eric Dumazet [EMAIL PROTECTED]

People get tripped up by this one all the time.

We hold a higher level lock which protects other
inserts from happening, namely the listening socket
lock, it works here like the RTNL semaphore does.

We only need to protect the actual change of the hash
head, as lookups can occur asynchronously and we want
linkage seen by lookups to be consistent.

Alexey likes to do this locking trick a lot.

Feel free to add a comment. :-)

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html