locking issue in ip_conntrack_alter_reply

locking issue in ip_conntrack_alter_reply

[Pablo Neira]

Hi everyone,

I submitted a patch for a similar issue not a long time ago. See:

http://lists.netfilter.org/pipermail/netfilter-devel/2004-March/014764.html

Well, actually I found a contradiction here, we write-locked the 
conntrack table but that assertion says that this conntrack is not 
confirmed, this implies that the conntrack is not in the hash table yet. 
If that assertion is right, we should replace that write_lock for a 
read_lock while calling __ip_conntrack_find and remove that write_lock 
at the end of the function.

int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
                             const struct ip_conntrack_tuple *newreply)
{
        WRITE_LOCK(&ip_conntrack_lock);
        if (__ip_conntrack_find(newreply, conntrack)) {
                WRITE_UNLOCK(&ip_conntrack_lock);
                return 0;
        }
        /* Should be unconfirmed, so not in hash table yet */
        IP_NF_ASSERT(!is_confirmed(conntrack)); <--------------------------


I'll study if that assertion is right before submitting a patch. 
Feedback is always welcome :-).

regards,
Pablo

Re: locking issue in ip_conntrack_alter_reply

[Jozsef Kadlecsik]

Hi Pablo,

On Fri, 30 Apr 2004, Pablo Neira wrote:

> Well, actually I found a contradiction here, we write-locked the
> conntrack table but that assertion says that this conntrack is not
> confirmed, this implies that the conntrack is not in the hash table yet.
> If that assertion is right, we should replace that write_lock for a
> read_lock while calling __ip_conntrack_find and remove that write_lock
> at the end of the function.
>
> int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
>                              const struct ip_conntrack_tuple *newreply)
> {
>         WRITE_LOCK(&ip_conntrack_lock);
>         if (__ip_conntrack_find(newreply, conntrack)) {
>                 WRITE_UNLOCK(&ip_conntrack_lock);
>                 return 0;
>         }
>         /* Should be unconfirmed, so not in hash table yet */
>         IP_NF_ASSERT(!is_confirmed(conntrack)); <--------------------------
>
>
> I'll study if that assertion is right before submitting a patch.

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.]

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: locking issue in ip_conntrack_alter_reply

[Patrick McHardy]

Jozsef Kadlecsik wrote:

>On Fri, 30 Apr 2004, Pablo Neira wrote:
>
>  
>
>>I'll study if that assertion is right before submitting a patch.
>>    
>>
>
>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.

Regards
Patrick

>Best regards,
>Jozsef
>-
>E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
>PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
>Address : KFKI Research Institute for Particle and Nuclear Physics
>          H-1525 Budapest 114, POB. 49, Hungary
>
>  
>

Re: locking issue in ip_conntrack_alter_reply

[Pablo Neira]

[-- 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;
 }

Re: locking issue in ip_conntrack_alter_reply

[Henrik Nordstrom]

On Mon, 3 May 2004, Pablo Neira wrote:

> 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.

Isn't there a LOCK/UNLOCK imbalance in your patch?

Regards
Henrik

Re: locking issue in ip_conntrack_alter_reply

[Pablo Neira]

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

Hi,

Henrik Nordstrom wrote:

>>Attached the patch to fix this.
>>    
>>
>
>Isn't there a LOCK/UNLOCK imbalance in your patch?
>  
>

you are right, I should rush... :-( Sorry!

regards,
Pablo


[-- Attachment #2: alter_reply_locking.patch --]
[-- Type: text/plain, Size: 1033 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 13:23:43 -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,7 @@
 		conntrack->helper = LIST_FIND(&helpers, helper_cmp,
 					      struct ip_conntrack_helper *,
 					      newreply);
-	WRITE_UNLOCK(&ip_conntrack_lock);
+	READ_UNLOCK(&ip_conntrack_lock);
 
 	return 1;
 }

Re: locking issue in ip_conntrack_alter_reply

[Jozsef Kadlecsik]

On Mon, 3 May 2004, Patrick McHardy wrote:

> I've ported your conntrack patches to 2.6, I will include this change
> before adding them to CVS.

Thanks! Finally, finally I'm before a a big pom-ng
(runme/Netfilter_POM.pm) update, stay tuned :-)

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: locking issue in ip_conntrack_alter_reply

[KOVACS Krisztian]

  Hi,

2004-05-03, h keltezéssel 11:05-kor Jozsef Kadlecsik ezt írta:
> > Well, actually I found a contradiction here, we write-locked the
> > conntrack table but that assertion says that this conntrack is not
> > confirmed, this implies that the conntrack is not in the hash table yet.
> > If that assertion is right, we should replace that write_lock for a
> > read_lock while calling __ip_conntrack_find and remove that write_lock
> > at the end of the function.
> >
> > int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
> >                              const struct ip_conntrack_tuple *newreply)
> > {
> >         WRITE_LOCK(&ip_conntrack_lock);
> >         if (__ip_conntrack_find(newreply, conntrack)) {
> >                 WRITE_UNLOCK(&ip_conntrack_lock);
> >                 return 0;
> >         }
> >         /* Should be unconfirmed, so not in hash table yet */
> >         IP_NF_ASSERT(!is_confirmed(conntrack)); <--------------------------
> >
> >
> > I'll study if that assertion is right before submitting a patch.
> 
> 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.]

  Why is the whole check necessary here? ip_conntrack_alter_reply() is
called by ip_nat_setup_info() to change the reply tuple of the
conntrack. However, by assuring that there is no clashing conntrack
entry at this time, we gain nothing. get_unique_tuple() tries to return
such a tuple, and ip_conntrack_alter_reply() checks once more in vain: a
new clashing conntrack entry can be created and confirmed before
confirmation of this conntrack.

  So I'd say this hash lookup should be removed completely.

-- 
 Regards,
   Krisztian KOVACS