[PATCH] Small SMP-race found in ip_conntrack_proto_tcp.c

[PATCH] Small SMP-race found in ip_conntrack_proto_tcp.c

[Martin Josefsson]

Hi,

I just found a small SMP-race when looking at an ugly lock (tcp_lock) in
ip_conntrack_proto_tcp.c

we use conntrack->proto.tcp.handshake_ack without holding the lock. This
is a very small race as it only applies to the ACK-packet in the
three-way handshake and there will probably only be one of those.

Patch to move the unlock further down is attached, I unlock before
calling ip_ct_refresh as it is unfriendly to hold the lock longer than
neccessary.

I believe this lock can be removed completely in Rusty's
optmization-patches, I'll send a patch for that aswell later.

-- 
/Martin

Never argue with an idiot. They drag you down to their level, then beat
you with experience.

Re: [PATCH] Small SMP-race found in ip_conntrack_proto_tcp.c

[Harald Welte]

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

On Thu, Oct 24, 2002 at 05:19:14PM +0200, Martin Josefsson wrote:
> Patch to move the unlock further down is attached, I unlock before
> calling ip_ct_refresh as it is unfriendly to hold the lock longer than
> neccessary.

I can't see a patch :)

> /Martin
-- 
Live long and prosper
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Small SMP-race found in ip_conntrack_proto_tcp.c

[Martin Josefsson]

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

On Wed, 2002-10-30 at 09:48, Harald Welte wrote:
> On Thu, Oct 24, 2002 at 05:19:14PM +0200, Martin Josefsson wrote:
> > Patch to move the unlock further down is attached, I unlock before
> > calling ip_ct_refresh as it is unfriendly to hold the lock longer than
> > neccessary.
> 
> I can't see a patch :)

Damn, I was sure I attached it... well well...
Here it is.

-- 
/Martin

Never argue with an idiot. They drag you down to their level, then beat
you with experience.

[-- Attachment #2: ip_conntrack_proto_tcp-lockfix --]
[-- Type: text/plain, Size: 1016 bytes --]

--- linux-2.4.20-pre10/net/ipv4/netfilter/ip_conntrack_proto_tcp.c.meep	2002-10-24 17:05:49.000000000 +0200
+++ linux-2.4.20-pre10/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2002-10-24 17:11:19.000000000 +0200
@@ -186,13 +186,13 @@
 	    && tcph->syn && tcph->ack)
 		conntrack->proto.tcp.handshake_ack
 			= htonl(ntohl(tcph->seq) + 1);
-	WRITE_UNLOCK(&tcp_lock);
 
 	/* If only reply is a RST, we can consider ourselves not to
 	   have an established connection: this is a fairly common
 	   problem case, so we can delete the conntrack
 	   immediately.  --RR */
 	if (!(conntrack->status & IPS_SEEN_REPLY) && tcph->rst) {
+		WRITE_UNLOCK(&tcp_lock);
 		if (del_timer(&conntrack->timeout))
 			conntrack->timeout.function((unsigned long)conntrack);
 	} else {
@@ -203,6 +203,7 @@
 		    && tcph->ack_seq == conntrack->proto.tcp.handshake_ack)
 			set_bit(IPS_ASSURED_BIT, &conntrack->status);
 
+		WRITE_UNLOCK(&tcp_lock);
 		ip_ct_refresh(conntrack, tcp_timeouts[newconntrack]);
 	}