Close long existed race with net.inet.ip.fw.one_pass = 0:

If packet leaves ipfw to other kernel subsystem (dummynet, netgraph, etc)
it carries pointer to matching ipfw rule. If this packet then reinjected back
to ipfw, ruleset processing starts from that rule. If rule was deleted
meanwhile, due to existed race condition panic was possible (as well as
other odd effects like parsing rules in 'reap list').

P.S. this commit changes ABI so userland ipfw related binaries should be
recompiled.

MFC after:	1 month
Tested by:	Mikolaj Golub

9 files changed, 89 insertions, 105 deletions

diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
index f4551d1..db2f617 100644
--- a/sys/net/if_bridge.c
+++ b/sys/net/if_bridge.c
@@ -3041,11 +3041,19 @@ bridge_pfil(struct mbuf **mp, struct ifnet *bifp, struct ifnet *ifp, int dir)
 
 	if (ip_fw_chk_ptr && pfil_ipfw != 0 && dir == PFIL_OUT && ifp != NULL) {
 		INIT_VNET_INET(curvnet);
+		struct dn_pkt_tag *dn_tag;
 
 		error = -1;
-		args.rule = ip_dn_claim_rule(*mp);
-		if (args.rule != NULL && V_fw_one_pass)
-			goto ipfwpass; /* packet already partially processed */
+		dn_tag = ip_dn_claim_tag(*mp);
+		if (dn_tag != NULL) {
+			if (dn_tag->rule != NULL && V_fw_one_pass)
+				/* packet already partially processed */
+				goto ipfwpass;
+			args.rule = dn_tag->rule; /* matching rule to restart */
+			args.rule_id = dn_tag->rule_id;
+			args.chain_id = dn_tag->chain_id;
+		} else
+			args.rule = NULL;
 
 		args.m = *mp;
 		args.oif = ifp;
diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c
index 2bf4d34..7c68908 100644
--- a/sys/net/if_ethersubr.c
+++ b/sys/net/if_ethersubr.c
@@ -145,8 +145,7 @@ MALLOC_DEFINE(M_ARPCOM, "arpcom", "802.* interface internals");
 
 #if defined(INET) || defined(INET6)
 int
-ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst,
-	struct ip_fw **rule, int shared);
+ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst, int shared);
 #ifdef VIMAGE_GLOBALS
 static int ether_ipfw;
 #endif
@@ -428,10 +427,9 @@ ether_output_frame(struct ifnet *ifp, struct mbuf *m)
 {
 #if defined(INET) || defined(INET6)
 	INIT_VNET_NET(ifp->if_vnet);
-	struct ip_fw *rule = ip_dn_claim_rule(m);
 
 	if (ip_fw_chk_ptr && V_ether_ipfw != 0) {
-		if (ether_ipfw_chk(&m, ifp, &rule, 0) == 0) {
+		if (ether_ipfw_chk(&m, ifp, 0) == 0) {
 			if (m) {
 				m_freem(m);
 				return EACCES;	/* pkt dropped */
@@ -455,8 +453,7 @@ ether_output_frame(struct ifnet *ifp, struct mbuf *m)
  * ether_output_frame.
  */
 int
-ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst,
-	struct ip_fw **rule, int shared)
+ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst, int shared)
 {
 	INIT_VNET_INET(dst->if_vnet);
 	struct ether_header *eh;
@@ -464,9 +461,19 @@ ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst,
 	struct mbuf *m;
 	int i;
 	struct ip_fw_args args;
+	struct dn_pkt_tag *dn_tag;
 
-	if (*rule != NULL && V_fw_one_pass)
-		return 1; /* dummynet packet, already partially processed */
+	dn_tag = ip_dn_claim_tag(*m0);
+
+	if (dn_tag != NULL) {
+		if (dn_tag->rule != NULL && V_fw_one_pass)
+			/* dummynet packet, already partially processed */
+			return (1);
+		args.rule = dn_tag->rule;	/* matching rule to restart */
+		args.rule_id = dn_tag->rule_id;
+		args.chain_id = dn_tag->chain_id;
+	} else
+		args.rule = NULL;
 
 	/*
 	 * I need some amt of data to be contiguous, and in case others need
@@ -487,7 +494,6 @@ ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst,
 
 	args.m = m;		/* the packet we are looking at		*/
 	args.oif = dst;		/* destination, if any			*/
-	args.rule = *rule;	/* matching rule to restart		*/
 	args.next_hop = NULL;	/* we do not support forward yet	*/
 	args.eh = &save_eh;	/* MAC header for bridged/MAC packets	*/
 	args.inp = NULL;	/* used by ipfw uid/gid/jail rules	*/
@@ -508,7 +514,6 @@ ether_ipfw_chk(struct mbuf **m0, struct ifnet *dst,
 				ETHER_HDR_LEN);
 	}
 	*m0 = m;
-	*rule = args.rule;
 
 	if (i == IP_FW_DENY) /* drop */
 		return 0;
@@ -765,9 +770,7 @@ ether_demux(struct ifnet *ifp, struct mbuf *m)
 	 * Do not do this for PROMISC frames in case we are re-entered.
 	 */
 	if (ip_fw_chk_ptr && V_ether_ipfw != 0 && !(m->m_flags & M_PROMISC)) {
-		struct ip_fw *rule = ip_dn_claim_rule(m);
-
-		if (ether_ipfw_chk(&m, NULL, &rule, 0) == 0) {
+		if (ether_ipfw_chk(&m, NULL, 0) == 0) {
 			if (m)
 				m_freem(m);	/* dropped; free mbuf chain */
 			return;			/* consumed */
diff --git a/sys/netgraph/ng_ipfw.c b/sys/netgraph/ng_ipfw.c
index cce623b..46bac8e 100644
--- a/sys/netgraph/ng_ipfw.c
+++ b/sys/netgraph/ng_ipfw.c
@@ -293,6 +293,8 @@ ng_ipfw_input(struct mbuf **m0, int dir, struct ip_fw_args *fwa, int tee)
 			return (ENOMEM);
 		}
 		ngit->rule = fwa->rule;
+		ngit->rule_id = fwa->rule_id;
+		ngit->chain_id = fwa->chain_id;
 		ngit->dir = dir;
 		ngit->ifp = fwa->oif;
 		m_tag_prepend(m, &ngit->mt);
diff --git a/sys/netgraph/ng_ipfw.h b/sys/netgraph/ng_ipfw.h
index 5448a38..29039f2 100644
--- a/sys/netgraph/ng_ipfw.h
+++ b/sys/netgraph/ng_ipfw.h
@@ -38,6 +38,8 @@ extern	ng_ipfw_input_t	*ng_ipfw_input_p;
 struct ng_ipfw_tag {
 	struct m_tag	mt;		/* tag header */
 	struct ip_fw	*rule;		/* matching rule */
+	uint32_t	rule_id;	/* matching rule id */
+	uint32_t	chain_id;	/* ruleset id */
 	struct ifnet	*ifp;		/* interface, for ip_output */
 	int		dir;
 #define	NG_IPFW_OUT	0
diff --git a/sys/netinet/ip_dummynet.h b/sys/netinet/ip_dummynet.h
index 118bdbb..5b6019d 100644
--- a/sys/netinet/ip_dummynet.h
+++ b/sys/netinet/ip_dummynet.h
@@ -113,6 +113,8 @@ struct dn_heap {
  */
 struct dn_pkt_tag {
     struct ip_fw *rule;		/* matching rule */
+    uint32_t rule_id;		/* matching rule id */
+    uint32_t chain_id;		/* ruleset id */
     int dn_dir;			/* action when packet comes out. */
 #define DN_TO_IP_OUT	1
 #define DN_TO_IP_IN	2
@@ -375,16 +377,16 @@ SLIST_HEAD(dn_pipe_head, dn_pipe);
 #ifdef _KERNEL
 
 /*
- * Return the IPFW rule associated with the dummynet tag; if any.
+ * Return the dummynet tag; if any.
  * Make sure that the dummynet tag is not reused by lower layers.
  */
-static __inline struct ip_fw *
-ip_dn_claim_rule(struct mbuf *m)
+static __inline struct dn_pkt_tag *
+ip_dn_claim_tag(struct mbuf *m)
 {
 	struct m_tag *mtag = m_tag_find(m, PACKET_TAG_DUMMYNET, NULL);
 	if (mtag != NULL) {
 		mtag->m_tag_id = PACKET_TAG_NONE;
-		return (((struct dn_pkt_tag *)(mtag+1))->rule);
+		return ((struct dn_pkt_tag *)(mtag + 1));
 	} else
 		return (NULL);
 }
diff --git a/sys/netinet/ip_fw.h b/sys/netinet/ip_fw.h
index 5602e9e..9e3468f 100644
--- a/sys/netinet/ip_fw.h
+++ b/sys/netinet/ip_fw.h
@@ -465,17 +465,18 @@ struct ip_fw {
 	struct ip_fw	*next_rule;	/* ptr to next [skipto] rule	*/
 	/* 'next_rule' is used to pass up 'set_disable' status		*/
 
-	u_int16_t	act_ofs;	/* offset of action in 32-bit units */
-	u_int16_t	cmd_len;	/* # of 32-bit words in cmd	*/
-	u_int16_t	rulenum;	/* rule number			*/
-	u_int8_t	set;		/* rule set (0..31)		*/
+	uint16_t	act_ofs;	/* offset of action in 32-bit units */
+	uint16_t	cmd_len;	/* # of 32-bit words in cmd	*/
+	uint16_t	rulenum;	/* rule number			*/
+	uint8_t	set;		/* rule set (0..31)		*/
 #define	RESVD_SET	31	/* set for default and persistent rules */
-	u_int8_t	_pad;		/* padding			*/
+	uint8_t		_pad;		/* padding			*/
+	uint32_t	id;		/* rule id */
 
 	/* These fields are present in all rules.			*/
-	u_int64_t	pcnt;		/* Packet counter		*/
-	u_int64_t	bcnt;		/* Byte counter			*/
-	u_int32_t	timestamp;	/* tv_sec of last match		*/
+	uint64_t	pcnt;		/* Packet counter		*/
+	uint64_t	bcnt;		/* Byte counter			*/
+	uint32_t	timestamp;	/* tv_sec of last match		*/
 
 	ipfw_insn	cmd[1];		/* storage for commands		*/
 };
@@ -619,10 +620,12 @@ struct ip_fw_args {
 	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 */
 	struct ether_header *eh;	/* for bridged packets		*/
 
 	struct ipfw_flow_id f_id;	/* grabbed from IP header	*/
-	u_int32_t	cookie;		/* a cookie depending on rule action */
+	uint32_t	cookie;		/* a cookie depending on rule action */
 	struct inpcb	*inp;
 
 	struct _ip6dn_args	dummypar; /* dummynet->ip6_output */
@@ -662,6 +665,7 @@ struct ip_fw_chain {
 	LIST_HEAD(, cfg_nat) nat;       /* list of nat entries */
 	struct radix_node_head *tables[IPFW_TABLES_MAX];
 	struct rwlock	rwmtx;
+	uint32_t	id;		/* ruleset id */
 };
 
 #ifdef IPFW_INTERNAL
diff --git a/sys/netinet/ipfw/ip_dummynet.c b/sys/netinet/ipfw/ip_dummynet.c
index 2f11ae0..24d949e 100644
--- a/sys/netinet/ipfw/ip_dummynet.c
+++ b/sys/netinet/ipfw/ip_dummynet.c
@@ -243,7 +243,6 @@ static void	dummynet_flush(void);
 static void	dummynet_send(struct mbuf *);
 void		dummynet_drain(void);
 static int	dummynet_io(struct mbuf **, int , struct ip_fw_args *);
-static void	dn_rule_delete(void *);
 
 /*
  * Heap management functions.
@@ -1399,6 +1398,8 @@ dummynet_io(struct mbuf **m0, int dir, struct ip_fw_args *fwa)
 	 * Build and enqueue packet + parameters.
 	 */
 	pkt->rule = fwa->rule;
+	pkt->rule_id = fwa->rule_id;
+	pkt->chain_id = fwa->chain_id;
 	pkt->dn_dir = dir;
 
 	pkt->ifp = fwa->oif;
@@ -1622,60 +1623,6 @@ dummynet_flush(void)
 	DUMMYNET_UNLOCK();
 }
 
-extern struct ip_fw *ip_fw_default_rule ;
-static void
-dn_rule_delete_fs(struct dn_flow_set *fs, void *r)
-{
-    int i ;
-    struct dn_flow_queue *q ;
-    struct mbuf *m ;
-
-    for (i = 0 ; i <= fs->rq_size ; i++) /* last one is ovflow */
-	for (q = fs->rq[i] ; q ; q = q->next )
-	    for (m = q->head ; m ; m = m->m_nextpkt ) {
-		struct dn_pkt_tag *pkt = dn_tag_get(m) ;
-		if (pkt->rule == r)
-		    pkt->rule = ip_fw_default_rule ;
-	    }
-}
-
-/*
- * When a firewall rule is deleted, scan all queues and remove the pointer
- * to the rule from matching packets, making them point to the default rule.
- * The pointer is used to reinject packets in case one_pass = 0.
- */
-void
-dn_rule_delete(void *r)
-{
-    struct dn_pipe *pipe;
-    struct dn_flow_set *fs;
-    struct dn_pkt_tag *pkt;
-    struct mbuf *m;
-    int i;
-
-    DUMMYNET_LOCK();
-    /*
-     * If the rule references a queue (dn_flow_set), then scan
-     * the flow set, otherwise scan pipes. Should do either, but doing
-     * both does not harm.
-     */
-    for (i = 0; i < HASHSIZE; i++)
-	SLIST_FOREACH(fs, &flowsethash[i], next)
-		dn_rule_delete_fs(fs, r);
-
-    for (i = 0; i < HASHSIZE; i++)
-	SLIST_FOREACH(pipe, &pipehash[i], next) {
-		fs = &(pipe->fs);
-		dn_rule_delete_fs(fs, r);
-		for (m = pipe->head ; m ; m = m->m_nextpkt ) {
-			pkt = dn_tag_get(m);
-			if (pkt->rule == r)
-				pkt->rule = ip_fw_default_rule;
-		}
-	}
-    DUMMYNET_UNLOCK();
-}
-
 /*
  * setup RED parameters
  */
@@ -2299,7 +2246,6 @@ ip_dn_init(void)
 
 	ip_dn_ctl_ptr = ip_dn_ctl;
 	ip_dn_io_ptr = dummynet_io;
-	ip_dn_ruledel_ptr = dn_rule_delete;
 
 	TASK_INIT(&dn_task, 0, dummynet_task, NULL);
 	dn_tq = taskqueue_create_fast("dummynet", M_NOWAIT,
@@ -2319,7 +2265,6 @@ ip_dn_destroy(void)
 {
 	ip_dn_ctl_ptr = NULL;
 	ip_dn_io_ptr = NULL;
-	ip_dn_ruledel_ptr = NULL;
 
 	DUMMYNET_LOCK();
 	callout_stop(&dn_timeout);
diff --git a/sys/netinet/ipfw/ip_fw2.c b/sys/netinet/ipfw/ip_fw2.c
index 0a45658..873a184 100644
--- a/sys/netinet/ipfw/ip_fw2.c
+++ b/sys/netinet/ipfw/ip_fw2.c
@@ -132,6 +132,8 @@ static int default_to_accept;
 #endif
 static uma_zone_t ipfw_dyn_rule_zone;
 
+struct ip_fw *ip_fw_default_rule;
+
 /*
  * Data structure to cache our ucred related
  * information. This structure only gets used if
@@ -2520,9 +2522,21 @@ do {									\
 	if (args->rule) {
 		/*
 		 * Packet has already been tagged. Look for the next rule
-		 * to restart processing.
+		 * to restart processing. Make sure that args->rule still
+		 * exists and not changed.
 		 */
-		f = args->rule->next_rule;
+		if (chain->id != args->chain_id) {
+			for (f = chain->rules; f != NULL; f = f->next)
+				if (f == args->rule && f->id == args->rule_id)
+					break;
+
+			if (f != NULL)
+				f = f->next_rule;
+			else
+				f = ip_fw_default_rule;
+		} else 
+			f = args->rule->next_rule;
+
 		if (f == NULL)
 			f = lookup_next_rule(args->rule, 0);
 	} else {
@@ -3234,6 +3248,8 @@ check_body:
 			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
@@ -3342,6 +3358,8 @@ check_body:
 			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
@@ -3364,6 +3382,8 @@ check_body:
 
  				if (IPFW_NAT_LOADED) {
 					args->rule = f; /* Report matching rule. */
+					args->rule_id = f->id;
+					args->chain_id = chain->id;
 					t = ((ipfw_insn_nat *)cmd)->nat;
 					if (t == NULL) {
 						nat_id = (cmd->arg1 == IP_FW_TABLEARG) ?
@@ -3422,6 +3442,8 @@ check_body:
 							ip->ip_sum = in_cksum(m, hlen);
 						retval = IP_FW_REASS;
 						args->rule = f;
+						args->rule_id = f->id;
+						args->chain_id = chain->id;
 						goto done;
 					} else {
 						retval = IP_FW_DENY;
@@ -3480,6 +3502,8 @@ flush_rule_ptrs(struct ip_fw_chain *chain)
 
 	IPFW_WLOCK_ASSERT(chain);
 
+	chain->id++;
+
 	for (rule = chain->rules; rule; rule = rule->next)
 		rule->next_rule = NULL;
 }
@@ -3516,6 +3540,7 @@ add_rule(struct ip_fw_chain *chain, struct ip_fw *input_rule)
 
 	if (chain->rules == NULL) {	/* default rule */
 		chain->rules = rule;
+		rule->id = ++chain->id;
 		goto done;
         }
 
@@ -3557,6 +3582,8 @@ add_rule(struct ip_fw_chain *chain, struct ip_fw *input_rule)
 		}
 	}
 	flush_rule_ptrs(chain);
+	/* chain->id incremented inside flush_rule_ptrs() */
+	rule->id = chain->id;
 done:
 	V_static_count++;
 	V_static_len += l;
@@ -3602,12 +3629,6 @@ remove_rule(struct ip_fw_chain *chain, struct ip_fw *rule,
 }
 
 /*
- * Hook for cleaning up dummynet when an ipfw rule is deleted.
- * Set/cleared when dummynet module is loaded/unloaded.
- */
-void   (*ip_dn_ruledel_ptr)(void *) = NULL;
-
-/**
  * Reclaim storage associated with a list of rules.  This is
  * typically the list created using remove_rule.
  */
@@ -3618,8 +3639,6 @@ reap_rules(struct ip_fw *head)
 
 	while ((rule = head) != NULL) {
 		head = head->next;
-		if (ip_dn_ruledel_ptr)
-			ip_dn_ruledel_ptr(rule);
 		free(rule, M_IPFW);
 	}
 }
@@ -4521,15 +4540,6 @@ ipfw_ctl(struct sockopt *sopt)
 #undef RULE_MAXSIZE
 }
 
-/**
- * dummynet needs a reference to the default rule, because rules can be
- * deleted while packets hold a reference to them. When this happens,
- * dummynet changes the reference to the default rule (it could well be a
- * NULL pointer, but this way we do not need to check for the special
- * case, plus here he have info on the default behaviour).
- */
-struct ip_fw *ip_fw_default_rule;
-
 /*
  * This procedure is only used to handle keepalives. It is invoked
  * every dyn_keepalive_period
diff --git a/sys/netinet/ipfw/ip_fw_pfil.c b/sys/netinet/ipfw/ip_fw_pfil.c
index 0b1ba2d..5fd6a05 100644
--- a/sys/netinet/ipfw/ip_fw_pfil.c
+++ b/sys/netinet/ipfw/ip_fw_pfil.c
@@ -113,6 +113,8 @@ ipfw_check_in(void *arg, struct mbuf **m0, struct ifnet *ifp, int dir,
 		KASSERT(ng_tag->dir == NG_IPFW_IN,
 		    ("ng_ipfw tag with wrong direction"));
 		args.rule = ng_tag->rule;
+		args.rule_id = ng_tag->rule_id;
+		args.chain_id = ng_tag->chain_id;
 		m_tag_delete(*m0, (struct m_tag *)ng_tag);
 	}
 
@@ -123,6 +125,8 @@ again:
 
 		dt = (struct dn_pkt_tag *)(dn_tag+1);
 		args.rule = dt->rule;
+		args.rule_id = dt->rule_id;
+		args.chain_id = dt->chain_id;
 
 		m_tag_delete(*m0, dn_tag);
 	}
@@ -243,6 +247,8 @@ ipfw_check_out(void *arg, struct mbuf **m0, struct ifnet *ifp, int dir,
 		KASSERT(ng_tag->dir == NG_IPFW_OUT,
 		    ("ng_ipfw tag with wrong direction"));
 		args.rule = ng_tag->rule;
+		args.rule_id = ng_tag->rule_id;
+		args.chain_id = ng_tag->chain_id;
 		m_tag_delete(*m0, (struct m_tag *)ng_tag);
 	}
 
@@ -253,6 +259,8 @@ again:
 
 		dt = (struct dn_pkt_tag *)(dn_tag+1);
 		args.rule = dt->rule;
+		args.rule_id = dt->rule_id;
+		args.chain_id = dt->chain_id;
 
 		m_tag_delete(*m0, dn_tag);
 	}