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

[Eric Dumazet <dada1@cosmosbay.com>]

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

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 <dada1@cosmosbay.com> 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 <dada1@cosmosbay.com>

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

--- linux-2.6.19-rc2/include/net/request_sock.h	2006-10-13 18:25:04.000000000 +0200
+++ linux-2.6.19-rc2-ed/include/net/request_sock.h	2006-10-16 19:34:19.000000000 +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 */