[PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Dong Wei]

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

Hi, all
  In xt_connlimit match module, the counter of an IP is decreased when
the TCP packet is go through the chain with ip_conntrack state TW.
Well, it's very natural that the server and client close the socket
with FIN packet. But when the client/server close the socket with RST
packet(using so_linger), the counter for this connection still exsit.
The following patch can fix it which is based on linux-2.6.25.4

diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
--- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000 +0800
+++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000 +0800
@@ -75,7 +75,8 @@
        u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;

        if (proto == IPPROTO_TCP)
-               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
+               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
+                       || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);
        else
                return 0;
 }

[-- Attachment #2: xt_connlimit.txt --]
[-- Type: text/plain, Size: 525 bytes --]

diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
--- a/net/netfilter/xt_connlimit.c	2008-06-02 18:48:38.000000000 +0800
+++ b/net/netfilter/xt_connlimit.c	2008-06-02 18:50:40.000000000 +0800
@@ -75,7 +75,8 @@
 	u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
 
 	if (proto == IPPROTO_TCP)
-		return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
+		return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
+		        || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);
 	else
 		return 0;
 }

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Patrick McHardy]

Dong Wei wrote:
> Hi, all
>   In xt_connlimit match module, the counter of an IP is decreased when
> the TCP packet is go through the chain with ip_conntrack state TW.
> Well, it's very natural that the server and client close the socket
> with FIN packet. But when the client/server close the socket with RST
> packet(using so_linger), the counter for this connection still exsit.
> The following patch can fix it which is based on linux-2.6.25.4
> 
> diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> --- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000 +0800
> +++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000 +0800
> @@ -75,7 +75,8 @@
>         u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
> 
>         if (proto == IPPROTO_TCP)
> -               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
> +               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
> +                       || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);

Looks fine to me. Jan?

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Jan Engelhardt]

On Monday 2008-06-02 14:20, Patrick McHardy wrote:
> Dong Wei wrote:
>> Hi, all
>>   In xt_connlimit match module, the counter of an IP is decreased when
>> the TCP packet is go through the chain with ip_conntrack state TW.
>> Well, it's very natural that the server and client close the socket
>> with FIN packet. But when the client/server close the socket with RST
>> packet(using so_linger), the counter for this connection still exsit.
>> The following patch can fix it which is based on linux-2.6.25.4
>> 
>> diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>> --- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000 +0800
>> +++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000 +0800
>> @@ -75,7 +75,8 @@
>>         u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
>> 
>>         if (proto == IPPROTO_TCP)
>> -               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
>> +               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
>> +                       || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);
>
> Looks fine to me. Jan?

The check for TCP_CONNTRACK_TIME_WAIT was introduced since there is
the 2*MSL delay before the TIME_WAIT->CLOSED transition, and not
counting a connection beginning with TIME_WAIT is common sense/what
people expect.

Though the cleanup delay between TCP_CONNTRACK_CLOSE and (deallocated
state) is much less than 2*MSL, it makes sense to also add this case
per common sense.

Patch is fine, yes, but you do not need the redundant
( ) that were introduced.

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Patrick McHardy]

Jan Engelhardt wrote:
> On Monday 2008-06-02 14:20, Patrick McHardy wrote:
>> Dong Wei wrote:
>>> diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>>> --- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000 +0800
>>> +++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000 +0800
>>> @@ -75,7 +75,8 @@
>>>         u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
>>>
>>>         if (proto == IPPROTO_TCP)
>>> -               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
>>> +               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
>>> +                       || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);
>> Looks fine to me. Jan?
> 
> The check for TCP_CONNTRACK_TIME_WAIT was introduced since there is
> the 2*MSL delay before the TIME_WAIT->CLOSED transition, and not
> counting a connection beginning with TIME_WAIT is common sense/what
> people expect.

Yes, though the end-result might not be what people expect.
The connection can be reopened, exceeding the configured
limit, and lots of TIME_WAIT/CLOSE connections might linger
around.

> Though the cleanup delay between TCP_CONNTRACK_CLOSE and (deallocated
> state) is much less than 2*MSL, it makes sense to also add this case
> per common sense.
> 
> Patch is fine, yes, but you do not need the redundant
> ( ) that were introduced.

I'll remove them when applying the patch.

Dong, I need a Signed-off-by: line from you before I can apply this.

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Dong Wei]

Hi, Jan

  My Signed-off-by message is:

Signed-off-by: Dong Wei <dwei.zh@gmail.com>

Thanks

On Mon, Jun 2, 2008 at 9:01 PM, Patrick McHardy <kaber@trash.net> wrote:
> Jan Engelhardt wrote:
>>
>> On Monday 2008-06-02 14:20, Patrick McHardy wrote:
>>>
>>> Dong Wei wrote:
>>>>
>>>> diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>>>> --- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000
>>>> +0800
>>>> +++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000
>>>> +0800
>>>> @@ -75,7 +75,8 @@
>>>>        u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
>>>>
>>>>        if (proto == IPPROTO_TCP)
>>>> -               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
>>>> +               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
>>>> +                       || conn->proto.tcp.state ==
>>>> TCP_CONNTRACK_CLOSE);
>>>
>>> Looks fine to me. Jan?
>>
>> The check for TCP_CONNTRACK_TIME_WAIT was introduced since there is
>> the 2*MSL delay before the TIME_WAIT->CLOSED transition, and not
>> counting a connection beginning with TIME_WAIT is common sense/what
>> people expect.
>
> Yes, though the end-result might not be what people expect.
> The connection can be reopened, exceeding the configured
> limit, and lots of TIME_WAIT/CLOSE connections might linger
> around.
>
>> Though the cleanup delay between TCP_CONNTRACK_CLOSE and (deallocated
>> state) is much less than 2*MSL, it makes sense to also add this case
>> per common sense.
>>
>> Patch is fine, yes, but you do not need the redundant
>> ( ) that were introduced.
>
> I'll remove them when applying the patch.
>
> Dong, I need a Signed-off-by: line from you before I can apply this.
>

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Patrick McHardy]

Dong Wei wrote:
> Hi, Jan
> 
>   My Signed-off-by message is:
> 
> Signed-off-by: Dong Wei <dwei.zh@gmail.com>

Applied, thanks.

Re: [PATCH] Fix connlimit bug when receive RST packet in ESTABLISHED state

[Jan Engelhardt]

On Monday 2008-06-02 15:01, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>> On Monday 2008-06-02 14:20, Patrick McHardy wrote:
>> > Dong Wei wrote:
>> > > diff -ruN a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>> > > --- a/net/netfilter/xt_connlimit.c      2008-06-02 18:48:38.000000000
>> > > +0800
>> > > +++ b/net/netfilter/xt_connlimit.c      2008-06-02 18:50:40.000000000
>> > > +0800
>> > > @@ -75,7 +75,8 @@
>> > >         u_int16_t proto = conn->tuplehash[0].tuple.dst.protonum;
>> > >
>> > >         if (proto == IPPROTO_TCP)
>> > > -               return conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT;
>> > > +               return (conn->proto.tcp.state == TCP_CONNTRACK_TIME_WAIT
>> > > +                       || conn->proto.tcp.state == TCP_CONNTRACK_CLOSE);
>> > Looks fine to me. Jan?

Acked-by: Jan Engelhardt <jengelh@medozas.de>