Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 5 Aug 2003 16:26:38 -0700 (PDT)
From:      Sam Leffler <sam@FreeBSD.org>
To:        Perforce Change Reviews <perforce@freebsd.org>
Subject:   PERFORCE change 35574 for review
Message-ID:  <200308052326.h75NQc3X074170@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=35574

Change 35574 by sam@sam_ebb on 2003/08/05 16:25:47

	Checkpoint ipfw locking: we use two "big locks", one for the static
	rules and one for the dynamic rules.  It may be possible to eliminate
	one or both of these by extending the coverage of another lock (TBD).
	Only lightly tested, more testing coming shortly.

Affected files ...

.. //depot/projects/netperf/sys/netinet/ip_fw2.c#2 edit

Differences ...

==== //depot/projects/netperf/sys/netinet/ip_fw2.c#2 (text+ko) ====

@@ -102,13 +102,21 @@
 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 mtx	mtx;		/* lock guarding rule list */
+};
+#define	IPFW_LOCK(_chain)	mtx_lock(&(_chain)->mtx)
+#define	IPFW_UNLOCK(_chain)	mtx_unlock(&(_chain)->mtx)
+#define	IPFW_LOCK_ASSERT(_chain, _what)	mtx_assert(&(_chain)->mtx, _what)
+
 /*
  * 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 +182,11 @@
 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()		mtx_lock(&ipfw_dyn_mtx)
+#define	IPFW_DYN_UNLOCK()	mtx_unlock(&ipfw_dyn_mtx)
+#define	IPFW_DYN_LOCK_ASSERT(_what)	mtx_assert(&ipfw_dyn_mtx, _what)
+
 /*
  * Timeouts for various events in handing dynamic rules.
  */
@@ -395,6 +408,7 @@
 	} else {
 		struct ifaddr *ia;
 
+		/* XXX lock? */
 		TAILQ_FOREACH(ia, &ifp->if_addrhead, ifa_link) {
 			if (ia->ifa_addr == NULL)
 				continue;
@@ -704,6 +718,8 @@
 	ipfw_dyn_rule *prev, *q;
 	int i, pass = 0, max_pass = 0;
 
+	IPFW_DYN_LOCK_ASSERT(MA_OWNED);
+
 	if (ipfw_dyn_v == NULL || dyn_count == 0)
 		return;
 	/* do not expire more than once per second, it is useless */
@@ -760,7 +776,7 @@
  * 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 +790,8 @@
 	int i, dir = MATCH_NONE;
 	ipfw_dyn_rule *prev, *q=NULL;
 
+	IPFW_DYN_LOCK_ASSERT(MA_OWNED);
+
 	if (ipfw_dyn_v == NULL)
 		goto done;	/* not found */
 	i = hash_packet( pkt );
@@ -878,9 +896,25 @@
 	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(MA_OWNED);
+
 	/*
 	 * Try reallocation, make sure we have a power of 2 and do
 	 * not allow more than 64k entries. In case of overflow,
@@ -921,6 +955,8 @@
 	ipfw_dyn_rule *r;
 	int i;
 
+	IPFW_DYN_LOCK_ASSERT(MA_OWNED);
+
 	if (ipfw_dyn_v == NULL ||
 	    (dyn_count == 0 && dyn_buckets != curr_dyn_buckets)) {
 		realloc_dynamic_table();
@@ -974,6 +1010,8 @@
 	ipfw_dyn_rule *q;
 	int i;
 
+	IPFW_DYN_LOCK_ASSERT(MA_OWNED);
+
 	if (ipfw_dyn_v) {
 		i = hash_packet( pkt );
 		for (q = ipfw_dyn_v[i] ; q != NULL ; q=q->next)
@@ -1011,13 +1049,16 @@
 	    (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 +1073,7 @@
 			last_log = time_second;
 			printf("ipfw: install_state: Too many dynamic rules\n");
 		}
+		IPFW_DYN_UNLOCK();
 		return 1; /* cannot install, notify caller */
 	}
 
@@ -1077,6 +1119,7 @@
 					log(LOG_SECURITY | LOG_DEBUG,
 					    "drop session, too many entries\n");
 				}
+				IPFW_DYN_UNLOCK();
 				return 1;
 			}
 		}
@@ -1085,9 +1128,11 @@
 		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 +1394,7 @@
 	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 +1482,7 @@
 	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 +1492,10 @@
 		 * 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 +1507,18 @@
 		 */
 		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 +1602,7 @@
 				} else
 					break;
 
+				/* XXX locking? */
 				pcb =  (oif) ?
 					in_pcblookup_hash(pi,
 					    dst_ip, htons(dst_port),
@@ -1911,6 +1965,7 @@
 					f = q->rule;
 					cmd = ACTION_PTR(f);
 					l = f->cmd_len - f->act_ofs;
+					IPFW_DYN_UNLOCK();
 					goto check_body;
 				}
 				/*
@@ -2006,6 +2061,7 @@
 
 	}		/* 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 +2069,7 @@
 	f->pcnt++;
 	f->bcnt += pktlen;
 	f->timestamp = time_second;
+	IPFW_UNLOCK(chain);
 	return retval;
 
 pullup_failed:
@@ -2024,28 +2081,30 @@
 /*
  * 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, MA_OWNED);
+
+	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_ASSERT(&layer3_chain, MA_OWNED);
+
+	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)
@@ -2068,13 +2127,12 @@
  * 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 +2148,10 @@
 	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 +2167,7 @@
 		/*
 		 * 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,23 +2180,23 @@
 	/*
 	 * 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);
@@ -2151,18 +2209,21 @@
  * 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)
+delete_rule(struct ip_fw_chain *chain, struct ip_fw *prev, struct ip_fw *rule)
 {
 	struct ip_fw *n;
 	int l = RULESIZE(rule);
 
+	IPFW_LOCK_ASSERT(chain, MA_OWNED);
+
 	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--;
@@ -2177,15 +2238,16 @@
 /*
  * Deletes all rules from a chain (except rules in set RESVD_SET
  * unless kill_default = 1).
- * Must be called at splimp().
  */
 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, MA_OWNED);
+
+	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);
 		else {
@@ -2208,10 +2270,9 @@
  *	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 +2292,8 @@
 			return EINVAL;
 	}
 
+	IPFW_LOCK(chain);
+	rule = chain->rules;
 	switch (cmd) {
 	case 0:	/* delete rules with given number */
 		/*
@@ -2238,23 +2301,23 @@
 		 */
 		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);
 		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);
@@ -2262,40 +2325,36 @@
 				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;
 	}
+	IPFW_UNLOCK(chain);
 	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 +2376,16 @@
  * @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 +2394,24 @@
 		 * 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 +2601,69 @@
 	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,12 +2671,12 @@
 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;
+	struct ip_fw *buf, *rule;
+	u_int32_t rulenum[2];
 
-	static u_int32_t rule_buf[255];	/* we copy the data here */
-
 	/*
 	 * Disallow modifications in really-really secure mode, but still allow
 	 * the logging counters to be reset.
@@ -2580,8 +2702,12 @@
 		 * 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 +2718,8 @@
 		 * 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 +2737,24 @@
 		 * the old list without the need for a lock.
 		 */
 
-		s = splimp();
+		IPFW_LOCK(&layer3_chain);
 		free_chain(&layer3_chain, 0 /* keep default rule */);
-		splx(s);
+		IPFW_UNLOCK(&layer3_chain);
 		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 +2770,16 @@
 		 *	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 +2787,15 @@
 
 	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 +2804,7 @@
 	}
 
 	return (error);
+#undef RULE_MAXSIZE
 }
 
 /**
@@ -2737,13 +2824,12 @@
 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 +2848,21 @@
 			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;
+	mtx_init(&layer3_chain.mtx, "IPFW static rules", NULL, MTX_DEF);
+	mtx_init(&ipfw_dyn_mtx, "IPFW dynamic rules", NULL, MTX_DEF);
+	callout_init(&ipfw_timeout, CALLOUT_MPSAFE);
 
 	bzero(&default_rule, sizeof default_rule);
 
@@ -2790,9 +2878,16 @@
 #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);
+		mtx_destroy(&ipfw_dyn_mtx);
+		mtx_destroy(&layer3_chain.mtx);
+		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,26 +2910,26 @@
 	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 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;
 
@@ -2843,13 +2938,18 @@
 		printf("ipfw statically compiled, cannot unload\n");
 		err = EBUSY;
 #else
-                s = splimp();
-		untimeout(ipfw_tick, NULL, ipfw_timeout_h);
+		IPFW_LOCK(&layer3_chain);
+		IPFW_DYN_LOCK();
+		callout_stop(&ipfw_timeout);
 		ip_fw_chk_ptr = NULL;
 		ip_fw_ctl_ptr = NULL;
 		free_chain(&layer3_chain, 1 /* kill default rule */);
-		splx(s);
+		IPFW_DYN_UNLOCK();
+		IPFW_UNLOCK(&layer3_chain);
+		mtx_destroy(&ipfw_dyn_mtx);
+		mtx_destroy(&layer3_chain.mtx);
 		printf("IP firewall unloaded\n");
+		err = 0;
 #endif
 		break;
 	default:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200308052326.h75NQc3X074170>