Hopefully fix the recent breakage in rule deletion.

A few  more tests and this will also go into -stable where
the problem is more critical.

1 files changed, 98 insertions, 58 deletions

diff --git a/sys/netinet/ipfw/ip_fw_sockopt.c b/sys/netinet/ipfw/ip_fw_sockopt.c
index 5f203ea..c505728 100644
--- a/sys/netinet/ipfw/ip_fw_sockopt.c
+++ b/sys/netinet/ipfw/ip_fw_sockopt.c
@@ -232,12 +232,49 @@ ipfw_reap_rules(struct ip_fw *head)
 	}
 }
 
+/*
+ * Used by del_entry() to check if a rule should be kept.
+ * Returns 1 if the rule must be kept, 0 otherwise.
+ *
+ * Called with cmd = {0,1,5}.
+ * cmd == 0 matches on rule numbers, excludes rules in RESVD_SET if n == 0 ;
+ * cmd == 1 matches on set numbers only, rule numbers are ignored;
+ * cmd == 5 matches on rule and set numbers.
+ *
+ * n == 0 is a wildcard for rule numbers, there is no wildcard for sets.
+ *
+ * Rules to keep are
+ *	(default || reserved || !match_set || !match_number)
+ * where
+ *   default ::= (rule->rulenum == IPFW_DEFAULT_RULE)
+ *	// the default rule is always protected
+ *
+ *   reserved ::= (cmd == 0 && n == 0 && rule->set == RESVD_SET)
+ *	// RESVD_SET is protected only if cmd == 0 and n == 0 ("ipfw flush")
+ *
+ *   match_set ::= (cmd == 0 || rule->set == set)
+ *	// set number is ignored for cmd == 0
+ *
+ *   match_number ::= (cmd == 1 || n == 0 || n == rule->rulenum)
+ *	// number is ignored for cmd == 1 or n == 0
+ *
+ */
+static int
+keep_rule(struct ip_fw *rule, uint8_t cmd, uint8_t set, uint32_t n)
+{
+	return
+		 (rule->rulenum == IPFW_DEFAULT_RULE)		||
+		 (cmd == 0 && n == 0 && rule->set == RESVD_SET)	||
+		!(cmd == 0 || rule->set == set)			||
+		!(cmd == 1 || n == 0 || n == rule->rulenum);
+}
+
 /**
- * Remove all rules with given number, and also do set manipulation.
+ * Remove all rules with given number, or do set manipulation.
  * Assumes chain != NULL && *chain != NULL.
  *
- * The argument is an u_int32_t. The low 16 bit are the rule or set number,
- * the next 8 bits are the new set, the top 8 bits are the command:
+ * The argument is an uint32_t. The low 16 bit are the rule or set number;
+ * the next 8 bits are the new set; the top 8 bits indicate the command:
  *
  *	0	delete rules numbered "rulenum"
  *	1	delete rules in set "rulenum"
@@ -247,26 +284,26 @@ ipfw_reap_rules(struct ip_fw *head)
  *	5	delete rules "rulenum" and set "new_set"
  */
 static int
-del_entry(struct ip_fw_chain *chain, u_int32_t arg)
+del_entry(struct ip_fw_chain *chain, uint32_t arg)
 {
 	struct ip_fw *rule;
-	uint32_t rulenum;	/* rule or old_set */
+	uint32_t num;	/* rule number or old_set */
 	uint8_t cmd, new_set;
-	int start, end = 0, i, ofs, n;
+	int start, end, i, ofs, n;
 	struct ip_fw **map = NULL;
 	int error = 0;
 
-	rulenum = arg & 0xffff;
+	num = arg & 0xffff;
 	cmd = (arg >> 24) & 0xff;
 	new_set = (arg >> 16) & 0xff;
 
 	if (cmd > 5 || new_set > RESVD_SET)
 		return EINVAL;
 	if (cmd == 0 || cmd == 2 || cmd == 5) {
-		if (rulenum >= IPFW_DEFAULT_RULE)
+		if (num >= IPFW_DEFAULT_RULE)
 			return EINVAL;
 	} else {
-		if (rulenum > RESVD_SET)	/* old_set */
+		if (num > RESVD_SET)	/* old_set */
 			return EINVAL;
 	}
 
@@ -274,20 +311,24 @@ del_entry(struct ip_fw_chain *chain, u_int32_t arg)
 	chain->reap = NULL;	/* prepare for deletions */
 
 	switch (cmd) {
-	case 0:	/* delete rules "rulenum" (rulenum == 0 matches all) */
+	case 0:	/* delete rules "num" (num == 0 matches all) */
 	case 1:	/* delete all rules in set N */
 	case 5: /* delete rules with number N and set "new_set". */
 
 		/*
 		 * Locate first rule to delete (start), the rule after
 		 * the last one to delete (end), and count how many
-		 * rules to delete (n)
+		 * rules to delete (n). Always use keep_rule() to
+		 * determine which rules to keep.
 		 */
 		n = 0;
-		if (cmd == 1) { /* look for a specific set, must scan all */
-			new_set = rulenum;
-			for (start = -1, i = 0; i < chain->n_rules; i++) {
-				if (chain->map[i]->set != new_set)
+		if (cmd == 1) {
+			/* look for a specific set including RESVD_SET.
+			 * Must scan the entire range, ignore num.
+			 */
+			new_set = num;
+			for (start = -1, end = i = 0; i < chain->n_rules; i++) {
+				if (keep_rule(chain->map[i], cmd, new_set, 0))
 					continue;
 				if (start < 0)
 					start = i;
@@ -296,61 +337,54 @@ del_entry(struct ip_fw_chain *chain, u_int32_t arg)
 			}
 			end++;	/* first non-matching */
 		} else {
-			start = ipfw_find_rule(chain, rulenum, 0);
+			/* Optimized search on rule numbers */
+			start = ipfw_find_rule(chain, num, 0);
 			for (end = start; end < chain->n_rules; end++) {
 				rule = chain->map[end];
-				if (rulenum > 0 && rule->rulenum != rulenum)
+				if (num > 0 && rule->rulenum != num)
 					break;
-				if (rule->set != RESVD_SET &&
-				    (cmd == 0 || rule->set == new_set) )
+				if (!keep_rule(rule, cmd, new_set, num))
 					n++;
 			}
 		}
-		if (n == 0 && arg == 0)
-			break; /* special case, flush on empty ruleset */
-		/* allocate the map, if needed */
-		if (n > 0)
-			map = get_map(chain, -n, 1 /* locked */);
-		if (n == 0 || map == NULL) {
+
+		if (n == 0) {
+			/* A flush request (arg == 0) on empty ruleset
+			 * returns with no error. On the contrary,
+			 * if there is no match on a specific request,
+			 * we return EINVAL.
+			 */
+			error = (arg == 0) ? 0 : EINVAL;
+			break;
+		}
+
+		/* We have something to delete. Allocate the new map */
+		map = get_map(chain, -n, 1 /* locked */);
+		if (map == NULL) {
 			error = EINVAL;
 			break;
 		}
-		/*
-		 * bcopy the initial part of the map, then individually
-		 * copy all matching entries between start and end,
-		 * and then bcopy the final part.
-		 * Once we are done we can swap maps and clean up the
-		 * deleted rules (unfortunately we need to repeat a
-		 * convoluted test). Rules to keep are
-		 *	(set == RESVD_SET || !match_set || !match_rule)
-		 * where
-		 *   match_set ::= (cmd == 0 || rule->set == new_set)
-		 *   match_rule ::= (cmd == 1 || rule->rulenum == rulenum)
-		 */
+
+		/* 1. bcopy the initial part of the map */
 		if (start > 0)
 			bcopy(chain->map, map, start * sizeof(struct ip_fw *));
+		/* 2. copy active rules between start and end */
 		for (i = ofs = start; i < end; i++) {
 			rule = chain->map[i];
-			if (rule->set == RESVD_SET ||
-			    !(cmd == 0 || rule->set == new_set) ||
-			    !(cmd == 1 || rule->rulenum == rulenum) ) {
-				map[ofs++] = chain->map[i];
-			}
+			if (keep_rule(rule, cmd, new_set, num))
+				map[ofs++] = rule;
 		}
+		/* 3. copy the final part of the map */
 		bcopy(chain->map + end, map + ofs,
 			(chain->n_rules - end) * sizeof(struct ip_fw *));
-
+		/* 4. swap the maps (under BH_LOCK) */
 		map = swap_map(chain, map, chain->n_rules - n);
-		/* now remove the rules deleted */
+		/* 5. now remove the rules deleted from the old map */
 		for (i = start; i < end; i++) {
 			int l;
 			rule = map[i];
-			/* same test as above */
-			if (rule->set == RESVD_SET ||
-			    !(cmd == 0 || rule->set == new_set) ||
-			    !(cmd == 1 || rule->rulenum == rulenum) )
+			if (keep_rule(rule, cmd, new_set, num))
 				continue;
-
 			l = RULESIZE(rule);
 			chain->static_len -= l;
 			ipfw_remove_dyn_children(rule);
@@ -359,32 +393,38 @@ del_entry(struct ip_fw_chain *chain, u_int32_t arg)
 		}
 		break;
 
-	case 2:	/* move rules with given number to new set */
-		for (i = 0; i < chain->n_rules; i++) {
+	/*
+	 * In the next 3 cases the loop stops at (n_rules - 1)
+	 * because the default rule is never eligible..
+	 */
+
+	case 2:	/* move rules with given RULE number to new set */
+		for (i = 0; i < chain->n_rules - 1; i++) {
 			rule = chain->map[i];
-			if (rule->rulenum == rulenum)
+			if (rule->rulenum == num)
 				rule->set = new_set;
 		}
 		break;
 
-	case 3: /* move rules with given set number to new set */
-		for (i = 0; i < chain->n_rules; i++) {
+	case 3: /* move rules with given SET number to new set */
+		for (i = 0; i < chain->n_rules - 1; i++) {
 			rule = chain->map[i];
-			if (rule->set == rulenum)
+			if (rule->set == num)
 				rule->set = new_set;
 		}
 		break;
 
 	case 4: /* swap two sets */
-		for (i = 0; i < chain->n_rules; i++) {
+		for (i = 0; i < chain->n_rules - 1; i++) {
 			rule = chain->map[i];
-			if (rule->set == rulenum)
+			if (rule->set == num)
 				rule->set = new_set;
 			else if (rule->set == new_set)
-				rule->set = rulenum;
+				rule->set = num;
 		}
 		break;
 	}
+
 	rule = chain->reap;
 	chain->reap = NULL;
 	IPFW_UH_WUNLOCK(chain);