some mostly cosmetic changes in preparation for upcoming work:

+ in many places, replace &V_layer3_chain with a local
  variable chain;
+ bring the counter of rules and static_len within ip_fw_chain
  replacing static variables;
+ remove some spurious comments and extern declaration;
+ document which lock protects certain data structures

4 files changed, 89 insertions, 92 deletions

diff --git a/sys/netinet/ipfw/ip_fw2.c b/sys/netinet/ipfw/ip_fw2.c
index 32cd548..ca998c1 100644
--- a/sys/netinet/ipfw/ip_fw2.c
+++ b/sys/netinet/ipfw/ip_fw2.c
@@ -1,5 +1,5 @@
 /*-
- * Copyright (c) 2002 Luigi Rizzo, Universita` di Pisa
+ * Copyright (c) 2002-2009 Luigi Rizzo, Universita` di Pisa
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -131,12 +131,10 @@ VNET_DEFINE(u_int32_t, set_disable);
 #define	V_set_disable			VNET(set_disable)
 
 VNET_DEFINE(int, fw_verbose);
-//#define	V_verbose_limit			VNET(verbose_limit)
 /* counter for ipfw_log(NULL...) */
 VNET_DEFINE(u_int64_t, norule_counter);
 VNET_DEFINE(int, verbose_limit);
 
-
 /* layer3_chain contains the list of rules for layer 3 */
 VNET_DEFINE(struct ip_fw_chain, layer3_chain);
 
@@ -147,8 +145,6 @@ ipfw_nat_cfg_t *ipfw_nat_del_ptr;
 ipfw_nat_cfg_t *ipfw_nat_get_cfg_ptr;
 ipfw_nat_cfg_t *ipfw_nat_get_log_ptr;
 
-extern int ipfw_chg_hook(SYSCTL_HANDLER_ARGS);
-
 #ifdef SYSCTL_NODE
 SYSCTL_NODE(_net_inet_ip, OID_AUTO, fw, CTLFLAG_RW, 0, "Firewall");
 SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, one_pass,
@@ -173,6 +169,9 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, default_to_accept, CTLFLAG_RDTUN,
     &default_to_accept, 0,
     "Make the default rule accept all packets.");
 TUNABLE_INT("net.inet.ip.fw.default_to_accept", &default_to_accept);
+SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, static_count,
+    CTLFLAG_RD, &VNET_NAME(layer3_chain.n_rules), 0,
+    "Number of static rules");
 
 #ifdef INET6
 SYSCTL_DECL(_net_inet6_ip6);
@@ -739,6 +738,17 @@ check_uidgid(ipfw_insn_u32 *insn, int proto, struct ifnet *oif,
 }
 
 /*
+ * Helper function to write the matching rule into args
+ */
+static inline void
+set_match(struct ip_fw_args *args, struct ip_fw *f, struct ip_fw_chain *chain)
+{
+	args->rule = f;
+	args->rule_id = f->id;
+	args->chain_id = chain->id;
+}
+
+/*
  * The main check routine for the firewall.
  *
  * All arguments are in args so we can modify them and return them
@@ -1942,13 +1952,9 @@ do {								\
 
 			case O_PIPE:
 			case O_QUEUE:
-				args->rule = f; /* report matching rule */
-				args->rule_id = f->id;
-				args->chain_id = chain->id;
-				if (cmd->arg1 == IP_FW_TABLEARG)
-					args->cookie = tablearg;
-				else
-					args->cookie = cmd->arg1;
+				set_match(args, f, chain);
+				args->cookie = (cmd->arg1 == IP_FW_TABLEARG) ?
+					tablearg : cmd->arg1;
 				retval = IP_FW_DUMMYNET;
 				l = 0;          /* exit inner loop */
 				done = 1;       /* exit outer loop */
@@ -2077,13 +2083,9 @@ do {								\
 
 			case O_NETGRAPH:
 			case O_NGTEE:
-				args->rule = f;	/* report matching rule */
-				args->rule_id = f->id;
-				args->chain_id = chain->id;
-				if (cmd->arg1 == IP_FW_TABLEARG)
-					args->cookie = tablearg;
-				else
-					args->cookie = cmd->arg1;
+				set_match(args, f, chain);
+				args->cookie = (cmd->arg1 == IP_FW_TABLEARG) ?
+					tablearg : cmd->arg1;
 				retval = (cmd->opcode == O_NETGRAPH) ?
 				    IP_FW_NETGRAPH : IP_FW_NGTEE;
 				l = 0;          /* exit inner loop */
@@ -2106,14 +2108,12 @@ do {								\
 				    struct cfg_nat *t;
 				    int nat_id;
 
-				    args->rule = f; /* Report matching rule. */
-				    args->rule_id = f->id;
-				    args->chain_id = chain->id;
+				    set_match(args, f, chain);
 				    t = ((ipfw_insn_nat *)cmd)->nat;
 				    if (t == NULL) {
 					nat_id = (cmd->arg1 == IP_FW_TABLEARG) ?
 						tablearg : cmd->arg1;
-					t = (*lookup_nat_ptr)(&V_layer3_chain.nat, nat_id);
+					t = (*lookup_nat_ptr)(&chain->nat, nat_id);
 
 					if (t == NULL) {
 					    retval = IP_FW_DENY;
@@ -2175,9 +2175,7 @@ do {								\
 				    else
 					ip->ip_sum = in_cksum(m, hlen);
 				    retval = IP_FW_REASS;
-				    args->rule = f;
-				    args->rule_id = f->id;
-				    args->chain_id = chain->id;
+				    set_match(args, f, chain);
 				}
 				done = 1;	/* exit outer loop */
 				break;
@@ -2311,8 +2309,13 @@ vnet_ipfw_init(const void *unused)
 {
 	int error;
 	struct ip_fw default_rule;
+	struct ip_fw_chain *chain;
+
+	chain = &V_layer3_chain;
 
 	/* First set up some values that are compile time options */
+	V_autoinc_step = 100;	/* bounded to 1..1000 in add_rule() */
+	V_fw_deny_unknown_exthdrs = 1;
 #ifdef IPFIREWALL_VERBOSE
 	V_fw_verbose = 1;
 #endif
@@ -2320,20 +2323,17 @@ vnet_ipfw_init(const void *unused)
 	V_verbose_limit = IPFIREWALL_VERBOSE_LIMIT;
 #endif
 
-	error = ipfw_init_tables(&V_layer3_chain);
+	error = ipfw_init_tables(chain);
 	if (error) {
 		panic("init_tables"); /* XXX Marko fix this ! */
 	}
 #ifdef IPFIREWALL_NAT
-	LIST_INIT(&V_layer3_chain.nat);
+	LIST_INIT(&chain->nat);
 #endif
 
-	V_autoinc_step = 100;	/* bounded to 1..1000 in add_rule() */
-
-	V_fw_deny_unknown_exthdrs = 1;
 
-	V_layer3_chain.rules = NULL;
-	IPFW_LOCK_INIT(&V_layer3_chain);
+	chain->rules = NULL;
+	IPFW_LOCK_INIT(chain);
 
 	bzero(&default_rule, sizeof default_rule);
 	default_rule.act_ofs = 0;
@@ -2342,17 +2342,17 @@ vnet_ipfw_init(const void *unused)
 	default_rule.set = RESVD_SET;
 	default_rule.cmd[0].len = 1;
 	default_rule.cmd[0].opcode = default_to_accept ? O_ACCEPT : O_DENY;
-	error = ipfw_add_rule(&V_layer3_chain, &default_rule);
+	error = ipfw_add_rule(chain, &default_rule);
 
 	if (error != 0) {
 		printf("ipfw2: error %u initializing default rule "
 			"(support disabled)\n", error);
-		IPFW_LOCK_DESTROY(&V_layer3_chain);
+		IPFW_LOCK_DESTROY(chain);
 		printf("leaving ipfw_iattach (1) with error %d\n", error);
 		return (error);
 	}
 
-	V_layer3_chain.default_rule = V_layer3_chain.rules;
+	chain->default_rule = chain->rules;
 
 	ipfw_dyn_init();
 
@@ -2386,6 +2386,7 @@ static int
 vnet_ipfw_uninit(const void *unused)
 {
 	struct ip_fw *reap;
+	struct ip_fw_chain *chain = &V_layer3_chain;
 
 	V_ipfw_vnet_ready = 0; /* tell new callers to go away */
 	/*
@@ -2400,21 +2401,21 @@ vnet_ipfw_uninit(const void *unused)
 	V_ip_fw_chk_ptr = NULL;
 	V_ip_fw_ctl_ptr = NULL;
 
-	IPFW_WLOCK(&V_layer3_chain);
+	IPFW_WLOCK(chain);
 	/* We wait on the wlock here until the last user leaves */
-	IPFW_WUNLOCK(&V_layer3_chain);
-	IPFW_WLOCK(&V_layer3_chain);
+	IPFW_WUNLOCK(chain);
+	IPFW_WLOCK(chain);
 
 	ipfw_dyn_uninit(0);	/* run the callout_drain */
-	ipfw_flush_tables(&V_layer3_chain);
-	V_layer3_chain.reap = NULL;
-	ipfw_free_chain(&V_layer3_chain, 1 /* kill default rule */);
-	reap = V_layer3_chain.reap;
-	V_layer3_chain.reap = NULL;
-	IPFW_WUNLOCK(&V_layer3_chain);
+	ipfw_flush_tables(chain);
+	chain->reap = NULL;
+	ipfw_free_chain(chain, 1 /* kill default rule */);
+	reap = chain->reap;
+	chain->reap = NULL;
+	IPFW_WUNLOCK(chain);
 	if (reap != NULL)
 		ipfw_reap_rules(reap);
-	IPFW_LOCK_DESTROY(&V_layer3_chain);
+	IPFW_LOCK_DESTROY(chain);
 	ipfw_dyn_uninit(1);	/* free the remaining parts */
 	return 0;
 }
diff --git a/sys/netinet/ipfw/ip_fw_private.h b/sys/netinet/ipfw/ip_fw_private.h
index 095de66..9c67d20 100644
--- a/sys/netinet/ipfw/ip_fw_private.h
+++ b/sys/netinet/ipfw/ip_fw_private.h
@@ -78,9 +78,11 @@ struct ip_fw_args {
 	struct mbuf	*m;		/* the mbuf chain		*/
 	struct ifnet	*oif;		/* output interface		*/
 	struct sockaddr_in *next_hop;	/* forward address		*/
+
 	struct ip_fw	*rule;		/* matching rule		*/
-	uint32_t	rule_id;	/* matching rule id */
-	uint32_t	chain_id;	/* ruleset id */
+	uint32_t	rule_id;	/* matching rule id		*/
+	uint32_t	chain_id;	/* ruleset id			*/
+
 	struct ether_header *eh;	/* for bridged packets		*/
 
 	struct ipfw_flow_id f_id;	/* grabbed from IP header	*/
@@ -174,6 +176,8 @@ struct ip_fw_chain {
 	struct ip_fw	*rules;		/* list of rules */
 	struct ip_fw	*reap;		/* list of rules to reap */
 	struct ip_fw	*default_rule;
+	int		n_rules;	/* number of static rules */
+	int		static_len;	/* total len of static rules */
 	LIST_HEAD(nat_list, cfg_nat) nat;       /* list of nat entries */
 	struct radix_node_head *tables[IPFW_TABLES_MAX];
 	struct rwlock	rwmtx;
diff --git a/sys/netinet/ipfw/ip_fw_sockopt.c b/sys/netinet/ipfw/ip_fw_sockopt.c
index d418881..49618da 100644
--- a/sys/netinet/ipfw/ip_fw_sockopt.c
+++ b/sys/netinet/ipfw/ip_fw_sockopt.c
@@ -77,23 +77,9 @@ __FBSDID("$FreeBSD$");
 MALLOC_DEFINE(M_IPFW, "IpFw/IpAcct", "IpFw/IpAcct chain's");
 
 /*
- * static variables followed by global ones
+ * static variables followed by global ones (none in this file)
  */
 
-static VNET_DEFINE(u_int32_t, static_count);	/* # of static rules */
-#define	V_static_count			VNET(static_count)
-
-static VNET_DEFINE(u_int32_t, static_len);	/* bytes of static rules */
-#define	V_static_len			VNET(static_len)
-
-#ifdef SYSCTL_NODE
-SYSCTL_DECL(_net_inet_ip_fw);
-SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, static_count,
-    CTLFLAG_RD, &VNET_NAME(static_count), 0,
-    "Number of static rules");
-
-#endif /* SYSCTL_NODE */
-
 /*
  * 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.
@@ -187,11 +173,9 @@ ipfw_add_rule(struct ip_fw_chain *chain, struct ip_fw *input_rule)
 	/* chain->id incremented inside flush_rule_ptrs() */
 	rule->id = chain->id;
 done:
-	V_static_count++;
-	V_static_len += l;
+	chain->n_rules++;
+	chain->static_len += l;
 	IPFW_WUNLOCK(chain);
-	DEB(printf("ipfw: installed rule %d, static count now %d\n",
-		rule->rulenum, V_static_count);)
 	return (0);
 }
 
@@ -218,8 +202,8 @@ remove_rule(struct ip_fw_chain *chain, struct ip_fw *rule,
 		chain->rules = n;
 	else
 		prev->next = n;
-	V_static_count--;
-	V_static_len -= l;
+	chain->n_rules--;
+	chain->static_len -= l;
 
 	rule->next = chain->reap;
 	chain->reap = rule;
@@ -839,6 +823,7 @@ ipfw_ctl(struct sockopt *sopt)
 	int error;
 	size_t size;
 	struct ip_fw *buf, *rule;
+	struct ip_fw_chain *chain;
 	u_int32_t rulenum[2];
 
 	error = priv_check(sopt->sopt_td, PRIV_NETINET_IPFW);
@@ -856,6 +841,7 @@ ipfw_ctl(struct sockopt *sopt)
 			return (error);
 	}
 
+	chain = &V_layer3_chain;
 	error = 0;
 
 	switch (sopt->sopt_name) {
@@ -871,7 +857,7 @@ ipfw_ctl(struct sockopt *sopt)
 		 * change between calculating the size and returning the
 		 * data in which case we'll just return what fits.
 		 */
-		size = V_static_len;	/* size of static rules */
+		size = chain->static_len;	/* size of static rules */
 		size += ipfw_dyn_len();
 
 		if (size >= sopt->sopt_valsize)
@@ -883,7 +869,7 @@ ipfw_ctl(struct sockopt *sopt)
 		 */
 		buf = malloc(size, M_TEMP, M_WAITOK);
 		error = sooptcopyout(sopt, buf,
-				ipfw_getrules(&V_layer3_chain, buf, size));
+				ipfw_getrules(chain, buf, size));
 		free(buf, M_TEMP);
 		break;
 
@@ -901,10 +887,10 @@ ipfw_ctl(struct sockopt *sopt)
 		 * the old list without the need for a lock.
 		 */
 
-		IPFW_WLOCK(&V_layer3_chain);
-		ipfw_free_chain(&V_layer3_chain, 0 /* keep default rule */);
-		rule = V_layer3_chain.reap;
-		IPFW_WUNLOCK(&V_layer3_chain);
+		IPFW_WLOCK(chain);
+		ipfw_free_chain(chain, 0 /* keep default rule */);
+		rule = chain->reap;
+		IPFW_WUNLOCK(chain);
 		ipfw_reap_rules(rule);
 		break;
 
@@ -915,7 +901,7 @@ ipfw_ctl(struct sockopt *sopt)
 		if (error == 0)
 			error = check_ipfw_struct(rule, sopt->sopt_valsize);
 		if (error == 0) {
-			error = ipfw_add_rule(&V_layer3_chain, rule);
+			error = ipfw_add_rule(chain, rule);
 			size = RULESIZE(rule);
 			if (!error && sopt->sopt_dir == SOPT_GET)
 				error = sooptcopyout(sopt, rule, size);
@@ -941,13 +927,14 @@ ipfw_ctl(struct sockopt *sopt)
 		if (error)
 			break;
 		size = sopt->sopt_valsize;
-		if (size == sizeof(u_int32_t))	/* delete or reassign */
-			error = del_entry(&V_layer3_chain, rulenum[0]);
-		else if (size == 2*sizeof(u_int32_t)) /* set enable/disable */
+		if (size == sizeof(u_int32_t) && rulenum[0] != 0) {
+			/* delete or reassign, locking done in del_entry() */
+			error = del_entry(chain, rulenum[0]);
+		} else if (size == 2*sizeof(u_int32_t)) { /* set enable/disable */
 			V_set_disable =
 			    (V_set_disable | rulenum[0]) & ~rulenum[1] &
 			    ~(1<<RESVD_SET); /* set RESVD_SET always enabled */
-		else
+		} else
 			error = EINVAL;
 		break;
 
@@ -960,10 +947,11 @@ ipfw_ctl(struct sockopt *sopt)
 		    if (error)
 			break;
 		}
-		error = zero_entry(&V_layer3_chain, rulenum[0],
+		error = zero_entry(chain, rulenum[0],
 			sopt->sopt_name == IP_FW_RESETLOG);
 		break;
 
+	/*--- TABLE manipulations are protected by the IPFW_LOCK ---*/
 	case IP_FW_TABLE_ADD:
 		{
 			ipfw_table_entry ent;
@@ -972,7 +960,7 @@ ipfw_ctl(struct sockopt *sopt)
 			    sizeof(ent), sizeof(ent));
 			if (error)
 				break;
-			error = ipfw_add_table_entry(&V_layer3_chain, ent.tbl,
+			error = ipfw_add_table_entry(chain, ent.tbl,
 			    ent.addr, ent.masklen, ent.value);
 		}
 		break;
@@ -985,7 +973,7 @@ ipfw_ctl(struct sockopt *sopt)
 			    sizeof(ent), sizeof(ent));
 			if (error)
 				break;
-			error = ipfw_del_table_entry(&V_layer3_chain, ent.tbl,
+			error = ipfw_del_table_entry(chain, ent.tbl,
 			    ent.addr, ent.masklen);
 		}
 		break;
@@ -998,9 +986,9 @@ ipfw_ctl(struct sockopt *sopt)
 			    sizeof(tbl), sizeof(tbl));
 			if (error)
 				break;
-			IPFW_WLOCK(&V_layer3_chain);
-			error = ipfw_flush_table(&V_layer3_chain, tbl);
-			IPFW_WUNLOCK(&V_layer3_chain);
+			IPFW_WLOCK(chain);
+			error = ipfw_flush_table(chain, tbl);
+			IPFW_WUNLOCK(chain);
 		}
 		break;
 
@@ -1011,9 +999,9 @@ ipfw_ctl(struct sockopt *sopt)
 			if ((error = sooptcopyin(sopt, &tbl, sizeof(tbl),
 			    sizeof(tbl))))
 				break;
-			IPFW_RLOCK(&V_layer3_chain);
-			error = ipfw_count_table(&V_layer3_chain, tbl, &cnt);
-			IPFW_RUNLOCK(&V_layer3_chain);
+			IPFW_RLOCK(chain);
+			error = ipfw_count_table(chain, tbl, &cnt);
+			IPFW_RUNLOCK(chain);
 			if (error)
 				break;
 			error = sooptcopyout(sopt, &cnt, sizeof(cnt));
@@ -1037,9 +1025,9 @@ ipfw_ctl(struct sockopt *sopt)
 			}
 			tbl->size = (size - sizeof(*tbl)) /
 			    sizeof(ipfw_table_entry);
-			IPFW_RLOCK(&V_layer3_chain);
-			error = ipfw_dump_table(&V_layer3_chain, tbl);
-			IPFW_RUNLOCK(&V_layer3_chain);
+			IPFW_RLOCK(chain);
+			error = ipfw_dump_table(chain, tbl);
+			IPFW_RUNLOCK(chain);
 			if (error) {
 				free(tbl, M_TEMP);
 				break;
@@ -1049,6 +1037,7 @@ ipfw_ctl(struct sockopt *sopt)
 		}
 		break;
 
+	/*--- NAT operations are protected by the IPFW_LOCK ---*/
 	case IP_FW_NAT_CFG:
 		if (IPFW_NAT_LOADED)
 			error = ipfw_nat_cfg_ptr(sopt);
diff --git a/sys/netinet/ipfw/ip_fw_table.c b/sys/netinet/ipfw/ip_fw_table.c
index 0dab070..342085b 100644
--- a/sys/netinet/ipfw/ip_fw_table.c
+++ b/sys/netinet/ipfw/ip_fw_table.c
@@ -34,6 +34,9 @@ __FBSDID("$FreeBSD$");
  * keys are network prefixes (addr/masklen), and values are integers.
  * As a degenerate case we can interpret keys as 32-bit integers
  * (with a /32 mask).
+ *
+ * The table is protected by the IPFW lock even for manipulation coming
+ * from userland, because operations are typically fast.
  */
 
 #if !defined(KLD_MODULE)