Skip to contentSkip to conversation

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

PATCH In review Samuel Jean 3 participants 9 messages Jan 23, 2005 – Feb 2, 2005

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Samuel Jean

author
#0 lore raw

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

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 <[email protected]>

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

ipv4/netfilter/ipt_hashlimit.c renamed from ipv4/netfilter/ipt_hashlimit.c.orig +4 -3
@@ -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,
#1 lore raw
Quoted 6 lines · Samuel Jean

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 <[email protected]>

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 <[email protected]>
Signature · 5 lines
- Harald Welte <[email protected]>             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
#2 lore raw
Quoted 6 lines · Harald Welte
Quoted 4 lines · Samuel Jean

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 <[email protected]>
Quoted 6 lines
Quoted 3 lines

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

#3 lore raw
Quoted 7 lines · Patrick McHardy
Quoted 5 lines
Quoted 2 lines

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.

Signature · 5 lines
- Harald Welte <[email protected]>             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
#4 lore raw
Quoted 10 lines · Harald Welte
Quoted 3 lines · Patrick McHardy

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

author
#5 lore raw
Quoted 3 lines · Patrick McHardy

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.

Quoted 3 lines

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.

#6 lore raw
Quoted 9 lines · Samuel Jean
Quoted 5 lines · Patrick McHardy

ipt_hashlimit already uses %ld. Samuel, what problem are you seeing exactly ?
Is it negative times or wrapped times ?

Quoted 5 lines

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

author
#7 lore raw
Quoted 3 lines · Patrick McHardy

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
Quoted 4 lines

Regards
Patrick

Hope this can help,

Samuel

#8 lore raw
Quoted 3 lines · Samuel Jean
# 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

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/02/02 02:59:11+01:00 [email protected]
# [NETFILTER]: Use correct types in seq_printf calls
#
# Signed-off-by: Patrick McHardy <[email protected]>
#

# net/ipv4/netfilter/ipt_hashlimit.c
#   2005/02/02 02:59:01+01:00 [email protected] +1 -1
#   [NETFILTER]: Use correct types in seq_printf calls
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/ipv4/netfilter/ip_conntrack_standalone.c
#   2005/02/02 02:59:01+01:00 [email protected] +6 -5
#   [NETFILTER]: Use correct types in seq_printf calls
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
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, "- ");

net/ipv4/netfilter/ipt_hashlimit.c +1 -1
diff -Nru a/net/ipv4/netfilter/ipt_hashlimit.c b/net/ipv4/netfilter/ipt_hashlimit.c
@@ -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,