Recent match jiffies wrap mismatches

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

Date: Wed, 23 Nov 2005 12:03:11 -0800
From: Phil Oester <[email protected]>
To: [email protected]
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

diff -ru linux-orig/net/ipv4/netfilter/ipt_recent.c linux-new/net/ipv4/netfilter/ipt_recent.c
@@ -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 -----

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;
}

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.

(this file could seriously use a pass through Lindent)

Phil

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

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

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

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