Add locking.

o change timeout to MPSAFE callout
o restructure rule deletion to deal with locking requirements
o replace static buffer used for ipfw control operations with malloc'd storage

Sponsored by:	FreeBSD Foundation

1 files changed, 309 insertions, 164 deletions

diff --git a/sys/netinet/ip_fw2.c b/sys/netinet/ip_fw2.c
index b26d3a6..cf314be 100644
--- a/sys/netinet/ip_fw2.c
+++ b/sys/netinet/ip_fw2.c
@@ -102,13 +102,25 @@ static u_int32_t set_disable;
 static int fw_verbose;
 static int verbose_limit;
 
-static struct callout_handle ipfw_timeout_h;
+static struct callout ipfw_timeout;
 #define	IPFW_DEFAULT_RULE	65535
 
+struct ip_fw_chain {
+	struct ip_fw	*rules;		/* list of rules */
+	struct ip_fw	*reap;		/* list of rules to reap */
+	struct mtx	mtx;		/* lock guarding rule list */
+};
+#define	IPFW_LOCK_INIT(_chain) \
+	mtx_init(&(_chain)->mtx, "IPFW static rules", NULL, MTX_DEF)
+#define	IPFW_LOCK_DESTROY(_chain)	mtx_destroy(&(_chain)->mtx)
+#define	IPFW_LOCK(_chain)	mtx_lock(&(_chain)->mtx)
+#define	IPFW_UNLOCK(_chain)	mtx_unlock(&(_chain)->mtx)
+#define	IPFW_LOCK_ASSERT(_chain)	mtx_assert(&(_chain)->mtx, MA_OWNED)
+
 /*
  * list of rules for layer 3
  */
-static struct ip_fw *layer3_chain;
+static struct ip_fw_chain layer3_chain;
 
 MALLOC_DEFINE(M_IPFW, "IpFw/IpAcct", "IpFw/IpAcct chain's");
 
@@ -174,6 +186,14 @@ static 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 */
 
+static struct mtx ipfw_dyn_mtx;		/* mutex guarding dynamic rules */
+#define	IPFW_DYN_LOCK_INIT() \
+	mtx_init(&ipfw_dyn_mtx, "IPFW dynamic rules", NULL, MTX_DEF)
+#define	IPFW_DYN_LOCK_DESTROY()	mtx_destroy(&ipfw_dyn_mtx)
+#define	IPFW_DYN_LOCK()		mtx_lock(&ipfw_dyn_mtx)
+#define	IPFW_DYN_UNLOCK()	mtx_unlock(&ipfw_dyn_mtx)
+#define	IPFW_DYN_LOCK_ASSERT()	mtx_assert(&ipfw_dyn_mtx, MA_OWNED)
+
 /*
  * Timeouts for various events in handing dynamic rules.
  */
@@ -395,6 +415,7 @@ iface_match(struct ifnet *ifp, ipfw_insn_if *cmd)
 	} else {
 		struct ifaddr *ia;
 
+		/* XXX lock? */
 		TAILQ_FOREACH(ia, &ifp->if_addrhead, ifa_link) {
 			if (ia->ifa_addr == NULL)
 				continue;
@@ -704,6 +725,8 @@ remove_dyn_rule(struct ip_fw *rule, ipfw_dyn_rule *keep_me)
 	ipfw_dyn_rule *prev, *q;
 	int i, pass = 0, max_pass = 0;
 
+	IPFW_DYN_LOCK_ASSERT();
+
 	if (ipfw_dyn_v == NULL || dyn_count == 0)
 		return;
 	/* do not expire more than once per second, it is useless */
@@ -760,7 +783,7 @@ next:
  * lookup a dynamic rule.
  */
 static ipfw_dyn_rule *
-lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction,
+lookup_dyn_rule_locked(struct ipfw_flow_id *pkt, int *match_direction,
 	struct tcphdr *tcp)
 {
 	/*
@@ -774,6 +797,8 @@ lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction,
 	int i, dir = MATCH_NONE;
 	ipfw_dyn_rule *prev, *q=NULL;
 
+	IPFW_DYN_LOCK_ASSERT();
+
 	if (ipfw_dyn_v == NULL)
 		goto done;	/* not found */
 	i = hash_packet( pkt );
@@ -878,9 +903,25 @@ done:
 	return q;
 }
 
+static ipfw_dyn_rule *
+lookup_dyn_rule(struct ipfw_flow_id *pkt, int *match_direction,
+	struct tcphdr *tcp)
+{
+	ipfw_dyn_rule *q;
+
+	IPFW_DYN_LOCK();
+	q = lookup_dyn_rule_locked(pkt, match_direction, tcp);
+	if (q == NULL)
+		IPFW_DYN_UNLOCK();
+	/* NB: return table locked when q is not NULL */
+	return q;
+}
+
 static void
 realloc_dynamic_table(void)
 {
+	IPFW_DYN_LOCK_ASSERT();
+
 	/*
 	 * Try reallocation, make sure we have a power of 2 and do
 	 * not allow more than 64k entries. In case of overflow,
@@ -921,6 +962,8 @@ add_dyn_rule(struct ipfw_flow_id *id, u_int8_t dyn_type, struct ip_fw *rule)
 	ipfw_dyn_rule *r;
 	int i;
 
+	IPFW_DYN_LOCK_ASSERT();
+
 	if (ipfw_dyn_v == NULL ||
 	    (dyn_count == 0 && dyn_buckets != curr_dyn_buckets)) {
 		realloc_dynamic_table();
@@ -974,6 +1017,8 @@ lookup_dyn_parent(struct ipfw_flow_id *pkt, struct ip_fw *rule)
 	ipfw_dyn_rule *q;
 	int i;
 
+	IPFW_DYN_LOCK_ASSERT();
+
 	if (ipfw_dyn_v) {
 		i = hash_packet( pkt );
 		for (q = ipfw_dyn_v[i] ; q != NULL ; q=q->next)
@@ -1011,13 +1056,16 @@ install_state(struct ip_fw *rule, ipfw_insn_limit *cmd,
 	    (args->f_id.src_ip), (args->f_id.src_port),
 	    (args->f_id.dst_ip), (args->f_id.dst_port) );)
 
-	q = lookup_dyn_rule(&args->f_id, NULL, NULL);
+	IPFW_DYN_LOCK();
+
+	q = lookup_dyn_rule_locked(&args->f_id, NULL, NULL);
 
 	if (q != NULL) { /* should never occur */
 		if (last_log != time_second) {
 			last_log = time_second;
 			printf("ipfw: install_state: entry already present, done\n");
 		}
+		IPFW_DYN_UNLOCK();
 		return 0;
 	}
 
@@ -1032,6 +1080,7 @@ install_state(struct ip_fw *rule, ipfw_insn_limit *cmd,
 			last_log = time_second;
 			printf("ipfw: install_state: Too many dynamic rules\n");
 		}
+		IPFW_DYN_UNLOCK();
 		return 1; /* cannot install, notify caller */
 	}
 
@@ -1077,6 +1126,7 @@ install_state(struct ip_fw *rule, ipfw_insn_limit *cmd,
 					log(LOG_SECURITY | LOG_DEBUG,
 					    "drop session, too many entries\n");
 				}
+				IPFW_DYN_UNLOCK();
 				return 1;
 			}
 		}
@@ -1085,9 +1135,11 @@ install_state(struct ip_fw *rule, ipfw_insn_limit *cmd,
 		break;
 	default:
 		printf("ipfw: unknown dynamic rule type %u\n", cmd->o.opcode);
+		IPFW_DYN_UNLOCK();
 		return 1;
 	}
-	lookup_dyn_rule(&args->f_id, NULL, NULL); /* XXX just set lifetime */
+	lookup_dyn_rule_locked(&args->f_id, NULL, NULL); /* XXX just set lifetime */
+	IPFW_DYN_UNLOCK();
 	return 0;
 }
 
@@ -1349,6 +1401,7 @@ ipfw_chk(struct ip_fw_args *args)
 	int pktlen;
 	int dyn_dir = MATCH_UNKNOWN;
 	ipfw_dyn_rule *q = NULL;
+	struct ip_fw_chain *chain = &layer3_chain;
 
 	if (m->m_flags & M_SKIP_FIREWALL)
 		return 0;	/* accept */
@@ -1436,6 +1489,7 @@ ipfw_chk(struct ip_fw_args *args)
 	args->f_id.dst_port = dst_port = ntohs(dst_port);
 
 after_ip_checks:
+	IPFW_LOCK(chain);		/* XXX expensive? can we run lock free? */
 	if (args->rule) {
 		/*
 		 * Packet has already been tagged. Look for the next rule
@@ -1445,8 +1499,10 @@ after_ip_checks:
 		 * XXX should not happen here, but optimized out in
 		 * the caller.
 		 */
-		if (fw_one_pass)
+		if (fw_one_pass) {
+			IPFW_UNLOCK(chain);	/* XXX optimize */
 			return 0;
+		}
 
 		f = args->rule->next_rule;
 		if (f == NULL)
@@ -1458,14 +1514,18 @@ after_ip_checks:
 		 */
 		int skipto = args->divert_rule;
 
-		f = layer3_chain;
+		f = chain->rules;
 		if (args->eh == NULL && skipto != 0) {
-			if (skipto >= IPFW_DEFAULT_RULE)
+			if (skipto >= IPFW_DEFAULT_RULE) {
+				IPFW_UNLOCK(chain);
 				return(IP_FW_PORT_DENY_FLAG); /* invalid */
+			}
 			while (f && f->rulenum <= skipto)
 				f = f->next;
-			if (f == NULL)	/* drop packet */
+			if (f == NULL) {	/* drop packet */
+				IPFW_UNLOCK(chain);
 				return(IP_FW_PORT_DENY_FLAG);
+			}
 		}
 	}
 	args->divert_rule = 0;	/* reset to avoid confusion later */
@@ -1549,6 +1609,7 @@ check_body:
 				} else
 					break;
 
+				/* XXX locking? */
 				pcb =  (oif) ?
 					in_pcblookup_hash(pi,
 					    dst_ip, htons(dst_port),
@@ -1911,6 +1972,7 @@ check_body:
 					f = q->rule;
 					cmd = ACTION_PTR(f);
 					l = f->cmd_len - f->act_ofs;
+					IPFW_DYN_UNLOCK();
 					goto check_body;
 				}
 				/*
@@ -2006,6 +2068,7 @@ next_rule:;		/* try next rule		*/
 
 	}		/* end of outer for, scan rules */
 	printf("ipfw: ouch!, skip past end of rules, denying packet\n");
+	IPFW_UNLOCK(chain);
 	return(IP_FW_PORT_DENY_FLAG);
 
 done:
@@ -2013,6 +2076,7 @@ done:
 	f->pcnt++;
 	f->bcnt += pktlen;
 	f->timestamp = time_second;
+	IPFW_UNLOCK(chain);
 	return retval;
 
 pullup_failed:
@@ -2024,28 +2088,29 @@ pullup_failed:
 /*
  * When a rule is added/deleted, clear the next_rule pointers in all rules.
  * These will be reconstructed on the fly as packets are matched.
- * Must be called at splimp().
  */
 static void
-flush_rule_ptrs(void)
+flush_rule_ptrs(struct ip_fw_chain *chain)
 {
 	struct ip_fw *rule;
 
-	for (rule = layer3_chain; rule; rule = rule->next)
+	IPFW_LOCK_ASSERT(chain);
+
+	for (rule = chain->rules; rule; rule = rule->next)
 		rule->next_rule = NULL;
 }
 
 /*
  * When pipes/queues are deleted, clear the "pipe_ptr" pointer to a given
  * pipe/queue, or to all of them (match == NULL).
- * Must be called at splimp().
  */
 void
 flush_pipe_ptrs(struct dn_flow_set *match)
 {
 	struct ip_fw *rule;
 
-	for (rule = layer3_chain; rule; rule = rule->next) {
+	IPFW_LOCK(&layer3_chain);
+	for (rule = layer3_chain.rules; rule; rule = rule->next) {
 		ipfw_insn_pipe *cmd = (ipfw_insn_pipe *)ACTION_PTR(rule);
 
 		if (cmd->o.opcode != O_PIPE && cmd->o.opcode != O_QUEUE)
@@ -2060,6 +2125,7 @@ flush_pipe_ptrs(struct dn_flow_set *match)
 		    !bcmp(&cmd->pipe_ptr, &match, sizeof(match)) )
 			bzero(&cmd->pipe_ptr, sizeof(cmd->pipe_ptr));
 	}
+	IPFW_UNLOCK(&layer3_chain);
 }
 
 /*
@@ -2068,13 +2134,12 @@ flush_pipe_ptrs(struct dn_flow_set *match)
  * Update the rule_number in the input struct so the caller knows it as well.
  */
 static int
-add_rule(struct ip_fw **head, struct ip_fw *input_rule)
+add_rule(struct ip_fw_chain *chain, struct ip_fw *input_rule)
 {
 	struct ip_fw *rule, *f, *prev;
-	int s;
 	int l = RULESIZE(input_rule);
 
-	if (*head == NULL && input_rule->rulenum != IPFW_DEFAULT_RULE)
+	if (chain->rules == NULL && input_rule->rulenum != IPFW_DEFAULT_RULE)
 		return (EINVAL);
 
 	rule = malloc(l, M_IPFW, M_NOWAIT | M_ZERO);
@@ -2090,10 +2155,10 @@ add_rule(struct ip_fw **head, struct ip_fw *input_rule)
 	rule->bcnt = 0;
 	rule->timestamp = 0;
 
-	s = splimp();
+	IPFW_LOCK(chain);
 
-	if (*head == NULL) {	/* default rule */
-		*head = rule;
+	if (chain->rules == NULL) {	/* default rule */
+		chain->rules = rule;
 		goto done;
         }
 
@@ -2109,7 +2174,7 @@ add_rule(struct ip_fw **head, struct ip_fw *input_rule)
 		/*
 		 * locate the highest numbered rule before default
 		 */
-		for (f = *head; f; f = f->next) {
+		for (f = chain->rules; f; f = f->next) {
 			if (f->rulenum == IPFW_DEFAULT_RULE)
 				break;
 			rule->rulenum = f->rulenum;
@@ -2122,72 +2187,94 @@ add_rule(struct ip_fw **head, struct ip_fw *input_rule)
 	/*
 	 * Now insert the new rule in the right place in the sorted list.
 	 */
-	for (prev = NULL, f = *head; f; prev = f, f = f->next) {
+	for (prev = NULL, f = chain->rules; f; prev = f, f = f->next) {
 		if (f->rulenum > rule->rulenum) { /* found the location */
 			if (prev) {
 				rule->next = f;
 				prev->next = rule;
 			} else { /* head insert */
-				rule->next = *head;
-				*head = rule;
+				rule->next = chain->rules;
+				chain->rules = rule;
 			}
 			break;
 		}
 	}
-	flush_rule_ptrs();
+	flush_rule_ptrs(chain);
 done:
 	static_count++;
 	static_len += l;
-	splx(s);
+	IPFW_UNLOCK(chain);
 	DEB(printf("ipfw: installed rule %d, static count now %d\n",
 		rule->rulenum, static_count);)
 	return (0);
 }
 
 /**
- * Free storage associated with a static rule (including derived
- * dynamic rules).
+ * Remove a static rule (including derived * dynamic rules)
+ * and place it on the ``reap list'' for later reclamation.
  * The caller is in charge of clearing rule pointers to avoid
  * dangling pointers.
  * @return a pointer to the next entry.
  * Arguments are not checked, so they better be correct.
- * Must be called at splimp().
  */
 static struct ip_fw *
-delete_rule(struct ip_fw **head, struct ip_fw *prev, struct ip_fw *rule)
+remove_rule(struct ip_fw_chain *chain, struct ip_fw *rule, struct ip_fw *prev)
 {
 	struct ip_fw *n;
 	int l = RULESIZE(rule);
 
+	IPFW_LOCK_ASSERT(chain);
+
 	n = rule->next;
+	IPFW_DYN_LOCK();
 	remove_dyn_rule(rule, NULL /* force removal */);
+	IPFW_DYN_UNLOCK();
 	if (prev == NULL)
-		*head = n;
+		chain->rules = n;
 	else
 		prev->next = n;
 	static_count--;
 	static_len -= l;
 
-	if (DUMMYNET_LOADED)
-		ip_dn_ruledel_ptr(rule);
-	free(rule, M_IPFW);
+	rule->next = chain->reap;
+	chain->reap = rule;
+
 	return n;
 }
 
+/**
+ * Reclaim storage associated with a list of rules.  This is
+ * typically the list created using remove_rule.
+ */
+static void
+reap_rules(struct ip_fw *head)
+{
+	struct ip_fw *rule;
+
+	while ((rule = head) != NULL) {
+		head = head->next;
+		if (DUMMYNET_LOADED)
+			ip_dn_ruledel_ptr(rule);
+		free(rule, M_IPFW);
+	}
+}
+
 /*
- * Deletes all rules from a chain (except rules in set RESVD_SET
- * unless kill_default = 1).
- * Must be called at splimp().
+ * Remove all rules from a chain (except rules in set RESVD_SET
+ * unless kill_default = 1).  The caller is responsible for
+ * reclaiming storage for the rules left in chain->reap.
  */
 static void
-free_chain(struct ip_fw **chain, int kill_default)
+free_chain(struct ip_fw_chain *chain, int kill_default)
 {
 	struct ip_fw *prev, *rule;
 
-	flush_rule_ptrs(); /* more efficient to do outside the loop */
-	for (prev = NULL, rule = *chain; rule ; )
+	IPFW_LOCK_ASSERT(chain);
+
+	flush_rule_ptrs(chain); /* more efficient to do outside the loop */
+	for (prev = NULL, rule = chain->rules; rule ; )
 		if (kill_default || rule->set != RESVD_SET)
-			rule = delete_rule(chain, prev, rule);
+			rule = remove_rule(chain, rule, prev);
 		else {
 			prev = rule;
 			rule = rule->next;
@@ -2208,10 +2295,9 @@ free_chain(struct ip_fw **chain, int kill_default)
  *	4	swap sets with given numbers
  */
 static int
-del_entry(struct ip_fw **chain, u_int32_t arg)
+del_entry(struct ip_fw_chain *chain, u_int32_t arg)
 {
-	struct ip_fw *prev = NULL, *rule = *chain;
-	int s;
+	struct ip_fw *prev = NULL, *rule;
 	u_int16_t rulenum;	/* rule or old_set */
 	u_int8_t cmd, new_set;
 
@@ -2231,6 +2317,9 @@ del_entry(struct ip_fw **chain, u_int32_t arg)
 			return EINVAL;
 	}
 
+	IPFW_LOCK(chain);
+	rule = chain->rules;
+	chain->reap = NULL;
 	switch (cmd) {
 	case 0:	/* delete rules with given number */
 		/*
@@ -2238,64 +2327,69 @@ del_entry(struct ip_fw **chain, u_int32_t arg)
 		 */
 		for (; rule->rulenum < rulenum; prev = rule, rule = rule->next)
 			;
-		if (rule->rulenum != rulenum)
+		if (rule->rulenum != rulenum) {
+			IPFW_UNLOCK(chain);
 			return EINVAL;
+		}
 
-		s = splimp(); /* no access to rules while removing */
 		/*
 		 * flush pointers outside the loop, then delete all matching
 		 * rules. prev remains the same throughout the cycle.
 		 */
-		flush_rule_ptrs();
+		flush_rule_ptrs(chain);
 		while (rule->rulenum == rulenum)
-			rule = delete_rule(chain, prev, rule);
-		splx(s);
+			rule = remove_rule(chain, rule, prev);
 		break;
 
 	case 1:	/* delete all rules with given set number */
-		s = splimp();
-		flush_rule_ptrs();
+		flush_rule_ptrs(chain);
+		rule = chain->rules;
 		while (rule->rulenum < IPFW_DEFAULT_RULE)
 			if (rule->set == rulenum)
-				rule = delete_rule(chain, prev, rule);
+				rule = remove_rule(chain, rule, prev);
 			else {
 				prev = rule;
 				rule = rule->next;
 			}
-		splx(s);
 		break;
 
 	case 2:	/* move rules with given number to new set */
-		s = splimp();
+		rule = chain->rules;
 		for (; rule->rulenum < IPFW_DEFAULT_RULE; rule = rule->next)
 			if (rule->rulenum == rulenum)
 				rule->set = new_set;
-		splx(s);
 		break;
 
 	case 3: /* move rules with given set number to new set */
-		s = splimp();
 		for (; rule->rulenum < IPFW_DEFAULT_RULE; rule = rule->next)
 			if (rule->set == rulenum)
 				rule->set = new_set;
-		splx(s);
 		break;
 
 	case 4: /* swap two sets */
-		s = splimp();
 		for (; rule->rulenum < IPFW_DEFAULT_RULE; rule = rule->next)
 			if (rule->set == rulenum)
 				rule->set = new_set;
 			else if (rule->set == new_set)
 				rule->set = rulenum;
-		splx(s);
 		break;
 	}
+	/*
+	 * Look for rules to reclaim.  We grab the list before
+	 * releasing the lock then reclaim them w/o the lock to
+	 * avoid a LOR with dummynet.
+	 */
+	rule = chain->reap;
+	chain->reap = NULL;
+	IPFW_UNLOCK(chain);
+	if (rule)
+		reap_rules(rule);
 	return 0;
 }
 
 /*
  * Clear counters for a specific rule.
+ * The enclosing "table" is assumed locked.
  */
 static void
 clear_counters(struct ip_fw *rule, int log_only)
@@ -2317,18 +2411,16 @@ clear_counters(struct ip_fw *rule, int log_only)
  * @arg log_only is 1 if we only want to reset logs, zero otherwise.
  */
 static int
-zero_entry(int rulenum, int log_only)
+zero_entry(struct ip_fw_chain *chain, int rulenum, int log_only)
 {
 	struct ip_fw *rule;
-	int s;
 	char *msg;
 
+	IPFW_LOCK(chain);
 	if (rulenum == 0) {
-		s = splimp();
 		norule_counter = 0;
-		for (rule = layer3_chain; rule; rule = rule->next)
+		for (rule = chain->rules; rule; rule = rule->next)
 			clear_counters(rule, log_only);
-		splx(s);
 		msg = log_only ? "ipfw: All logging counts reset.\n" :
 				"ipfw: Accounting cleared.\n";
 	} else {
@@ -2337,22 +2429,24 @@ zero_entry(int rulenum, int log_only)
 		 * We can have multiple rules with the same number, so we
 		 * need to clear them all.
 		 */
-		for (rule = layer3_chain; rule; rule = rule->next)
+		for (rule = chain->rules; rule; rule = rule->next)
 			if (rule->rulenum == rulenum) {
-				s = splimp();
 				while (rule && rule->rulenum == rulenum) {
 					clear_counters(rule, log_only);
 					rule = rule->next;
 				}
-				splx(s);
 				cleared = 1;
 				break;
 			}
-		if (!cleared)	/* we did not find any matching rules */
+		if (!cleared) {	/* we did not find any matching rules */
+			IPFW_UNLOCK(chain);
 			return (EINVAL);
+		}
 		msg = log_only ? "ipfw: Entry %d logging count reset.\n" :
 				"ipfw: Entry %d cleared.\n";
 	}
+	IPFW_UNLOCK(chain);
+
 	if (fw_verbose)
 		log(LOG_SECURITY | LOG_NOTICE, msg, rulenum);
 	return (0);
@@ -2542,6 +2636,69 @@ bad_size:
 	return EINVAL;
 }
 
+/*
+ * Copy the static and dynamic rules to the supplied buffer
+ * and return the amount of space actually used.
+ */
+static size_t
+ipfw_getrules(struct ip_fw_chain *chain, void *buf, size_t space)
+{
+	char *bp = buf;
+	char *ep = bp + space;
+	struct ip_fw *rule;
+	int i;
+
+	/* XXX this can take a long time and locking will block packet flow */
+	IPFW_LOCK(chain);
+	for (rule = chain->rules; rule ; rule = rule->next) {
+		/*
+		 * Verify the entry fits in the buffer in case the
+		 * rules changed between calculating buffer space and
+		 * now.  This would be better done using a generation
+		 * number but should suffice for now.
+		 */
+		i = RULESIZE(rule);
+		if (bp + i <= ep) {
+			bcopy(rule, bp, i);
+			bcopy(&set_disable, &(((struct ip_fw *)bp)->next_rule),
+			    sizeof(set_disable));
+			bp += i;
+		}
+	}
+	IPFW_UNLOCK(chain);
+	if (ipfw_dyn_v) {
+		ipfw_dyn_rule *p, *last = NULL;
+
+		IPFW_DYN_LOCK();
+		for (i = 0 ; i < curr_dyn_buckets; i++)
+			for (p = ipfw_dyn_v[i] ; p != NULL; p = p->next) {
+				if (bp + sizeof *p <= ep) {
+					ipfw_dyn_rule *dst =
+						(ipfw_dyn_rule *)bp;
+					bcopy(p, dst, sizeof *p);
+					bcopy(&(p->rule->rulenum), &(dst->rule),
+					    sizeof(p->rule->rulenum));
+					/*
+					 * store a non-null value in "next".
+					 * The userland code will interpret a
+					 * NULL here as a marker
+					 * for the last dynamic rule.
+					 */
+					bcopy(&dst, &dst->next, sizeof(dst));
+					last = dst;
+					dst->expire =
+					    TIME_LEQ(dst->expire, time_second) ?
+						0 : dst->expire - time_second ;
+					bp += sizeof(ipfw_dyn_rule);
+				}
+			}
+		IPFW_DYN_UNLOCK();
+		if (last != NULL) /* mark last dynamic rule */
+			bzero(&last->next, sizeof(last));
+	}
+	return (bp - (char *)buf);
+}
+
 
 /**
  * {set|get}sockopt parser.
@@ -2549,11 +2706,11 @@ bad_size:
 static int
 ipfw_ctl(struct sockopt *sopt)
 {
-	int error, s, rulenum;
+#define	RULE_MAXSIZE	(256*sizeof(u_int32_t))
+	int error, rule_num;
 	size_t size;
-	struct ip_fw *bp , *buf, *rule;
-
-	static u_int32_t rule_buf[255];	/* we copy the data here */
+	struct ip_fw *buf, *rule;
+	u_int32_t rulenum[2];
 
 	/*
 	 * Disallow modifications in really-really secure mode, but still allow
@@ -2580,8 +2737,12 @@ ipfw_ctl(struct sockopt *sopt)
 		 * come first (the last of which has number IPFW_DEFAULT_RULE),
 		 * followed by a possibly empty list of dynamic rule.
 		 * The last dynamic rule has NULL in the "next" field.
+		 *
+		 * Note that the calculated size is used to bound the
+		 * amount of data returned to the user.  The rule set may
+		 * change between calculating the size and returning the
+		 * data in which case we'll just return what fits.
 		 */
-		s = splimp();
 		size = static_len;	/* size of static rules */
 		if (ipfw_dyn_v)		/* add size of dyn.rules */
 			size += (dyn_count * sizeof(ipfw_dyn_rule));
@@ -2592,49 +2753,8 @@ ipfw_ctl(struct sockopt *sopt)
 		 * buffer, just jump to the sooptcopyout.
 		 */
 		buf = malloc(size, M_TEMP, M_WAITOK);
-		if (buf == 0) {
-			splx(s);
-			error = ENOBUFS;
-			break;
-		}
-
-		bp = buf;
-		for (rule = layer3_chain; rule ; rule = rule->next) {
-			int i = RULESIZE(rule);
-			bcopy(rule, bp, i);
-			bcopy(&set_disable, &(bp->next_rule),
-			    sizeof(set_disable));
-			bp = (struct ip_fw *)((char *)bp + i);
-		}
-		if (ipfw_dyn_v) {
-			int i;
-			ipfw_dyn_rule *p, *dst, *last = NULL;
-
-			dst = (ipfw_dyn_rule *)bp;
-			for (i = 0 ; i < curr_dyn_buckets ; i++ )
-				for ( p = ipfw_dyn_v[i] ; p != NULL ;
-				    p = p->next, dst++ ) {
-					bcopy(p, dst, sizeof *p);
-					bcopy(&(p->rule->rulenum), &(dst->rule),
-					    sizeof(p->rule->rulenum));
-					/*
-					 * store a non-null value in "next".
-					 * The userland code will interpret a
-					 * NULL here as a marker
-					 * for the last dynamic rule.
-					 */
-					bcopy(&dst, &dst->next, sizeof(dst));
-					last = dst ;
-					dst->expire =
-					    TIME_LEQ(dst->expire, time_second) ?
-						0 : dst->expire - time_second ;
-				}
-			if (last != NULL) /* mark last dynamic rule */
-				bzero(&last->next, sizeof(last));
-		}
-		splx(s);
-
-		error = sooptcopyout(sopt, buf, size);
+		error = sooptcopyout(sopt, buf,
+				ipfw_getrules(&layer3_chain, buf, size));
 		free(buf, M_TEMP);
 		break;
 
@@ -2652,23 +2772,28 @@ ipfw_ctl(struct sockopt *sopt)
 		 * the old list without the need for a lock.
 		 */
 
-		s = splimp();
+		IPFW_LOCK(&layer3_chain);
+		layer3_chain.reap = NULL;
 		free_chain(&layer3_chain, 0 /* keep default rule */);
-		splx(s);
+		rule = layer3_chain.reap, layer3_chain.reap = NULL;
+		IPFW_UNLOCK(&layer3_chain);
+		if (layer3_chain.reap != NULL)
+			reap_rules(rule);
 		break;
 
 	case IP_FW_ADD:
-		rule = (struct ip_fw *)rule_buf; /* XXX do a malloc */
-		error = sooptcopyin(sopt, rule, sizeof(rule_buf),
+		rule = malloc(RULE_MAXSIZE, M_TEMP, M_WAITOK);
+		error = sooptcopyin(sopt, rule, RULE_MAXSIZE,
 			sizeof(struct ip_fw) );
-		size = sopt->sopt_valsize;
-		if (error || (error = check_ipfw_struct(rule, size)))
-			break;
-
-		error = add_rule(&layer3_chain, rule);
-		size = RULESIZE(rule);
-		if (!error && sopt->sopt_dir == SOPT_GET)
-			error = sooptcopyout(sopt, rule, size);
+		if (error == 0)
+			error = check_ipfw_struct(rule, sopt->sopt_valsize);
+		if (error == 0) {
+			error = add_rule(&layer3_chain, rule);
+			size = RULESIZE(rule);
+			if (!error && sopt->sopt_dir == SOPT_GET)
+				error = sooptcopyout(sopt, rule, size);
+		}
+		free(rule, M_TEMP);
 		break;
 
 	case IP_FW_DEL:
@@ -2684,16 +2809,16 @@ ipfw_ctl(struct sockopt *sopt)
 		 *	first u_int32_t contains sets to be disabled,
 		 *	second u_int32_t contains sets to be enabled.
 		 */
-		error = sooptcopyin(sopt, rule_buf,
+		error = sooptcopyin(sopt, rulenum,
 			2*sizeof(u_int32_t), sizeof(u_int32_t));
 		if (error)
 			break;
 		size = sopt->sopt_valsize;
 		if (size == sizeof(u_int32_t))	/* delete or reassign */
-			error = del_entry(&layer3_chain, rule_buf[0]);
+			error = del_entry(&layer3_chain, rulenum[0]);
 		else if (size == 2*sizeof(u_int32_t)) /* set enable/disable */
 			set_disable =
-			    (set_disable | rule_buf[0]) & ~rule_buf[1] &
+			    (set_disable | rulenum[0]) & ~rulenum[1] &
 			    ~(1<<RESVD_SET); /* set RESVD_SET always enabled */
 		else
 			error = EINVAL;
@@ -2701,15 +2826,15 @@ ipfw_ctl(struct sockopt *sopt)
 
 	case IP_FW_ZERO:
 	case IP_FW_RESETLOG: /* argument is an int, the rule number */
-		rulenum=0;
-
+		rule_num = 0;
 		if (sopt->sopt_val != 0) {
-		    error = sooptcopyin(sopt, &rulenum,
+		    error = sooptcopyin(sopt, &rule_num,
 			    sizeof(int), sizeof(int));
 		    if (error)
 			break;
 		}
-		error = zero_entry(rulenum, sopt->sopt_name == IP_FW_RESETLOG);
+		error = zero_entry(&layer3_chain, rule_num,
+			sopt->sopt_name == IP_FW_RESETLOG);
 		break;
 
 	default:
@@ -2718,6 +2843,7 @@ ipfw_ctl(struct sockopt *sopt)
 	}
 
 	return (error);
+#undef RULE_MAXSIZE
 }
 
 /**
@@ -2737,13 +2863,12 @@ static void
 ipfw_tick(void * __unused unused)
 {
 	int i;
-	int s;
 	ipfw_dyn_rule *q;
 
 	if (dyn_keepalive == 0 || ipfw_dyn_v == NULL || dyn_count == 0)
 		goto done;
 
-	s = splimp();
+	IPFW_DYN_LOCK();
 	for (i = 0 ; i < curr_dyn_buckets ; i++) {
 		for (q = ipfw_dyn_v[i] ; q ; q = q->next ) {
 			if (q->dyn_type == O_LIMIT_PARENT)
@@ -2762,19 +2887,21 @@ ipfw_tick(void * __unused unused)
 			send_pkt(&(q->id), q->ack_fwd - 1, q->ack_rev, 0);
 		}
 	}
-	splx(s);
+	IPFW_DYN_UNLOCK();
 done:
-	ipfw_timeout_h = timeout(ipfw_tick, NULL, dyn_keepalive_period*hz);
+	callout_reset(&ipfw_timeout, dyn_keepalive_period*hz, ipfw_tick, NULL);
 }
 
-static void
+static int
 ipfw_init(void)
 {
 	struct ip_fw default_rule;
+	int error;
 
-	ip_fw_chk_ptr = ipfw_chk;
-	ip_fw_ctl_ptr = ipfw_ctl;
-	layer3_chain = NULL;
+	layer3_chain.rules = NULL;
+	IPFW_LOCK_INIT(&layer3_chain);
+	IPFW_DYN_LOCK_INIT();
+	callout_init(&ipfw_timeout, CALLOUT_MPSAFE);
 
 	bzero(&default_rule, sizeof default_rule);
 
@@ -2790,9 +2917,16 @@ ipfw_init(void)
 #endif
 				O_DENY;
 
-	add_rule(&layer3_chain, &default_rule);
+	error = add_rule(&layer3_chain, &default_rule);
+	if (error != 0) {
+		printf("ipfw2: error %u initializing default rule "
+			"(support disabled)\n", error);
+		IPFW_DYN_LOCK_DESTROY();
+		IPFW_LOCK_DESTROY(&layer3_chain);
+		return (error);
+	}
 
-	ip_fw_default_rule = layer3_chain;
+	ip_fw_default_rule = layer3_chain.rules;
 	printf("ipfw2 initialized, divert %s, "
 		"rule-based forwarding enabled, default to %s, logging ",
 #ifdef IPDIVERT
@@ -2815,42 +2949,53 @@ ipfw_init(void)
 	else
 		printf("limited to %d packets/entry by default\n",
 		    verbose_limit);
-	bzero(&ipfw_timeout_h, sizeof(struct callout_handle));
-	ipfw_timeout_h = timeout(ipfw_tick, NULL, hz);
+
+	ip_fw_chk_ptr = ipfw_chk;
+	ip_fw_ctl_ptr = ipfw_ctl;
+	callout_reset(&ipfw_timeout, hz, ipfw_tick, NULL);
+
+	return (0);
+}
+
+static void
+ipfw_destroy(void)
+{
+	struct ip_fw *reap;
+
+	IPFW_LOCK(&layer3_chain);
+	callout_stop(&ipfw_timeout);
+	ip_fw_chk_ptr = NULL;
+	ip_fw_ctl_ptr = NULL;
+	layer3_chain.reap = NULL;
+	free_chain(&layer3_chain, 1 /* kill default rule */);
+	reap = layer3_chain.reap, layer3_chain.reap = NULL;
+	IPFW_UNLOCK(&layer3_chain);
+	if (reap != NULL)
+		reap_rules(reap);
+
+	IPFW_DYN_LOCK_DESTROY();
+	IPFW_LOCK_DESTROY(&layer3_chain);
+	printf("IP firewall unloaded\n");
 }
 
 static int
 ipfw_modevent(module_t mod, int type, void *unused)
 {
-	int s;
 	int err = 0;
 
 	switch (type) {
 	case MOD_LOAD:
-		s = splimp();
 		if (IPFW_LOADED) {
-			splx(s);
 			printf("IP firewall already loaded\n");
 			err = EEXIST;
 		} else {
-			ipfw_init();
-			splx(s);
+			err = ipfw_init();
 		}
 		break;
 
 	case MOD_UNLOAD:
-#if !defined(KLD_MODULE)
-		printf("ipfw statically compiled, cannot unload\n");
-		err = EBUSY;
-#else
-                s = splimp();
-		untimeout(ipfw_tick, NULL, ipfw_timeout_h);
-		ip_fw_chk_ptr = NULL;
-		ip_fw_ctl_ptr = NULL;
-		free_chain(&layer3_chain, 1 /* kill default rule */);
-		splx(s);
-		printf("IP firewall unloaded\n");
-#endif
+		ipfw_destroy();
+		err = 0;
 		break;
 	default:
 		break;