[RESEND][PATCH] Recent match jiffies wrap mismatches

[RESEND][PATCH] Recent match jiffies wrap mismatches

[Phil Oester]

As 2.6.15-rc3 just came out, and the below fixes 3 bugzilla entries (I missed
#370 in the original report), it would be good to get this into 2.6.15 if
possible.

Phil

----- Forwarded message from Phil Oester <kernel@linuxace.com> -----

Date: Wed, 23 Nov 2005 12:03:11 -0800
From: Phil Oester <kernel@linuxace.com>
To: netfilter-devel@lists.netfilter.org
Subject: [PATCH] Recent match jiffies wrap mismatches

Around jiffies wrap time (i.e. within first 5 mins after boot), recent match
rules which contain both --seconds and --hitcount arguments experience
false matches.

This is because the last_pkts array is filled with zeros on creation, and
when comparing 'now' to 0 (+ --seconds argument), time_before_eq thinks it
has found a hit.

Below patch adds a break if the packet value is zero.  This has the unfortunate
side effect of causing mismatches if a packet was received when jiffies really
was equal to zero.  The odds of that happening are slim compared to the
problems caused by not adding the break however.  Plus, the author used
this same method just below, so it is "good enough".

This fixes bugs #383 and #395.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>




diff -ru linux-orig/net/ipv4/netfilter/ipt_recent.c linux-new/net/ipv4/netfilter/ipt_recent.c
--- linux-orig/net/ipv4/netfilter/ipt_recent.c	2005-10-27 20:02:08.000000000 -0400
+++ linux-new/net/ipv4/netfilter/ipt_recent.c	2005-11-23 13:29:29.000000000 -0500
@@ -532,6 +532,7 @@
 			}
 			if(info->seconds && info->hit_count) {
 				for(pkt_count = 0, hits_found = 0; pkt_count < ip_pkt_list_tot; pkt_count++) {
+					if(r_list[location].last_pkts[pkt_count] == 0) break;
 					if(time_before_eq(now,r_list[location].last_pkts[pkt_count]+info->seconds*HZ)) hits_found++;
 				}
 				if(hits_found >= info->hit_count) ans = !info->invert; else ans = info->invert;


----- End forwarded message -----

Re: [RESEND][PATCH] Recent match jiffies wrap mismatches

[Patrick McHardy]

Phil Oester wrote:
> Around jiffies wrap time (i.e. within first 5 mins after boot), recent match
> rules which contain both --seconds and --hitcount arguments experience
> false matches.
> 
> This is because the last_pkts array is filled with zeros on creation, and
> when comparing 'now' to 0 (+ --seconds argument), time_before_eq thinks it
> has found a hit.
> 
> Below patch adds a break if the packet value is zero.  This has the unfortunate
> side effect of causing mismatches if a packet was received when jiffies really
> was equal to zero.  The odds of that happening are slim compared to the
> problems caused by not adding the break however.  Plus, the author used
> this same method just below, so it is "good enough".

Applied, thanks. The lines just above that have the same problem, don't
they?

[slightly reformated for better readability]

if(info->seconds && !info->hit_count) {
     if(time_before_eq(now,r_list[location].last_seen+info->seconds*HZ))
         ans = !info->invert;
     else
         ans = info->invert;
}

Re: [RESEND][PATCH] Recent match jiffies wrap mismatches

[Phil Oester]

On Wed, Nov 30, 2005 at 12:03:42AM +0100, Patrick McHardy wrote:
> Applied, thanks. The lines just above that have the same problem, don't
> they?

No, since we aren't comparing to values which have been memset to zero.
We are comparing against last_seen + seconds, which will be a legitimate
value. 

> [slightly reformated for better readability]

(this file could seriously use a pass through Lindent)

Phil

Re: [RESEND][PATCH] Recent match jiffies wrap mismatches

[Patrick McHardy]

Phil Oester wrote:
> On Wed, Nov 30, 2005 at 12:03:42AM +0100, Patrick McHardy wrote:
> 
>>Applied, thanks. The lines just above that have the same problem, don't
>>they?
> 
> 
> No, since we aren't comparing to values which have been memset to zero.
> We are comparing against last_seen + seconds, which will be a legitimate
> value. 

But last_seen is initialized to 0 too. Is there something I'm missing
that prevents these entries from beeing used?

>>[slightly reformated for better readability]
> 
> 
> (this file could seriously use a pass through Lindent)

Or better some serious rewriting .. its a pile of junk.

Re: [RESEND][PATCH] Recent match jiffies wrap mismatches

[Phil Oester]

On Wed, Nov 30, 2005 at 08:43:33AM +0100, Patrick McHardy wrote:
> But last_seen is initialized to 0 too. Is there something I'm missing
> that prevents these entries from beeing used?

We only reach this section of code if an existing hash table entry is found,
and when the hash entry is created:

                r_list[location].last_seen = now;

So no - last_seen will not be unitialized.

Phil

Re: [RESEND][PATCH] Recent match jiffies wrap mismatches

[Stephen Frost]

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

* Patrick McHardy (kaber@trash.net) wrote:
> Or better some serious rewriting .. its a pile of junk.

Feel free, it's really not all that complicated.  It should be made part
of the ipset stuff though, really.  I kept hearing ipset was going to be
built in such a way so it'd be easy to incorporate things like
ipt_recent but I havn't had a chance to really look at it lately.  Last
time I looked, the new ipset was still very much 'under construction'
but I gather the new version has been incorporated now.

Sorry for the assumptions about jiffies but that's certainly an area
that likes to change every other month in the kernel and I expect the
API for them will change yet again (along with what assumptions can be
made about them).

	Enjoy,

		Stephen

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