[PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[Martin Josefsson]

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

Hi Jozsef

Your latest patch contained a change to the RST handling.
The change was that an RST is ignored if the previous packet was an ACK.
This is happens all the time. I know it was intended as a fix for the
SYN - ACK probe - RST sequence but it breaks normal usage. The problem
is that connections that end with RST never get their state changed and
are left in ESTABLISHED state with a large timeout.

The patch below adds a check for
!test_bit(IPS_ASSURED_BIT, &conntrack->status) so your change will only
be active for unassured connections. Maybe you have a better idea for
how to fix both cases.

This patch has been tested by a user that reported the problem on irc
and it fixes the problem for him. I'm also running it on a machine with
lots of traffic and it fixes the problem for me as well.

Please make sure something that fixes the problem is submitted fairly
quickly.

<hint> A tcp-state/windowtracking testcase for nfsim would be great
</hint> :)

--- linux-2.6.10-rc3-bk14/net/ipv4/netfilter/ip_conntrack_proto_tcp.c.orig	2004-12-30 19:48:33.000000000 +0100
+++ linux-2.6.10-rc3-bk14/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-30 19:49:46.000000000 +0100
@@ -906,7 +906,8 @@ static int tcp_packet(struct ip_conntrac
 		if (index == TCP_RST_SET
 		    && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
 		         && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET)
-		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
+		        || (!test_bit(IPS_ASSURED_BIT, &conntrack->status)
+			 && conntrack->proto.tcp.last_index == TCP_ACK_SET))
 		    && after(ntohl(th->ack_seq),
 		    	     conntrack->proto.tcp.last_seq)) {
 			/* Ignore RST closing down invalid SYN or ACK

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[Jozsef Kadlecsik]

Hi Martin,

On Fri, 31 Dec 2004, Martin Josefsson wrote:

> Your latest patch contained a change to the RST handling.
> The change was that an RST is ignored if the previous packet was an ACK.
> This is happens all the time. I know it was intended as a fix for the
> SYN - ACK probe - RST sequence but it breaks normal usage. The problem
> is that connections that end with RST never get their state changed and
> are left in ESTABLISHED state with a large timeout.

Ouch!

> The patch below adds a check for
> !test_bit(IPS_ASSURED_BIT, &conntrack->status) so your change will only
> be active for unassured connections. Maybe you have a better idea for
> how to fix both cases.
>
> This patch has been tested by a user that reported the problem on irc
> and it fixes the problem for him. I'm also running it on a machine with
> lots of traffic and it fixes the problem for me as well.
>
> Please make sure something that fixes the problem is submitted fairly
> quickly.

I think your patch should  be submitted immediately. The introduced bug is
too bad to leave it linger on.

> <hint> A tcp-state/windowtracking testcase for nfsim would be great
> </hint> :)

Yep, I got Rusty's message as well :-) Time to dig out the initial tests
from the old cvs and convert them to nfsim with new cases added.

> --- linux-2.6.10-rc3-bk14/net/ipv4/netfilter/ip_conntrack_proto_tcp.c.orig	2004-12-30 19:48:33.000000000 +0100
> +++ linux-2.6.10-rc3-bk14/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-30 19:49:46.000000000 +0100
> @@ -906,7 +906,8 @@ static int tcp_packet(struct ip_conntrac
>  		if (index == TCP_RST_SET
>  		    && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
>  		         && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET)
> -		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
> +		        || (!test_bit(IPS_ASSURED_BIT, &conntrack->status)
> +			 && conntrack->proto.tcp.last_index == TCP_ACK_SET))
>  		    && after(ntohl(th->ack_seq),
>  		    	     conntrack->proto.tcp.last_seq)) {
>  			/* Ignore RST closing down invalid SYN or ACK
>

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: [PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[Martin Josefsson]

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

On Sat, 2005-01-01 at 15:59 +0100, Jozsef Kadlecsik wrote:
> Hi Martin,

Hi Jozsef

> Ouch!

I agree :)

> > Please make sure something that fixes the problem is submitted fairly
> > quickly.
> 
> I think your patch should  be submitted immediately. The introduced bug is
> too bad to leave it linger on.

I'll submit it to Andrew since Davem is on vacation.

> > <hint> A tcp-state/windowtracking testcase for nfsim would be great
> > </hint> :)
> 
> Yep, I got Rusty's message as well :-) Time to dig out the initial tests
> from the old cvs and convert them to nfsim with new cases added.

:) We need more testcases badly, for all sorts of things.

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[Patrick McHardy]

Martin Josefsson wrote:

>On Sat, 2005-01-01 at 15:59 +0100, Jozsef Kadlecsik wrote:
>
>>I think your patch should  be submitted immediately. The introduced bug is
>>too bad to leave it linger on.
>>
>
>I'll submit it to Andrew since Davem is on vacation.
>
The patch hasn't shown up in Linus's tree yet. I'm going to add it to
my tree and push it to Dave with the other patches if it didn't show up
by Tuesday.

Regards
Patrick

Re: [PATCH] Fix RST handling in ip_conntrack_proto_tcp.c

[Martin Josefsson]

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

On Mon, 2005-01-03 at 00:21 +0100, Patrick McHardy wrote:

> >I'll submit it to Andrew since Davem is on vacation.
> >
> The patch hasn't shown up in Linus's tree yet. I'm going to add it to
> my tree and push it to Dave with the other patches if it didn't show up
> by Tuesday.

Great, thanks

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]