[PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Samuel Jean]

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

Name: Expire's type overflow when showing tuples at bad moment.
Status: Tested under Linux 2.6.6 uml patched.
Signed-off-by: Samuel Jean <sjean@cookinglinux.org>

When garbage collector runs slow enough, we have a chance to print
expired tuples before they get freed. We see weird expiry time.

Not sure if I needed 'now' reference so I didn't took chance.

Index: linux-2.6.6/net/ipv4/netfilter/ipt_hashlimit.c
===================================================================


[-- Attachment #2: ipt_hashlimit.c-proc_show.patch --]
[-- Type: text/x-patch, Size: 840 bytes --]

--- net/ipv4/netfilter/ipt_hashlimit.c.orig	2005-01-22 19:48:05.000000000 -0500
+++ net/ipv4/netfilter/ipt_hashlimit.c	2005-01-22 19:50:44.000000000 -0500
@@ -605,11 +605,12 @@ static void dl_seq_stop(struct seq_file 
 
 static inline int dl_seq_real_show(struct dsthash_ent *ent, struct seq_file *s)
 {
+	unsigned int now = jiffies;
 	/* recalculate to show accurate numbers */
-	rateinfo_recalc(ent, jiffies);
+	rateinfo_recalc(ent, now);
 
-	return seq_printf(s, "%ld %u.%u.%u.%u:%u->%u.%u.%u.%u:%u %u %u %u\n",
-			(ent->expires - jiffies)/HZ,
+	return seq_printf(s, "%lu %u.%u.%u.%u:%u->%u.%u.%u.%u:%u %u %u %u\n",
+			(ent->expires > now) ? (ent->expires - now)/HZ : 0,
 			NIPQUAD(ent->dst.src_ip), ntohs(ent->dst.src_port),
 			NIPQUAD(ent->dst.dst_ip), ntohs(ent->dst.dst_port),
 			ent->rateinfo.credit, ent->rateinfo.credit_cap,

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Harald Welte]

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

On Sat, Jan 22, 2005 at 08:32:02PM -0500, Samuel Jean wrote:
> Name: Expire's type overflow when showing tuples at bad moment.
> Status: Tested under Linux 2.6.6 uml patched.
> Signed-off-by: Samuel Jean <sjean@cookinglinux.org>
> 
> When garbage collector runs slow enough, we have a chance to print
> expired tuples before they get freed. We see weird expiry time.

Thanks.  Patrick, please put this patch in your pending queue.

Signed-off-by: Harald Welte <laforge@netfilter.org>

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Patrick McHardy]

Harald Welte wrote:

>On Sat, Jan 22, 2005 at 08:32:02PM -0500, Samuel Jean wrote:
>
>>Name: Expire's type overflow when showing tuples at bad moment.
>>Status: Tested under Linux 2.6.6 uml patched.
>>Signed-off-by: Samuel Jean <sjean@cookinglinux.org>
>>
>>When garbage collector runs slow enough, we have a chance to print
>>expired tuples before they get freed. We see weird expiry time.
>>
>
>Thanks.  Patrick, please put this patch in your pending queue.
>
We also have this problem in ip_conntrack_standalone. I think the best
fix would be to use %d for time deltas.

Regards
Patrick

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Harald Welte]

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

On Tue, Feb 01, 2005 at 02:43:25PM +0100, Patrick McHardy wrote:
> >>When garbage collector runs slow enough, we have a chance to print
> >>expired tuples before they get freed. We see weird expiry time.
> >
> >Thanks.  Patrick, please put this patch in your pending queue.
> >
> We also have this problem in ip_conntrack_standalone. I think the best
> fix would be to use %d for time deltas.

ACK.   we should never have times larger than five days anyway... way
less than the negative maximum of 32bit.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Patrick McHardy]

Harald Welte wrote:

>On Tue, Feb 01, 2005 at 02:43:25PM +0100, Patrick McHardy wrote:
>  
>
>>We also have this problem in ip_conntrack_standalone. I think the best
>>fix would be to use %d for time deltas.
>>
>
>ACK.   we should never have times larger than five days anyway... way
>less than the negative maximum of 32bit.
>
ipt_hashlimit already uses %ld. Samuel, what problem are you seeing 
exactly ?
Is it negative times or wrapped times ?

Regards
Patrick

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Samuel Jean]

On Tue, February 1, 2005 9:20 am, Patrick McHardy said:
> ipt_hashlimit already uses %ld. Samuel, what problem are you seeing
> exactly ?
> Is it negative times or wrapped times ?

I see wrapped time.

I've tried making it print negative values, but seems I can't get
unsigned operations to result in a signed value. Is that right ?

However,

int r = (unsigned - unsigned) gave me negative value.

>
> Regards
> Patrick

Have a nice day,

Samuel

P.S.: Please disregard that patch, I`ve send another one [PATCH 1/2] which
doesn't use unecessary variable, I think. Thank you.

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Patrick McHardy]

Samuel Jean wrote:

>On Tue, February 1, 2005 9:20 am, Patrick McHardy said:
>  
>
>>ipt_hashlimit already uses %ld. Samuel, what problem are you seeing
>>exactly ?
>>Is it negative times or wrapped times ?
>>    
>>
>
>I see wrapped time.
>
>I've tried making it print negative values, but seems I can't get
>unsigned operations to result in a signed value. Is that right ?
>
No. On my system va_arg (used by vsnprintf, used by seq_sprintf)
behaves strangely. From vsnprintf:

                else if (qualifier == 'l') {
                        num = va_arg(args, unsigned long);
                        if (flags & SIGN)
                                num = (signed long) num;

On my system (x86_64, gcc-3.3.5) va_arg returns 2^32-1 for -1, but it
should return 2^64-1. Are you seeing this problem on a 32bit or a 64bit
machine ?

Regards
Patrick

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Samuel Jean]

Patrick McHardy wrote:
> On my system (x86_64, gcc-3.3.5) va_arg returns 2^32-1 for -1, but it
> should return 2^64-1. Are you seeing this problem on a 32bit or a 64bit
> machine ?

It's on x86 32bits, gcc-3.3.4.
Here's an exemple of consecutive cat:

# cat /proc/net/ipt_hashlimit/moo
0 x.239.59.104:0->0.0.0.0:0 1600 1600 320
0 x.162.158.131:0->0.0.0.0:0 1600 1600 320

# cat /proc/net/ipt_hashlimit/moo
4294967 x.239.59.104:0->0.0.0.0:0 1600 1600 320
4294967 x.162.158.131:0->0.0.0.0:0 1600 1600 320

# cat /proc/net/ipt_hashlimit/moo
#

 From what I can think, it goes bad only when a third operation comes into.
Here's what happen without dividing by HZ.

# cat /proc/net/ipt_hashlimit/moo
6 206.162.158.131:0->0.0.0.0:0 1600 1600 320

# cat /proc/net/ipt_hashlimit/moo
-447 206.162.158.131:0->0.0.0.0:0 1600 1600 320


> 
> Regards
> Patrick
> 
Hope this can help,

Samuel

Re: [PATCH] ipt_hashlimit.c: expire's type overflow when showing tuples at bad moment.

[Patrick McHardy]

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

Samuel Jean wrote:

> # cat /proc/net/ipt_hashlimit/moo
> 4294967 x.239.59.104:0->0.0.0.0:0 1600 1600 320
> 4294967 x.162.158.131:0->0.0.0.0:0 1600 1600 320

Apparently we saw two different problems. The problem I saw on my
64 bit system was caused by my false assumption that gcc would
promote arguments to functions declared with attribute(format(printf))
to the types specified in the format string. It does not, so -1
was passed as signed int, but interpreted as an signed long.
The reason for the problem you are seeing is that the result of
an arithmetic operation where both operands have equal types has
the same type as the operands. This means the subtraction of
two unsigned longs yields an unsigned long. The division by HZ
then makes it small enough so it looses the sign bit.

This patch should fix the problem.

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2554 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/02 02:59:11+01:00 kaber@coreworks.de 
#   [NETFILTER]: Use correct types in seq_printf calls
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/netfilter/ipt_hashlimit.c
#   2005/02/02 02:59:01+01:00 kaber@coreworks.de +1 -1
#   [NETFILTER]: Use correct types in seq_printf calls
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/netfilter/ip_conntrack_standalone.c
#   2005/02/02 02:59:01+01:00 kaber@coreworks.de +6 -5
#   [NETFILTER]: Use correct types in seq_printf calls
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/netfilter/ip_conntrack_standalone.c b/net/ipv4/netfilter/ip_conntrack_standalone.c
--- a/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-02-02 03:03:08 +01:00
+++ b/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-02-02 03:03:08 +01:00
@@ -115,11 +115,12 @@
 			       .tuple.dst.protonum);
 	IP_NF_ASSERT(proto);
 
-	if (seq_printf(s, "%-8s %u %lu ",
+	if (seq_printf(s, "%-8s %u %ld ",
 		      proto->name,
 		      conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum,
 		      timer_pending(&conntrack->timeout)
-		      ? (conntrack->timeout.expires - jiffies)/HZ : 0) != 0)
+		      ? (long)(conntrack->timeout.expires - jiffies)/HZ
+		      : 0) != 0)
 		return 1;
 
 	if (proto->print_conntrack(s, conntrack))
@@ -148,7 +149,7 @@
 			return 1;
 
 #if defined(CONFIG_IP_NF_CONNTRACK_MARK)
-	if (seq_printf(s, "mark=%ld ", conntrack->mark))
+	if (seq_printf(s, "mark=%lu ", conntrack->mark))
 		return 1;
 #endif
 
@@ -235,8 +236,8 @@
 	struct ip_conntrack_expect *expect = v;
 
 	if (expect->timeout.function)
-		seq_printf(s, "%lu ", timer_pending(&expect->timeout)
-			   ? (expect->timeout.expires - jiffies)/HZ : 0);
+		seq_printf(s, "%ld ", timer_pending(&expect->timeout)
+			   ? (long)(expect->timeout.expires - jiffies)/HZ : 0);
 	else
 		seq_printf(s, "- ");
 
diff -Nru a/net/ipv4/netfilter/ipt_hashlimit.c b/net/ipv4/netfilter/ipt_hashlimit.c
--- a/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-02 03:03:08 +01:00
+++ b/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-02 03:03:08 +01:00
@@ -609,7 +609,7 @@
 	rateinfo_recalc(ent, jiffies);
 
 	return seq_printf(s, "%ld %u.%u.%u.%u:%u->%u.%u.%u.%u:%u %u %u %u\n",
-			(ent->expires - jiffies)/HZ,
+			(long)(ent->expires - jiffies)/HZ,
 			NIPQUAD(ent->dst.src_ip), ntohs(ent->dst.src_port),
 			NIPQUAD(ent->dst.dst_ip), ntohs(ent->dst.dst_port),
 			ent->rateinfo.credit, ent->rateinfo.credit_cap,