From fb4b37a357c6b5aa569be76abc4c57db5f72d76b Mon Sep 17 00:00:00 2001
From: "Alexander V. Chernikov" <melifaro@FreeBSD.org>
Date: Tue, 2 Sep 2014 20:46:18 +0000
Subject: [PATCH] * Fix crash due to forgotten value refcouting in
 ipfw_link_table_values() * Fix argument order in rollback_toperation_state()
 * Make flush_table() use operation state API to ease checks.

---
 sys/netpfil/ipfw/ip_fw_table.c       | 56 +++++++++++++++++++++++-----
 sys/netpfil/ipfw/ip_fw_table_value.c |  3 +-
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/sys/netpfil/ipfw/ip_fw_table.c b/sys/netpfil/ipfw/ip_fw_table.c
index 74a31e05bfcc..51f64496ef6f 100644
--- a/sys/netpfil/ipfw/ip_fw_table.c
+++ b/sys/netpfil/ipfw/ip_fw_table.c
@@ -140,7 +140,7 @@ rollback_toperation_state(struct ip_fw_chain *ch, void *object)
 
 	tcfg = CHAIN_TO_TCFG(ch);
 	TAILQ_FOREACH(os, &tcfg->state_list, next)
-		os->func(os, object);
+		os->func(object, os);
 }
 
 void
@@ -576,6 +576,8 @@ add_table_entry(struct ip_fw_chain *ch, struct tid_info *ti,
 		return (error);
 	}
 	ta = tc->ta;
+
+	/* Fill in tablestate */
 	ts.ch = ch;
 	ts.opstate.func = rollback_add_entry;
 	ts.tc = tc;
@@ -1185,12 +1187,26 @@ ipfw_flush_table(struct ip_fw_chain *ch, ip_fw3_opheader *op3,
 	return (error);
 }
 
+static void
+restart_flush(void *object, struct op_state *_state)
+{
+	struct tableop_state *ts;
+
+	ts = (struct tableop_state *)_state;
+
+	if (ts->tc != object)
+		return;
+
+	/* Indicate we've called */
+	ts->modified = 1;
+}
+
 /*
  * Flushes given table.
  *
  * Function create new table instance with the same
  * parameters, swaps it with old one and
- * flushes state without holding any locks.
+ * flushes state without holding runtime WLOCK.
  *
  * Returns 0 on success.
  */
@@ -1203,6 +1219,7 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	struct table_info ti_old, ti_new, *tablestate;
 	void *astate_old, *astate_new;
 	char algostate[64], *pstate;
+	struct tableop_state ts;
 	int error;
 	uint16_t kidx;
 	uint8_t tflags;
@@ -1217,13 +1234,18 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 		IPFW_UH_WUNLOCK(ch);
 		return (ESRCH);
 	}
+restart:
+	/* Set up swap handler */
+	memset(&ts, 0, sizeof(ts));
+	ts.opstate.func = restart_flush;
+	ts.tc = tc;
+
 	ta = tc->ta;
 	/* Do not flush readonly tables */
 	if ((ta->flags & TA_FLAG_READONLY) != 0) {
 		IPFW_UH_WUNLOCK(ch);
 		return (EACCES);
 	}
-	tc->no.refcnt++;
 	/* Save startup algo parameters */
 	if (ta->print_config != NULL) {
 		ta->print_config(tc->astate, KIDX_TO_TI(ch, tc->no.kidx),
@@ -1232,24 +1254,39 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	} else
 		pstate = NULL;
 	tflags = tc->tflags;
+	tc->no.refcnt++;
+	add_toperation_state(ch, &ts);
 	IPFW_UH_WUNLOCK(ch);
 
 	/*
 	 * Stage 2: allocate new table instance using same algo.
 	 */
 	memset(&ti_new, 0, sizeof(struct table_info));
-	if ((error = ta->init(ch, &astate_new, &ti_new, pstate, tflags)) != 0) {
-		IPFW_UH_WLOCK(ch);
-		tc->no.refcnt--;
-		IPFW_UH_WUNLOCK(ch);
-		return (error);
-	}
+	error = ta->init(ch, &astate_new, &ti_new, pstate, tflags);
 
 	/*
 	 * Stage 3: swap old state pointers with newly-allocated ones.
 	 * Decrease refcount.
 	 */
 	IPFW_UH_WLOCK(ch);
+	tc->no.refcnt--;
+	del_toperation_state(ch, &ts);
+
+	if (error != 0) {
+		IPFW_UH_WUNLOCK(ch);
+		return (error);
+	}
+
+	/*
+	 * Restart operation if table swap has happened:
+	 * even if algo may be the same, algo init parameters
+	 * may change. Restart operation instead of doing
+	 * complex checks.
+	 */
+	if (ts.modified != 0) {
+		ta->destroy(astate_new, &ti_new);
+		goto restart;
+	}
 
 	ni = CHAIN_TO_NI(ch);
 	kidx = tc->no.kidx;
@@ -1264,7 +1301,6 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	tc->astate = astate_new;
 	tc->ti = ti_new;
 	tc->count = 0;
-	tc->no.refcnt--;
 
 	/*
 	 * Stage 4: unref values.
diff --git a/sys/netpfil/ipfw/ip_fw_table_value.c b/sys/netpfil/ipfw/ip_fw_table_value.c
index c458629c27c5..a8d4a31b65fc 100644
--- a/sys/netpfil/ipfw/ip_fw_table_value.c
+++ b/sys/netpfil/ipfw/ip_fw_table_value.c
@@ -531,8 +531,8 @@ ipfw_link_table_values(struct ip_fw_chain *ch, struct tableop_state *ts)
 
 		/* Value found. Bump refcount */
 		ptv->pval->refcnt++;
-		found++;
 		ptei->value = ptv->no.kidx;
+		found++;
 	}
 
 	if (ts->count == found) {
@@ -585,6 +585,7 @@ ipfw_link_table_values(struct ip_fw_chain *ch, struct tableop_state *ts)
 		ptv = (struct table_val_link *)ipfw_objhash_lookup_name(vi, 0,
 		    (char *)&tval);
 		if (ptv != NULL) {
+			ptv->pval->refcnt++;
 			ptei->value = ptv->no.kidx;
 			continue;
 		}