All of lore.kernel.org
 help / color / mirror / Atom feed
* ipt_recent fix
@ 2006-02-17 17:37 Amin Azez
  2006-02-20 16:12 ` [patch] ipt_recent Amin Azez
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Amin Azez @ 2006-02-17 17:37 UTC (permalink / raw)
  To: netfilter-devel

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

Here is a fix for ipt_recent.

The problem is that curr_table[x].hash_entry=0 (for all x)

So all pristine (never used) curr_table entries claim to have hash pos 0.

So when we add new items to the hash and do:
  hash_table[r_list[location].hash_entry] = -1;
to use the oldest (used or unused) location - if it is unused we trash 
anything that really has hash pos 0 already allocated.

This can be reproduced with this:

# iptables -A FORWARD -d 1.1.1.1 -m recent --name TEST1 --set
# G=1 ; while test $G -lt 17 ; do echo $G ; echo 1.1.1.$G> \
  /proc/net/ipt_recent/TEST1 ; G=$(($G+1)) ; dmesg -c ; cat \
  /proc/net/ipt_recent/TEST1  | head ; done > x

the 13th item has hash of 0 and gets trashed when the 14th item is added 
without this patch.

We should only trash it then if we _know_ it has been used before and 
therefore is REALLY at the position it claims...

This patch makes sure that items claiming to have a hash entry really do 
have that hash entry before destroying that hash entry.

This patch also adds counting of the number of hash entries in 
preparation for conditions based on the number of entries in the recent 
table rule; patch for that to follow next week.

Any comments on this?

Sam

[-- Attachment #2: ipt_recent.patch --]
[-- Type: text/x-patch, Size: 3157 bytes --]

--- ./net/ipv4/netfilter/ipt_recent.c.nolimit	2006-02-15 16:34:20.000000000 +0000
+++ ./net/ipv4/netfilter/ipt_recent.c	2006-02-17 17:23:00.000000000 +0000
@@ -70,7 +70,11 @@
 /* Structure of our linked list of tables of recent lists. */
 struct recent_ip_tables {
 	char name[IPT_RECENT_NAME_LEN];
+	/* number of entries in list *table */
+	int entry_count;
+	/* number of reference to this structure from iptables rules */
 	int count;
+	/* an index increased with each operation which maps time_info[x].position to a position in table */
 	int time_pos;
 	struct recent_ip_list *table;
 	struct recent_ip_tables *next;
@@ -139,6 +143,7 @@
 	curr_table = (struct recent_ip_tables*) data;
 
 	spin_lock_bh(&curr_table->list_lock);
+	len += sprintf(buffer+len,"count=%d\n",curr_table->entry_count);
 	for(count = 0; count < ip_list_tot; count++) {
 		if(!curr_table->table[count].addr) continue;
 		last_len = len;
@@ -216,6 +221,7 @@
 		used += 5;
 		spin_lock_bh(&curr_table->list_lock);
 		curr_table->time_pos = 0;
+		curr_table->entry_count = 0;
 		for(count = 0; count < ip_list_hash_size; count++) {
 			curr_table->hash_table[count] = -1;
 		}
@@ -500,7 +506,27 @@
 
 		/* New item found and IPT_RECENT_SET, so we need to add it */
 		location = time_info[curr_table->time_pos].position;
-		hash_table[r_list[location].hash_entry] = -1;
+		/* 
+		   location is the next slot in r_list that we will use. 
+		   Maybe it is already used and we need to remove it fromthe hash.
+		   If it is used then: 
+		   1) hash_table[r_list[location].hash_entry]=location
+		   Which is to say that anything claiming to have a hash entry
+		   should also be pointed to BY that hash entry.
+		     This only comes up becase by default r_list[x].hash_entry=0
+		   i.e. all items claim to have hash entry 0, so if a real
+		   item has hash slot 0 it gets trashed because of lies.
+		   SO: If r_list[location].hash_entry]==0 we are in warning condition
+		   and need to double check that hash_table reciprocates before we
+		   set the hash_table entry to -1 (it may not really belong)
+		   This would work; 
+		     if (r_list[location].hash_entry!=0 || hash_table[0]==location) hash_table[r_list[location].hash_entry] = -1; 
+		   but we want to combine with a counter for new entries
+		*/
+		/* increase counter if this is a new item */
+		if (hash_table[r_list[location].hash_entry]!=location) curr_table->entry_count++;
+		else hash_table[r_list[location].hash_entry] = -1;
+
 		hash_table[hash_result] = location;
 		memset(r_list[location].last_pkts,0,ip_pkt_list_tot*sizeof(u_int32_t));
 		r_list[location].time_pos = curr_table->time_pos;
@@ -633,6 +659,7 @@
 			r_list[location].ttl = 0;
 			memset(r_list[location].last_pkts,0,ip_pkt_list_tot*sizeof(u_int32_t));
 			r_list[location].oldest_pkt = 0;
+			curr_table->entry_count--;
 			ans = !info->invert;
 		}
 		spin_unlock_bh(&curr_table->list_lock);
@@ -719,6 +746,7 @@
 	curr_table->next = NULL;
 	curr_table->count = 1;
 	curr_table->time_pos = 0;
+	curr_table->entry_count = 0;
 	strncpy(curr_table->name,info->name,IPT_RECENT_NAME_LEN);
 	curr_table->name[IPT_RECENT_NAME_LEN-1] = '\0';
 

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2006-05-03 11:41 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-17 17:37 ipt_recent fix Amin Azez
2006-02-20 16:12 ` [patch] ipt_recent Amin Azez
2006-03-04 10:00   ` Patrick McHardy
2006-03-07 15:48     ` Amin Azez
2006-03-08 12:16       ` Patrick McHardy
2006-03-13 17:47         ` ipt_recent patch Amin Azez
2006-03-22 14:26           ` Stephen Frost
2006-05-03  9:32             ` Amin Azez
2006-05-03 11:41               ` Amin Azez
2006-03-22 12:04         ` [patch] ipt_recent Amin Azez
2006-02-21 11:16 ` BUG: More ipt_recent queries Amin Azez
2006-02-21 16:28   ` Amin Azez
2006-03-04 10:13   ` Patrick McHardy
2006-03-06  3:04     ` Stephen Frost
2006-03-07 15:46       ` Amin Azez
2006-03-04 10:10 ` ipt_recent fix Patrick McHardy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.