4 files changed, 44 insertions, 17 deletions

diff --git a/sys/netpfil/ipfw/ip_fw_eaction.c b/sys/netpfil/ipfw/ip_fw_eaction.c
index f194d95e5075f..250061da82fcc 100644
--- a/sys/netpfil/ipfw/ip_fw_eaction.c
+++ b/sys/netpfil/ipfw/ip_fw_eaction.c
@@ -377,33 +377,30 @@ ipfw_reset_eaction(struct ip_fw_chain *ch, struct ip_fw *rule,
     uint16_t eaction_id, uint16_t default_id, uint16_t instance_id)
 {
 	ipfw_insn *cmd, *icmd;
-	int l, cmdlen;
+	int l;
 
 	IPFW_UH_WLOCK_ASSERT(ch);
 	IPFW_WLOCK_ASSERT(ch);
 
-	cmd = ACTION_PTR(rule);
-	l = rule->cmd_len - rule->act_ofs;
-	while (l > 0) {
-		cmdlen = F_LEN(cmd);
-		l -= cmdlen;
-		if (cmd->opcode == O_EXTERNAL_ACTION || l <= 0)
-			break;
-		cmd += cmdlen;
-	}
 	/*
 	 * Return if there is not O_EXTERNAL_ACTION or its id is
 	 * different.
 	 */
+	cmd = ipfw_get_action(rule);
 	if (cmd->opcode != O_EXTERNAL_ACTION ||
 	    cmd->arg1 != eaction_id)
 		return (0);
 	/*
 	 * If instance_id is specified, we need to truncate the
 	 * rule length. Check if there is O_EXTERNAL_INSTANCE opcode.
+	 *
+	 * NOTE: F_LEN(cmd) must be 1 for O_EXTERNAL_ACTION opcode,
+	 *  and rule length should be enough to keep O_EXTERNAL_INSTANCE
+	 *  opcode, thus we do check for l > 1.
 	 */
-	if (instance_id != 0 && l > 0) {
-		MPASS(cmdlen == 1);
+	l = rule->cmd + rule->cmd_len - cmd;
+	if (instance_id != 0 && l > 1) {
+		MPASS(F_LEN(cmd) == 1);
 		icmd = cmd + 1;
 		if (icmd->opcode != O_EXTERNAL_INSTANCE ||
 		    icmd->arg1 != instance_id)
@@ -415,8 +412,9 @@ ipfw_reset_eaction(struct ip_fw_chain *ch, struct ip_fw *rule,
 		 * opcode.
 		 */
 		EACTION_DEBUG("truncate rule %d: len %u -> %u",
-		    rule->rulenum, rule->cmd_len, rule->cmd_len - l);
-		rule->cmd_len -= l;
+		    rule->rulenum, rule->cmd_len,
+		    rule->cmd_len - F_LEN(icmd));
+		rule->cmd_len -= F_LEN(icmd);
 		MPASS(((uint32_t *)icmd -
 		    (uint32_t *)rule->cmd) == rule->cmd_len);
 	}
diff --git a/sys/netpfil/ipfw/ip_fw_nat.c b/sys/netpfil/ipfw/ip_fw_nat.c
index 0527d9641c3a4..6693cc533284e 100644
--- a/sys/netpfil/ipfw/ip_fw_nat.c
+++ b/sys/netpfil/ipfw/ip_fw_nat.c
@@ -140,13 +140,12 @@ ifaddr_change(void *arg __unused, struct ifnet *ifp)
 static void
 flush_nat_ptrs(struct ip_fw_chain *chain, const int ix)
 {
-	int i;
 	ipfw_insn_nat *cmd;
+	int i;
 
 	IPFW_WLOCK_ASSERT(chain);
 	for (i = 0; i < chain->n_rules; i++) {
-		cmd = (ipfw_insn_nat *)ACTION_PTR(chain->map[i]);
-		/* XXX skip log and the like ? */
+		cmd = (ipfw_insn_nat *)ipfw_get_action(chain->map[i]);
 		if (cmd->o.opcode == O_NAT && cmd->nat != NULL &&
 			    (ix < 0 || cmd->nat->id == ix))
 			cmd->nat = NULL;
diff --git a/sys/netpfil/ipfw/ip_fw_private.h b/sys/netpfil/ipfw/ip_fw_private.h
index adb7243a9a996..57fa7464e429d 100644
--- a/sys/netpfil/ipfw/ip_fw_private.h
+++ b/sys/netpfil/ipfw/ip_fw_private.h
@@ -665,6 +665,7 @@ struct ip_fw *ipfw_alloc_rule(struct ip_fw_chain *chain, size_t rulesize);
 void ipfw_free_rule(struct ip_fw *rule);
 int ipfw_match_range(struct ip_fw *rule, ipfw_range_tlv *rt);
 int ipfw_mark_object_kidx(uint32_t *bmask, uint16_t etlv, uint16_t kidx);
+ipfw_insn *ipfw_get_action(struct ip_fw *);
 
 typedef int (sopt_handler_f)(struct ip_fw_chain *ch,
     ip_fw3_opheader *op3, struct sockopt_data *sd);
diff --git a/sys/netpfil/ipfw/ip_fw_sockopt.c b/sys/netpfil/ipfw/ip_fw_sockopt.c
index 297b01ca7d3da..b0f14bf0539a2 100644
--- a/sys/netpfil/ipfw/ip_fw_sockopt.c
+++ b/sys/netpfil/ipfw/ip_fw_sockopt.c
@@ -1218,6 +1218,35 @@ move_range(struct ip_fw_chain *chain, ipfw_range_tlv *rt)
 }
 
 /*
+ * Returns pointer to action instruction, skips all possible rule
+ * modifiers like O_LOG, O_TAG, O_ALTQ.
+ */
+ipfw_insn *
+ipfw_get_action(struct ip_fw *rule)
+{
+	ipfw_insn *cmd;
+	int l, cmdlen;
+
+	cmd = ACTION_PTR(rule);
+	l = rule->cmd_len - rule->act_ofs;
+	while (l > 0) {
+		switch (cmd->opcode) {
+		case O_ALTQ:
+		case O_LOG:
+		case O_TAG:
+			break;
+		default:
+			return (cmd);
+		}
+		cmdlen = F_LEN(cmd);
+		l -= cmdlen;
+		cmd += cmdlen;
+	}
+	panic("%s: rule (%p) has not action opcode", __func__, rule);
+	return (NULL);
+}
+
+/*
  * Clear counters for a specific rule.
  * Normally run under IPFW_UH_RLOCK, but these are idempotent ops
  * so we only care that rules do not disappear.