Re: locking issue in ip_conntrack_alter_reply

[Pablo Neira <pablo@eurodev.net>]

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

Hi Patrick,

Patrick McHardy wrote:

> Jozsef Kadlecsik wrote:
>
>> The assertion is right, because we alter unconfirmed packets when 
>> setting
>> up nat info. And you're also right, write-locking is unnecessary: the
>> entry is not in the hash table yet, so nobody else can grab and 
>> modify the
>> conntrack entry. [And it is overlooked in the per bucket locking 
>> patch too.]
>
>
> I've ported your conntrack patches to 2.6, I will include this change
> before adding them to CVS.


Attached the patch to fix this.

regards,
Pablo

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

Index: net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
RCS file: /home/cvs/linux-2.6/net/ipv4/netfilter/ip_conntrack_core.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	2 May 2004 13:26:10 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	3 May 2004 12:59:22 -0000
@@ -1093,9 +1093,9 @@
 int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
 			     const struct ip_conntrack_tuple *newreply)
 {
-	WRITE_LOCK(&ip_conntrack_lock);
+	READ_LOCK(&ip_conntrack_lock);
 	if (__ip_conntrack_find(newreply, conntrack)) {
-		WRITE_UNLOCK(&ip_conntrack_lock);
+		READ_UNLOCK(&ip_conntrack_lock);
 		return 0;
 	}
 	/* Should be unconfirmed, so not in hash table yet */
@@ -1109,7 +1109,6 @@
 		conntrack->helper = LIST_FIND(&helpers, helper_cmp,
 					      struct ip_conntrack_helper *,
 					      newreply);
-	WRITE_UNLOCK(&ip_conntrack_lock);
 
 	return 1;
 }