Bug 32600

Summary:

[PATCH] incorrect handling of parent rules in ipfw

Product:

Base System

Reporter:

Dan Pelleg <peldan>

Component:

kern

Assignee:

Luigi Rizzo <luigi>

Status:

Closed FIXED

Severity:

Affects Only Me

Priority:

Normal

Version:

4.4-STABLE

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
file.diff	none

Description Dan Pelleg 2001-12-08 12:50:00 UTC

when I started to use limit rules, I noticed ipfw was emitting lots 
of messages:
OUCH! cannot remove rule, count 2

Inspecting ip_fw.c, this is caused when a parent rule with a nonzero count
is detected to have expired.

Here is one scenario how this can legally happen:

 For example, lookup_dyn_parent() increases the expiry by
dyn_short_lifetime, whereas add_dyn_rule() will create it with time_second +
dyn_syn_lifetime.

So, when a second limit rule is created, the parent's expire field is not
extended by enough time to match this second child.

 Luigi Rizzo confirmed this analysis and on his advice I patched to ignore
the expire field altogether for limit rules.




Note that the (userland) ipfw still takes the expire field into account, as
exemplified by the following scenario:

Suppose you have a "limit" rule, and a rule is created for it, and a minute
later 4 more rules are created for it. So the count is 5. Now terminate the
first connection. Suppose you have net.inet.ip.fw.*lifetime set to huge
values like I do. After a while, the LIMIT rule will expire, and "ipfw -d
show" will not show it anymore. However, only when it gets freed will the
count value for its parent be decreased. So, in the meantime, when you do a
"ipfw -d" show, you see "PARENT 5", but only 4 rules that match it.

Another thing that can happen is for the PARENT rule to expire, and then
you see LIMIT rules listed but not their parent.

At that point I added a patch to ipfw to list PARENT rules with count > 0
regardless of their expire value.

(this still doesn't solve the problem of the number of counts for the
parent being larger than the number of children ipfw lists; I started
solving this one by having ipfw update the reference counter in the parent,
and then realized that ip_fw.c doesn't pass parent pointers - at least not
usable ones - the patch includes a comment about this).





Now, running this patched code gave me kernel panics in add_dyn_rule(),
called from install_state(). I came up with this explanation for them:

Suppose you have plenty of rules (ie, conn_limit of them) for a
parent, but they are all expired. This happens to me when I use Mozilla,
which opens dozens of connections, and then leave that window alone for a
while.

In the DYN_LIMIT case of install_state(), the lookup_dyn_parent finds the
parent, which is found out to have too large a count. Then EXPIRE_DYN_CHAIN
is called, the count goes down to zero in the first pass, and the parent is
removed in the second. But install_state is still holding a pointer to the
freed structure, and later passes it on to add_dyn_rule().

I fixed this by re-looking the parent up after the expiry code, which has
solved the problem.

Fix: apply provided patch.
How-To-Repeat:  see above.

Comment 1 Sheldon Hearn freebsd_committer

2001-12-10 11:22:34 UTC

Responsible Changed
From-To: freebsd-bugs->luigi

The attached patch was suggested by Mr Rizzo.

Comment 2 Dan Pelleg 2002-02-17 23:40:20 UTC

Following up to my own PR, I'm providing a patch that applies cleanly
against 4.5-R. It doesn't differ from the previous one in any significant
way (well, except for the fact that it applies cleanly).

--- sys/netinet/ip_fw.c.orig	Mon Feb 11 06:39:33 2002
+++ sys/netinet/ip_fw.c	Mon Feb 11 06:41:21 2002
@@ -694,17 +694,23 @@
 	     * and possibly more in the future.
 	     */
 	    int zap = ( rule == NULL || rule == q->rule);
-	    if (zap)
-		zap = force || TIME_LEQ( q->expire , time_second );
+
 	    /* do not zap parent in first pass, record we need a second pass */
 	    if (q->dyn_type == DYN_LIMIT_PARENT) {
 		max_pass = 1; /* we need a second pass */
 		if (zap == 1 && (pass == 0 || q->count != 0) ) {
 		    zap = 0 ;
-		    if (pass == 1) /* should not happen */
-			printf("OUCH! cannot remove rule, count %d\n",
-				q->count);
+			if (force && pass == 1) { /* should not happen */
+				printf("OUCH! cannot remove rule 0x%08x %d -> 0x%08x %d, count %d, bucket %d\n",
+				 (q->id.src_ip), (q->id.src_port),
+				 (q->id.dst_ip), (q->id.dst_port),
+				 q->count,
+				 i);
+			}
 		}
+	    } else {
+		if (zap)
+			zap = force || TIME_LEQ( q->expire , time_second );
 	    }
 	    if (zap) {
 		UNLINK_DYN_RULE(prev, ipfw_dyn_v[i], q);
@@ -885,7 +891,7 @@
     DEB(printf("-- add entry 0x%08x %d -> 0x%08x %d, total %d\n",
        (r->id.src_ip), (r->id.src_port),
        (r->id.dst_ip), (r->id.dst_port),
-       dyn_count ); )
+               dyn_count); )
     return r;
 }
 
@@ -988,8 +994,20 @@
 	}
 	if (parent->count >= conn_limit) {
 	    EXPIRE_DYN_CHAIN(rule); /* try to expire some */
+         /*
+           The expiry might have removed the parent too.
+           We lookup again, which will re-create if necessary.
+         */
+ 	    parent = lookup_dyn_parent(&id, rule);
+ 	    if (parent == NULL) {
+           printf("add parent failed\n");
+           return 1;
+        }
 	    if (parent->count >= conn_limit) {
-		printf("drop session, too many entries\n");
+          if (last_log != time_second) {
+            last_log = time_second ;
+            printf("drop session, too many entries\n");
+          }
 		return 1;
 	    }
 	}
@@ -1928,6 +1946,12 @@
 			for ( p = ipfw_dyn_v[i] ; p != NULL ; p = p->next, dst++ ) {
 			    bcopy(p, dst, sizeof *p);
                             (int)dst->rule = p->rule->fw_number ;
+			    /*
+			     * we should really set the parent field
+			     * to the corresponding parent in the new
+			     * structure. For now, just set it to NULL.
+			     */
+			    dst->parent = NULL ;
 			    /*
 			     * store a non-null value in "next". The userland
 			     * code will interpret a NULL here as a marker
--- sbin/ipfw/ipfw.c.orig	Mon Feb 11 06:39:56 2002
+++ sbin/ipfw/ipfw.c	Mon Feb 11 06:42:58 2002
@@ -813,10 +813,18 @@
 		    (struct ipfw_dyn_rule *)&rules[num];
 		struct in_addr a;
 		struct protoent *pe;
+		int skip;
 
             printf("## Dynamic rules:\n");
             for (;; d++) {
-		if (d->expire == 0 && !do_expired) {
+			/* determine whether to skip this rule */
+			skip = !do_expired;
+			if( d->dyn_type == DYN_LIMIT_PARENT) {
+				skip = skip && d->count == 0;
+			} else {
+				skip = skip && d->expire == 0;                
+			}
+			if(skip) {
 			if (d->next == NULL)
 				break;
 			continue;

Comment 3 Luigi Rizzo freebsd_committer

2002-09-03 02:52:55 UTC

State Changed
From-To: open->closed

this is fixed in ipfw2.