A bunch of minor changes to the code (see below) for readability, code size

and speed. No new functionality added (yet) apart from a bugfix.
MFC will occur in due time and probably in stages.

BUGFIX: fix a problem in old code which prevented reallocation of
the hash table for dynamic rules (there is a PR on this).

OTHER CHANGES: minor changes to the internal struct for static and dynamic rules.
Requires rebuild of ipfw binary.

Add comments to show how data structures are linked together.
(It probably makes no sense to keep the chain pointers separate
from actual rule descriptors. They will be hopefully merged soon.

keep a (sysctl-readable) counter for the number of static rules,
to speed up IP_FW_GET operations

initial support for a "grace time" for expired connections, so we
can set timeouts for closing connections to much shorter times.

merge zero_entry() and resetlog_entry(), they use basically the
same code.

clean up and reduce replication of code for removing rules,
both for readability and code size.

introduce a separate lifetime for dynamic UDP rules.

fix a problem in old code which prevented reallocation of
the hash table for dynamic rules (PR ...)

restructure dynamic rule descriptors

introduce some local variables to avoid multiple dereferencing of
pointer chains (reduces code size and hopefully increases speed).

1 files changed, 318 insertions, 304 deletions

diff --git a/sys/netinet/ip_fw.c b/sys/netinet/ip_fw.c
index e325436..81219da 100644
--- a/sys/netinet/ip_fw.c
+++ b/sys/netinet/ip_fw.c
@@ -156,13 +156,26 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, permanent_rules, CTLFLAG_RW,
 static struct ipfw_dyn_rule **ipfw_dyn_v = NULL ;
 static u_int32_t dyn_buckets = 256 ; /* must be power of 2 */
 static u_int32_t curr_dyn_buckets = 256 ; /* must be power of 2 */
+/**
+ * timeouts for various events in handing dynamic rules.
+ */
 static u_int32_t dyn_ack_lifetime = 300 ;
 static u_int32_t dyn_syn_lifetime = 20 ;
-static u_int32_t dyn_fin_lifetime = 20 ;
-static u_int32_t dyn_rst_lifetime = 5 ;
-static u_int32_t dyn_short_lifetime = 30 ;
-static u_int32_t dyn_count = 0 ;
-static u_int32_t dyn_max = 1000 ;
+static u_int32_t dyn_fin_lifetime = 1 ;
+static u_int32_t dyn_rst_lifetime = 1 ;
+static u_int32_t dyn_udp_lifetime = 10 ;
+static u_int32_t dyn_short_lifetime = 5 ;
+
+/*
+ * after reaching 0, dynamic rules are considered still valid for
+ * an additional grace time, unless there is lack of resources.
+ */
+static u_int32_t dyn_grace_time = 10 ;
+
+static u_int32_t static_count = 0 ;	/* # of static rules */
+static u_int32_t dyn_count = 0 ;	/* # of dynamic rules */
+static u_int32_t dyn_max = 1000 ;	/* max # of dynamic rules */
+
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_buckets, CTLFLAG_RW,
     &dyn_buckets, 0, "Number of dyn. buckets");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, curr_dyn_buckets, CTLFLAG_RD,
@@ -171,6 +184,8 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_count, CTLFLAG_RD,
     &dyn_count, 0, "Number of dyn. rules");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_max, CTLFLAG_RW,
     &dyn_max, 0, "Max number of dyn. rules");
+SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, static_count, CTLFLAG_RD,
+    &static_count, 0, "Number of static rules");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_ack_lifetime, CTLFLAG_RW,
     &dyn_ack_lifetime, 0, "Lifetime of dyn. rules for acks");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_syn_lifetime, CTLFLAG_RW,
@@ -179,8 +194,13 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_fin_lifetime, CTLFLAG_RW,
     &dyn_fin_lifetime, 0, "Lifetime of dyn. rules for fin");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_rst_lifetime, CTLFLAG_RW,
     &dyn_rst_lifetime, 0, "Lifetime of dyn. rules for rst");
+SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_udp_lifetime, CTLFLAG_RW,
+    &dyn_udp_lifetime, 0, "Lifetime of dyn. rules for UDP");
 SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_short_lifetime, CTLFLAG_RW,
     &dyn_short_lifetime, 0, "Lifetime of dyn. rules for other situations");
+SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_grace_time, CTLFLAG_RD,
+    &dyn_grace_time, 0, "Grace time for dyn. rules");
+
 
 #endif
 
@@ -192,8 +212,7 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dyn_short_lifetime, CTLFLAG_RW,
 
 static int	add_entry __P((struct ip_fw_head *chainptr, struct ip_fw *frwl));
 static int	del_entry __P((struct ip_fw_head *chainptr, u_short number));
-static int	zero_entry __P((struct ip_fw *));
-static int	resetlog_entry __P((struct ip_fw *));
+static int	zero_entry __P((struct ip_fw *, int));
 static int	check_ipfw_struct __P((struct ip_fw *m));
 static __inline int
 		iface_match __P((struct ifnet *ifp, union ip_fw_if *ifu,
@@ -654,8 +673,28 @@ hash_packet(struct ipfw_flow_id *id)
     return i ;
 }
 
+/**
+ * unlink a dynamic rule from a chain. prev is a pointer to
+ * the previous one, q is a pointer to the rule to delete,
+ * head is a pointer to the head of the queue.
+ * Modifies q and potentially also head.
+ */
+#define UNLINK_DYN_RULE(prev, head, q) {			\
+    struct ipfw_dyn_rule *old_q = q;				\
+								\
+    DEB(printf("-- unlink 0x%08x %d -> 0x%08x %d, %d left\n",	\
+	(q->id.src_ip), (q->id.src_port),			\
+	(q->id.dst_ip), (q->id.dst_port), dyn_count-1 ); )	\
+    if (prev != NULL)						\
+	prev->next = q = q->next ;				\
+    else							\
+	ipfw_dyn_v[i] = q = q->next ;				\
+    dyn_count-- ;						\
+    free(old_q, M_IPFW); }
+
+
 #define TIME_LEQ(a,b)       ((int)((a)-(b)) <= 0)
-/*
+/**
  * Remove all dynamic rules pointing to a given chain, or all
  * rules if chain == NULL. Second parameter is 1 if we want to
  * delete unconditionally, otherwise only expired rules are removed.
@@ -663,7 +702,7 @@ hash_packet(struct ipfw_flow_id *id)
 static void
 remove_dyn_rule(struct ip_fw_chain *chain, int force)
 {
-    struct ipfw_dyn_rule *prev, *q, *old_q ;
+    struct ipfw_dyn_rule *prev, *q;
     int i ;
     static u_int32_t last_remove = 0 ;
 
@@ -676,19 +715,18 @@ remove_dyn_rule(struct ip_fw_chain *chain, int force)
 
     for (i = 0 ; i < curr_dyn_buckets ; i++) {
 	for (prev=NULL, q = ipfw_dyn_v[i] ; q ; ) {
-	    if ( (chain == NULL || chain == q->chain) &&
-		 (force || TIME_LEQ( q->expire , time_second ) ) ) {
-		DEB(printf("-- remove entry 0x%08x %d -> 0x%08x %d, %d left\n",
-		    (q->id.src_ip), (q->id.src_port),
-		    (q->id.dst_ip), (q->id.dst_port), dyn_count-1 ); )
-		old_q = q ;
-		if (prev != NULL)
-		    prev->next = q = q->next ;
-		else
-		    ipfw_dyn_v[i] = q = q->next ;
-		dyn_count-- ;
-		free(old_q, M_IPFW);
-		continue ;
+	    /*
+	     * logic can become complex here, so we split tests.
+	     * First, test if we match any chain,
+	     * then make sure the rule is expired or we want to kill it,
+	     * and possibly more in the future.
+	     */
+	    int zap = ( chain == NULL || chain == q->chain);
+	    if (zap)
+		zap = force || TIME_LEQ( q->expire , time_second );
+
+	    if (zap) {
+		UNLINK_DYN_RULE(prev, ipfw_dyn_v[i], q);
 	    } else {
 		prev = q ;
 		q = q->next ;
@@ -697,6 +735,14 @@ remove_dyn_rule(struct ip_fw_chain *chain, int force)
     }
 }
 
+#define EXPIRE_DYN_CHAIN(chain) remove_dyn_rule(chain, 0 /* expired ones */)
+#define EXPIRE_DYN_CHAINS() remove_dyn_rule(NULL, 0 /* expired ones */)
+#define DELETE_DYN_CHAIN(chain) remove_dyn_rule(chain, 1 /* force removal */)
+#define DELETE_DYN_CHAINS() remove_dyn_rule(NULL, 1 /* force removal */)
+
+/**
+ * lookup a dynamic rule.
+ */
 static struct ipfw_dyn_rule *
 lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction)
 {
@@ -704,7 +750,7 @@ lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction)
      * stateful ipfw extensions.
      * Lookup into dynamic session queue
      */
-    struct ipfw_dyn_rule *prev, *q, *old_q ;
+    struct ipfw_dyn_rule *prev, *q ;
     int i, dir = 0;
 #define MATCH_FORWARD 1
 
@@ -713,33 +759,23 @@ lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction)
     i = hash_packet( pkt );
     for (prev=NULL, q = ipfw_dyn_v[i] ; q != NULL ; ) {
        if (TIME_LEQ( q->expire , time_second ) ) { /* expire entry */
-           old_q = q ;
-           if (prev != NULL)
-               prev->next = q = q->next ;
-           else
-               ipfw_dyn_v[i] = q = q->next ;
-           dyn_count-- ;
-           free(old_q, M_IPFW);
+	   UNLINK_DYN_RULE(prev, ipfw_dyn_v[i], q);
            continue ;
 	}
 	if ( pkt->proto == q->id.proto) {
-	    switch (q->type) {
-	    default:        /* bidirectional rule, no masks */
-		if (pkt->src_ip == q->id.src_ip &&
-			pkt->dst_ip == q->id.dst_ip &&
-			pkt->src_port == q->id.src_port &&
-			pkt->dst_port == q->id.dst_port ) {
-		    dir = MATCH_FORWARD ;
-		    goto found ;
-		}
-		if (pkt->src_ip == q->id.dst_ip &&
-			pkt->dst_ip == q->id.src_ip &&
-			pkt->src_port == q->id.dst_port &&
-			pkt->dst_port == q->id.src_port ) {
-		   dir = 0 ; /* reverse match */
-		   goto found ;
-		}
-		break ;
+	    if (pkt->src_ip == q->id.src_ip &&
+		    pkt->dst_ip == q->id.dst_ip &&
+		    pkt->src_port == q->id.src_port &&
+		    pkt->dst_port == q->id.dst_port ) {
+		dir = MATCH_FORWARD ;
+		goto found ;
+	    }
+	    if (pkt->src_ip == q->id.dst_ip &&
+		    pkt->dst_ip == q->id.src_ip &&
+		    pkt->src_port == q->id.dst_port &&
+		    pkt->dst_port == q->id.src_port ) {
+	       dir = 0 ; /* reverse match */
+	       goto found ;
 	    }
 	}
 	prev = q ;
@@ -786,8 +822,10 @@ found:
 	    q->expire = time_second + dyn_rst_lifetime ;
 	    break ;
 	}
+    } else if (pkt->proto == IPPROTO_UDP) {
+	q->expire = time_second + dyn_udp_lifetime ;
     } else {
-	/* should do something for UDP and others... */
+	/* other protocols */
 	q->expire = time_second + dyn_short_lifetime ;
     }
     if (match_direction)
@@ -799,44 +837,44 @@ found:
  * Install state for a dynamic session.
  */
 
-static void
-add_dyn_rule(struct ipfw_flow_id *id, struct ipfw_flow_id *mask,
+static struct ipfw_dyn_rule *
+add_dyn_rule(struct ipfw_flow_id *id, u_int8_t dyn_type,
        struct ip_fw_chain *chain)
 {
     struct ipfw_dyn_rule *r ;
 
     int i ;
     if (ipfw_dyn_v == NULL ||
-       (dyn_count == 0 && dyn_buckets != curr_dyn_buckets)) {
-       /* try reallocation, make sure we have a power of 2 */
-       u_int32_t i = dyn_buckets ;
-       while ( i > 0 && (i & 1) == 0 )
-           i >>= 1 ;
-       if (i != 1) /* not a power of 2 */
-           dyn_buckets = curr_dyn_buckets ; /* reset */
-       else {
-           if (ipfw_dyn_v != NULL)
+		(dyn_count == 0 && dyn_buckets != curr_dyn_buckets)) {
+	/* try reallocation, make sure we have a power of 2 */
+	u_int32_t i = dyn_buckets ;
+	while ( i > 0 && (i & 1) == 0 )
+	    i >>= 1 ;
+	if (i != 1) /* not a power of 2 */
+	    dyn_buckets = curr_dyn_buckets ; /* reset */
+	else {
+	    curr_dyn_buckets = dyn_buckets ;
+            if (ipfw_dyn_v != NULL)
 		free(ipfw_dyn_v, M_IPFW);
-           ipfw_dyn_v = malloc(curr_dyn_buckets * sizeof r,
+            ipfw_dyn_v = malloc(curr_dyn_buckets * sizeof r,
                    M_IPFW, M_DONTWAIT | M_ZERO);
-	   if (ipfw_dyn_v == NULL)
-		return ; /* failed ! */
-       }
+	    if (ipfw_dyn_v == NULL)
+		return NULL; /* failed ! */
+	}
     }
     i = hash_packet(id);
 
     r = malloc(sizeof *r, M_IPFW, M_DONTWAIT | M_ZERO);
     if (r == NULL) {
        printf ("sorry cannot allocate state\n");
-       return ;
+       return NULL ;
     }
 
-    if (mask)
-	r->mask = *mask ;
     r->id = *id ;
     r->expire = time_second + dyn_syn_lifetime ;
     r->chain = chain ;
-    r->type = ((struct ip_fw_ext *)chain->rule)->dyn_type ;
+    r->dyn_type = dyn_type ;
+    r->pcnt = r->bcnt = 0 ;
 
     r->bucket = i ;
     r->next = ipfw_dyn_v[i] ;
@@ -846,6 +884,7 @@ add_dyn_rule(struct ipfw_flow_id *id, struct ipfw_flow_id *mask,
        (r->id.src_ip), (r->id.src_port),
        (r->id.dst_ip), (r->id.dst_port),
        dyn_count ); )
+    return r;
 }
 
 /*
@@ -853,14 +892,16 @@ add_dyn_rule(struct ipfw_flow_id *id, struct ipfw_flow_id *mask,
  * There are different types of dynamic rules which can be installed.
  * The type is in chain->dyn_type.
  * Type 0 (default) is a bidirectional rule
+ *
+ * Returns 1 (failure) if state is not installed.
  */
-static void
+static int
 install_state(struct ip_fw_chain *chain)
 {
     struct ipfw_dyn_rule *q ;
     static int last_log ;
 
-    u_long type = ((struct ip_fw_ext *)chain->rule)->dyn_type ;
+    u_int8_t type = ((struct ip_fw_ext *)chain->rule)->dyn_type ;
 
     DEB(printf("-- install state type %d 0x%08lx %u -> 0x%08lx %u\n",
        type,
@@ -868,28 +909,29 @@ install_state(struct ip_fw_chain *chain)
        (last_pkt.dst_ip), (last_pkt.dst_port) );)
 
     q = lookup_dyn_rule(&last_pkt, NULL) ;
-    if (q != NULL) {
-	if (last_log == time_second)
-	    return ;
-	last_log = time_second ;
-       printf(" entry already present, done\n");
-       return ;
+    if (q != NULL) { /* should never occur */
+	if (last_log != time_second) {
+	    last_log = time_second ;
+	    printf(" entry already present, done\n");
+	}
+	return 0 ;
     }
     if (dyn_count >= dyn_max) /* try remove old ones... */
-	remove_dyn_rule(NULL, 0 /* expire */);
+	EXPIRE_DYN_CHAINS();
     if (dyn_count >= dyn_max) {
-	if (last_log == time_second)
-	    return ;
-	last_log = time_second ;
-       printf(" Too many dynamic rules, sorry\n");
-       return ;
+	if (last_log != time_second) {
+	    last_log = time_second ;
+	    printf(" Too many dynamic rules, sorry\n");
+	}
+	return 1; /* cannot install, notify caller */
     }
     switch (type) {
     default: /* bidir rule */
-       add_dyn_rule(&last_pkt, NULL, chain);
+       add_dyn_rule(&last_pkt, DYN_KEEP_STATE, chain);
        break ;
     }
-    q = lookup_dyn_rule(&last_pkt, NULL) ; /* XXX this just sets the lifetime ... */
+    lookup_dyn_rule(&last_pkt, NULL) ; /* XXX just set the lifetime */
+    return 0;
 }
 
 /*
@@ -1001,23 +1043,26 @@ ip_fw_chk(struct ip **pip, int hlen,
 	    ip_len = ip->ip_len;
 	}
 	if (offset == 0) {
-	    struct tcphdr *tcp;
-	    struct udphdr *udp;
-
 	    switch (proto) {
-	    case IPPROTO_TCP :
+	    case IPPROTO_TCP : {
+		struct tcphdr *tcp;
+
 		PULLUP_TO(hlen + sizeof(struct tcphdr));
 		tcp =(struct tcphdr *)((u_int32_t *)ip + ip->ip_hl);
 		dst_port = tcp->th_dport ;
 		src_port = tcp->th_sport ;
 		flags = tcp->th_flags ;
+		}
 		break ;
 
-	    case IPPROTO_UDP :
+	    case IPPROTO_UDP : {
+		struct udphdr *udp;
+
 		PULLUP_TO(hlen + sizeof(struct udphdr));
 		udp =(struct udphdr *)((u_int32_t *)ip + ip->ip_hl);
 		dst_port = udp->uh_dport ;
 		src_port = udp->uh_sport ;
+		}
 		break;
 
 	    case IPPROTO_ICMP:
@@ -1343,8 +1388,10 @@ got_match:
 		 * If not a dynamic match (q == NULL) and keep-state, install
 		 * a new dynamic entry.
 		 */
-		if (q == NULL && f->fw_flg & IP_FW_F_KEEP_S)
-		    install_state(chain);
+		if (q == NULL && f->fw_flg & IP_FW_F_KEEP_S) {
+		    if (install_state(chain)) /* error or limit violation */
+			goto dropit;
+		}
 		/* Update statistics */
 		f->fw_pcnt += 1;
 		f->fw_bcnt += ip_len;
@@ -1369,11 +1416,10 @@ got_match:
 			return(f->fw_divert_port | IP_FW_PORT_TEE_FLAG);
 #endif
 		case IP_FW_F_SKIPTO: /* XXX check */
-			if ( f->next_rule_ptr )
-			    chain = f->next_rule_ptr ;
-			else
+			if ( (chain = f->next_rule_ptr) == NULL )
 			    chain = lookup_next_rule(chain) ;
-			if (! chain) goto dropit;
+			if (! chain)
+			    goto dropit;
 			goto again ;
 #ifdef DUMMYNET
 		case IP_FW_F_PIPE:
@@ -1418,7 +1464,7 @@ got_match:
 	 * - The packet is not a multicast or broadcast packet
 	 */
 	if ((rule->fw_flg & IP_FW_F_COMMAND) == IP_FW_F_REJECT
-	    && (ip->ip_p != IPPROTO_ICMP || is_icmp_query(ip))
+	    && (proto != IPPROTO_ICMP || is_icmp_query(ip))
 	    && !((*m)->m_flags & (M_BCAST|M_MCAST))
 	    && !IN_MULTICAST(ntohl(ip->ip_dst.s_addr))) {
 		switch (rule->fw_reject_code) {
@@ -1483,65 +1529,66 @@ flush_rule_ptrs()
 }
 
 static int
-add_entry(struct ip_fw_head *chainptr, struct ip_fw *frwl)
+add_entry(struct ip_fw_head *head, struct ip_fw *rule)
 {
 	struct ip_fw *ftmp = 0;
 	struct ip_fw_ext *ftmp_ext = 0 ;
-	struct ip_fw_chain *fwc = 0, *fcp, *fcpl = 0;
+	struct ip_fw_chain *fwc, *fcp, *fcpl;
 	u_short nbr = 0;
 	int s;
 
 	fwc = malloc(sizeof *fwc, M_IPFW, M_DONTWAIT);
+	if (!fwc)
+		return (ENOSPC);
 	ftmp_ext = malloc(sizeof *ftmp_ext, M_IPFW, M_DONTWAIT | M_ZERO);
-	ftmp = &ftmp_ext->rule ;
-	if (!fwc || !ftmp) {
-		dprintf(("%s malloc said no\n", err_prefix));
-		if (fwc)  free(fwc, M_IPFW);
-		if (ftmp) free(ftmp, M_IPFW);
+	if (!ftmp_ext) {
+		free(fwc, M_IPFW);
 		return (ENOSPC);
 	}
+	fwc->rule = ftmp = &ftmp_ext->rule ;
 
-	bcopy(frwl, ftmp, sizeof(*ftmp));
+	bcopy(rule, ftmp, sizeof(*ftmp));
 	if (ftmp->fw_flg & IP_FW_F_RND_MATCH)
 		ftmp_ext->dont_match_prob = (intptr_t)ftmp->pipe_ptr;
-	if (ftmp->fw_flg & IP_FW_F_KEEP_S)
-		ftmp_ext->dyn_type = (u_long)(ftmp->next_rule_ptr) ;
+	if (ftmp->fw_flg & IP_FW_F_KEEP_S) {
+		u_long type = (u_long)(ftmp->next_rule_ptr) ;
+		ftmp_ext->dyn_type = type & 0xff;
+	}
 
 	ftmp->fw_in_if.fu_via_if.name[FW_IFNLEN - 1] = '\0';
 	ftmp->fw_pcnt = 0L;
 	ftmp->fw_bcnt = 0L;
 	ftmp->next_rule_ptr = NULL ;
 	ftmp->pipe_ptr = NULL ;
-	fwc->rule = ftmp;
 	
 	s = splnet();
 
-	if (LIST_FIRST(chainptr) == 0) {
-		LIST_INSERT_HEAD(chainptr, fwc, next);
-		splx(s);
-		return(0);
+	if (LIST_FIRST(head) == 0) {
+		LIST_INSERT_HEAD(head, fwc, next);
+		goto done;
         }
 
 	/* If entry number is 0, find highest numbered rule and add 100 */
 	if (ftmp->fw_number == 0) {
-		LIST_FOREACH(fcp, chainptr, next) {
-			if (fcp->rule->fw_number != (u_short)-1)
+		LIST_FOREACH(fcp, head, next) {
+			if (fcp->rule->fw_number != IPFW_DEFAULT_RULE)
 				nbr = fcp->rule->fw_number;
 			else
 				break;
 		}
 		if (nbr < IPFW_DEFAULT_RULE - 100)
 			nbr += 100;
-		ftmp->fw_number = frwl->fw_number = nbr;
+		ftmp->fw_number = rule->fw_number = nbr;
 	}
 
 	/* Got a valid number; now insert it, keeping the list ordered */
-	LIST_FOREACH(fcp, chainptr, next) {
+	fcpl = NULL ;
+	LIST_FOREACH(fcp, head, next) {
 		if (fcp->rule->fw_number > ftmp->fw_number) {
 			if (fcpl) {
 				LIST_INSERT_AFTER(fcpl, fwc, next);
 			} else {
-				LIST_INSERT_HEAD(chainptr, fwc, next);
+				LIST_INSERT_HEAD(head, fwc, next);
 			}
 			break;
 		} else {
@@ -1549,150 +1596,125 @@ add_entry(struct ip_fw_head *chainptr, struct ip_fw *frwl)
 		}
 	}
 	flush_rule_ptrs();
-
+done:
+	static_count++;
 	splx(s);
+	DEB(printf("++ installed rule %d, static count now %d\n",
+		ftmp->fw_number, static_count);)
 	return (0);
 }
 
-static int
-del_entry(struct ip_fw_head *chainptr, u_short number)
+/**
+ * free storage associated with a static chain entry (including
+ * dependent dynamic rules), and zeroes rule pointers to avoid
+ * dangling pointer dereferences.
+ * @return a pointer to the next entry.
+ * Must be called at splnet() and with a non-null argument.
+ */
+static struct ip_fw_chain *
+free_chain(struct ip_fw_chain *fcp)
 {
-	struct ip_fw_chain *fcp;
-
-	fcp = LIST_FIRST(chainptr);
-	if (number != (u_short)-1) {
-		for (; fcp; fcp = LIST_NEXT(fcp, next)) {
-			if (fcp->rule->fw_number == number) {
-				int s;
+    struct ip_fw_chain *n;
 
-				/* prevent access to rules while removing them */
-				s = splnet();
-				while (fcp && fcp->rule->fw_number == number) {
-					struct ip_fw_chain *next;
-
-					remove_dyn_rule(fcp, 1 /* delete */);
-					next = LIST_NEXT(fcp, next);
-					LIST_REMOVE(fcp, next);
+    n = LIST_NEXT(fcp, next);
+    DELETE_DYN_CHAIN(fcp);
+    LIST_REMOVE(fcp, next);
+    static_count--;
 #ifdef DUMMYNET
-					dn_rule_delete(fcp) ;
+    dn_rule_delete(fcp) ;
 #endif
-					flush_rule_ptrs();
-					free(fcp->rule, M_IPFW);
-					free(fcp, M_IPFW);
-					fcp = next;
-				}
-				splx(s);
-				return 0;
-			}
-		}
-	}
-
-	return (EINVAL);
+    flush_rule_ptrs(); /* more efficient to do outside the loop */
+    free(fcp->rule, M_IPFW);
+    free(fcp, M_IPFW);
+    return n;
 }
 
+/**
+ * remove all rules with given number.
+ */
 static int
-zero_entry(struct ip_fw *frwl)
+del_entry(struct ip_fw_head *chainptr, u_short number)
 {
-	struct ip_fw_chain *fcp;
-	int s, cleared;
+    struct ip_fw_chain *fcp;
 
-	if (frwl == 0) {
-		s = splnet();
-		LIST_FOREACH(fcp, &ip_fw_chain_head, next) {
-			fcp->rule->fw_bcnt = fcp->rule->fw_pcnt = 0;
-			fcp->rule->fw_loghighest = fcp->rule->fw_logamount;
-			fcp->rule->timestamp = 0;
-		}
-		splx(s);
-	}
-	else {
-		cleared = 0;
-
-		/*
-		 *	It's possible to insert multiple chain entries with the
-		 *	same number, so we don't stop after finding the first
-		 *	match if zeroing a specific entry.
-		 */
-		LIST_FOREACH(fcp, &ip_fw_chain_head, next)
-			if (frwl->fw_number == fcp->rule->fw_number) {
-				s = splnet();
-				while (fcp && frwl->fw_number == fcp->rule->fw_number) {
-					fcp->rule->fw_bcnt = fcp->rule->fw_pcnt = 0;
-					fcp->rule->fw_loghighest =
-					    fcp->rule->fw_logamount;
-					fcp->rule->timestamp = 0;
-					fcp = LIST_NEXT(fcp, next);
-				}
-				splx(s);
-				cleared = 1;
-				break;
-			}
-		if (!cleared)	/* we didn't find any matching rules */
-			return (EINVAL);
-	}
+    if (number != IPFW_DEFAULT_RULE) {
+	LIST_FOREACH(fcp, chainptr, next) {
+	    if (fcp->rule->fw_number == number) {
+		int s ;
 
-	if (fw_verbose) {
-		if (frwl)
-			log(LOG_SECURITY | LOG_NOTICE,
-			    "ipfw: Entry %d cleared.\n", frwl->fw_number);
-		else
-			log(LOG_SECURITY | LOG_NOTICE,
-			    "ipfw: Accounting cleared.\n");
+		s = splnet(); /* prevent access to rules while removing */
+		while (fcp && fcp->rule->fw_number == number)
+		    fcp = free_chain(fcp);
+		/* XXX could move flush_rule_ptrs() here */
+		splx(s);
+		return 0 ;
+	    }
 	}
-
-	return (0);
+    }
+    return (EINVAL);
 }
 
+/**
+ * Reset some or all counters on firewall rules.
+ * @arg frwl is null to clear all entries, or contains a specific
+ * rule number.
+ * @arg log_only is 1 if we only want to reset logs, zero otherwise.
+ */
+
 static int
-resetlog_entry(struct ip_fw *frwl)
+zero_entry(struct ip_fw *frwl, int log_only)
 {
-	struct ip_fw_chain *fcp;
-	int s, cleared;
+    struct ip_fw_chain *fcp;
+    struct ip_fw *rule;
+    int s;
+    u_short number = 0 ;
+    char *msg ;
 
-	if (frwl == 0) {
-		s = splnet();
-		counter = 0;
-		LIST_FOREACH(fcp, &ip_fw_chain_head, next)
-			fcp->rule->fw_loghighest = fcp->rule->fw_pcnt +
-			    fcp->rule->fw_logamount;
-		splx(s);
-	}
-	else {
-		cleared = 0;
-
-		/*
-		 *	It's possible to insert multiple chain entries with the
-		 *	same number, so we don't stop after finding the first
-		 *	match if zeroing a specific entry.
-		 */
-		LIST_FOREACH(fcp, &ip_fw_chain_head, next)
-			if (frwl->fw_number == fcp->rule->fw_number) {
-				s = splnet();
-				while (fcp && frwl->fw_number == fcp->rule->fw_number) {
-					fcp->rule->fw_loghighest =
-					    fcp->rule->fw_pcnt +
-					    fcp->rule->fw_logamount;
-					fcp = LIST_NEXT(fcp, next);
-				}
-				splx(s);
-				cleared = 1;
-				break;
-			}
-		if (!cleared)	/* we didn't find any matching rules */
-			return (EINVAL);
-	}
-
-	if (fw_verbose) {
-		if (frwl)
-			log(LOG_SECURITY | LOG_NOTICE,
-			    "ipfw: Entry %d logging count reset.\n",
-			    frwl->fw_number);
-		else
-			log(LOG_SECURITY | LOG_NOTICE, "
-			    ipfw: All logging counts cleared.\n");
+    if (frwl == 0) {
+	s = splnet();
+	LIST_FOREACH(fcp, &ip_fw_chain_head, next) {
+	    rule = fcp->rule;
+	    if (log_only == 0) {
+		rule->fw_bcnt = rule->fw_pcnt = 0;
+		rule->timestamp = 0;
+	    }
+	    rule->fw_loghighest = rule->fw_pcnt+rule->fw_logamount;
 	}
+	splx(s);
+	msg = log_only ? "ipfw: All logging counts cleared.\n" :
+		"ipfw: Accounting cleared.\n";
+    } else {
+	int cleared = 0;
+	number = frwl->fw_number ;
 
-	return (0);
+	/*
+	 * It's possible to insert multiple chain entries with the
+	 * same number, so we don't stop after finding the first
+	 * match if zeroing a specific entry.
+	 */
+	LIST_FOREACH(fcp, &ip_fw_chain_head, next)
+	    if (number == fcp->rule->fw_number) {
+		s = splnet();
+		while (fcp && number == (rule=fcp->rule)->fw_number) {
+		    if (log_only == 0) {
+			rule->fw_bcnt = rule->fw_pcnt = 0;
+			rule->timestamp = 0;
+		    }
+		    rule->fw_loghighest = rule->fw_pcnt+ rule->fw_logamount;
+		    fcp = LIST_NEXT(fcp, next);
+		}
+		splx(s);
+		cleared = 1;
+		break;
+	    }
+	if (!cleared)	/* we didn't find any matching rules */
+	    return (EINVAL);
+	msg = log_only ? "Entry %d logging count reset.\n" :
+		"ipfw: Entry %d cleared.\n";
+    }
+    if (fw_verbose)
+	log(LOG_SECURITY | LOG_NOTICE, msg, number);
+    return (0);
 }
 
 static int
@@ -1791,8 +1813,7 @@ check_ipfw_struct(struct ip_fw *frwl)
 	}
 
 	/* Check command specific stuff */
-	switch (frwl->fw_flg & IP_FW_F_COMMAND)
-	{
+	switch (frwl->fw_flg & IP_FW_F_COMMAND) {
 	case IP_FW_F_REJECT:
 		if (frwl->fw_reject_code >= 0x100
 		    && !(frwl->fw_prot == IPPROTO_TCP
@@ -1807,11 +1828,11 @@ check_ipfw_struct(struct ip_fw *frwl)
 	case IP_FW_F_TEE:
 #endif
 #ifdef DUMMYNET
-	case IP_FW_F_PIPE:              /* piping through 0 is invalid */
-	case IP_FW_F_QUEUE:             /* piping through 0 is invalid */
+	case IP_FW_F_PIPE:              /* pipe 0 is invalid */
+	case IP_FW_F_QUEUE:             /* queue 0 is invalid */
 #endif
 		if (frwl->fw_divert_port == 0) {
-			dprintf(("%s can't divert to port 0\n", err_prefix));
+			dprintf(("%s 0 is an invalid argument\n", err_prefix));
 			return (EINVAL);
 		}
 		break;
@@ -1851,32 +1872,33 @@ ip_fw_ctl(struct sockopt *sopt)
 
 	switch (sopt->sopt_name) {
 	case IP_FW_GET:
-		size = 0 ;
 		s = splnet();
-		LIST_FOREACH(fcp, &ip_fw_chain_head, next)
-		    size += sizeof(struct ip_fw) ;
-		if (ipfw_dyn_v) {
-		    int i ;
-		    struct ipfw_dyn_rule *p ;
+		/* size of static rules */
+		size = static_count * sizeof(struct ip_fw) ;
+		if (ipfw_dyn_v)		/* add size of dyn.rules */
+		    size += (dyn_count * sizeof(struct ipfw_dyn_rule));
 
-		    for (i = 0 ; i < curr_dyn_buckets ; i++ )
-			for ( p = ipfw_dyn_v[i] ; p != NULL ; p = p->next )
-			    size += sizeof(*p) ;
-		}
+		/*
+		 * XXX todo: if the user passes a short length to know how
+		 * much room is needed, do not
+		 * bother filling up the buffer, just jump to the
+		 * sooptcopyout.
+		 */
 		buf = malloc(size, M_TEMP, M_WAITOK);
 		if (buf == 0) {
+			splx(s);
 			error = ENOBUFS;
 			break;
 		}
 
 		bp = buf ;
 		LIST_FOREACH(fcp, &ip_fw_chain_head, next) {
-			bcopy(fcp->rule, bp, sizeof *fcp->rule);
-			bp->pipe_ptr = (void *)(intptr_t)
-			    ((struct ip_fw_ext *)fcp->rule)->dont_match_prob;
-			bp->next_rule_ptr = (void *)(intptr_t)
-			    ((struct ip_fw_ext *)fcp->rule)->dyn_type;
-			bp++;
+		    struct ip_fw_ext *e = (struct ip_fw_ext *)fcp->rule;
+
+		    bcopy(e, bp, sizeof *fcp->rule);
+		    bp->pipe_ptr = (void *)(intptr_t) e->dont_match_prob;
+		    bp->next_rule_ptr = (void *)(intptr_t) (e->dyn_type);
+		    bp++;
 		}
 		if (ipfw_dyn_v) {
 		    int i ;
@@ -1904,37 +1926,24 @@ ip_fw_ctl(struct sockopt *sopt)
 		break;
 
 	case IP_FW_FLUSH:
+		/*
+		 * Normally we cannot release the lock on each iteration.
+		 * We could do it here only because we start from the head all
+		 * the times so there is no risk of missing some entries.
+		 * On the other hand, the risk is that we end up with
+		 * a very inconsistent ruleset, so better keep the lock
+		 * around the whole cycle.
+		 * 
+		 * XXX this code can be improved by resetting the head of
+		 * the list to point to the default rule, and then freeing
+		 * the old list without the need for a lock.
+		 */
+
 		s = splnet();
-		remove_dyn_rule(NULL, 1 /* force delete */);
+		while ( (fcp = LIST_FIRST(&ip_fw_chain_head)) &&
+			fcp->rule->fw_number != IPFW_DEFAULT_RULE )
+		    free_chain(fcp);
 		splx(s);
-		fcp = LIST_FIRST(&ip_fw_chain_head);
-		while (fcp) {
-			struct ip_fw_chain *next;
-			next = LIST_NEXT(fcp, next);
-			if (fcp->rule->fw_number > fw_permanent_rules &&
-			     fcp->rule->fw_number != IPFW_DEFAULT_RULE ) {
-				s = splnet();
-				LIST_REMOVE(fcp, next);
-#ifdef DUMMYNET
-				dn_rule_delete(fcp);
-#endif
-				FREE(fcp->rule, M_IPFW);
-				FREE(fcp, M_IPFW);
-				splx(s);
-			}
-			fcp = next;
-		}
-		break;
-
-	case IP_FW_ZERO:
-		if (sopt->sopt_val != 0) {
-			error = sooptcopyin(sopt, &frwl, sizeof frwl,
-					    sizeof frwl);
-			if (error || (error = zero_entry(&frwl)))
-				break;
-		} else {
-			error = zero_entry(0);
-		}
 		break;
 
 	case IP_FW_ADD:
@@ -1967,16 +1976,21 @@ ip_fw_ctl(struct sockopt *sopt)
 		}
 		break;
 
+	case IP_FW_ZERO:
 	case IP_FW_RESETLOG:
+	    {
+		int cmd = (sopt->sopt_name == IP_FW_RESETLOG );
+		void *arg = NULL ;
+
 		if (sopt->sopt_val != 0) {
-			error = sooptcopyin(sopt, &frwl, sizeof frwl,
-					    sizeof frwl);
-			if (error || (error = resetlog_entry(&frwl)))
-				break;
-		} else {
-			error = resetlog_entry(0);
+		    error = sooptcopyin(sopt, &frwl, sizeof frwl, sizeof frwl);
+		    if (error)
+			break ;
+		    arg = &frwl ;
 		}
-		break;
+		error = zero_entry(arg, cmd);
+	    }
+	    break;
 
 	default:
 		printf("ip_fw_ctl invalid option %d\n", sopt->sopt_name);
@@ -1986,6 +2000,14 @@ ip_fw_ctl(struct sockopt *sopt)
 	return (error);
 }
 
+/**
+ * dummynet needs a reference to the default rule, because rules can
+ * be deleted while packets hold a reference to them (e.g. to resume
+ * processing at the next rule). When this happens, dummynet changes
+ * the reference to the default rule (probably it could well be a
+ * NULL pointer, but this way we do not need to check for the special
+ * case, plus here he have info on the default behaviour.
+ */
 struct ip_fw_chain *ip_fw_default_rule ;
 
 void
@@ -2061,16 +2083,8 @@ ipfw_modevent(module_t mod, int type, void *unused)
 		s = splnet();
 		ip_fw_chk_ptr =  old_chk_ptr;
 		ip_fw_ctl_ptr =  old_ctl_ptr;
-		remove_dyn_rule(NULL, 1 /* force delete */);
-		while ( (fcp = LIST_FIRST(&ip_fw_chain_head)) != NULL) {
-			LIST_REMOVE(fcp, next);
-#ifdef DUMMYNET
-			dn_rule_delete(fcp);
-#endif
-			free(fcp->rule, M_IPFW);
-			free(fcp, M_IPFW);
-		}
-	
+		while ( (fcp = LIST_FIRST(&ip_fw_chain_head)) != NULL)
+		    free_chain(fcp);
 		splx(s);
 		printf("IP firewall unloaded\n");
 		return 0;