* Make sure table algorithm destroy hook is always called without locks

* Explicitly lock freeing interface references in ta_destroy_ifidx
* Change ipfw_iface_unref() to require UH lock
* Add forgotten ipfw_iface_unref() to destroy_ifidx_locked()

PR:		kern/197276
Submitted by:	lev
Sponsored by:	Yandex LLC


git-svn-id: svn+ssh://svn.freebsd.org/base/head@278259 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

sys/netpfil/ipfw/ip_fw_iface.c

@@ -24,7 +24,7 @@
  */
 
 #include <sys/cdefs.h>
-__FBSDID("$FreeBSD: projects/ipfw/sys/netpfil/ipfw/ip_fw_iface.c 267384 2014-06-12 09:59:11Z melifaro $");
+__FBSDID("$FreeBSD$");
 
 /*
  * Kernel interface tracking API.
@@ -397,20 +397,20 @@ ipfw_iface_del_notify(struct ip_fw_chain *ch, struct ipfw_ifc *ic)
 
 /*
  * Unreference interface specified by @ic.
- * Must be called without holding any locks.
+ * Must be called while holding UH lock.
  */
 void
 ipfw_iface_unref(struct ip_fw_chain *ch, struct ipfw_ifc *ic)
 {
 	struct ipfw_iface *iif;
 
+	IPFW_UH_WLOCK_ASSERT(ch);
+
 	iif = ic->iface;
 	ic->iface = NULL;
 
-	IPFW_UH_WLOCK(ch);
 	iif->no.refcnt--;
 	/* TODO: check for references & delete */
-	IPFW_UH_WUNLOCK(ch);
 }
 
 /*

sys/netpfil/ipfw/ip_fw_private.h

@@ -429,6 +429,7 @@ struct ipfw_ifc {
 
 #define	IPFW_UH_RLOCK_ASSERT(_chain)	rw_assert(&(_chain)->uh_lock, RA_RLOCKED)
 #define	IPFW_UH_WLOCK_ASSERT(_chain)	rw_assert(&(_chain)->uh_lock, RA_WLOCKED)
+#define	IPFW_UH_UNLOCK_ASSERT(_chain)	rw_assert(&(_chain)->uh_lock, RA_UNLOCKED)
 
 #define IPFW_UH_RLOCK(p) rw_rlock(&(p)->uh_lock)
 #define IPFW_UH_RUNLOCK(p) rw_runlock(&(p)->uh_lock)

sys/netpfil/ipfw/ip_fw_table.c

@@ -1198,7 +1198,7 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	void *astate_old, *astate_new;
 	char algostate[64], *pstate;
 	struct tableop_state ts;
-	int error;
+	int error, need_gc;
 	uint16_t kidx;
 	uint8_t tflags;
 
@@ -1212,6 +1212,9 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 		IPFW_UH_WUNLOCK(ch);
 		return (ESRCH);
 	}
+	need_gc = 0;
+	astate_new = NULL;
+	memset(&ti_new, 0, sizeof(ti_new));
 restart:
 	/* Set up swap handler */
 	memset(&ts, 0, sizeof(ts));
@@ -1236,6 +1239,14 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	add_toperation_state(ch, &ts);
 	IPFW_UH_WUNLOCK(ch);
 
+	/*
+	 * Stage 1.5: if this is not the first attempt, destroy previous state
+	 */
+	if (need_gc != 0) {
+		ta->destroy(astate_new, &ti_new);
+		need_gc = 0;
+	}
+
 	/*
 	 * Stage 2: allocate new table instance using same algo.
 	 */
@@ -1262,7 +1273,8 @@ flush_table(struct ip_fw_chain *ch, struct tid_info *ti)
 	 * complex checks.
 	 */
 	if (ts.modified != 0) {
-		ta->destroy(astate_new, &ti_new);
+		/* Delay destroying data since we're holding UH lock */
+		need_gc = 1;
 		goto restart;
 	}
 
@@ -3042,6 +3054,7 @@ free_table_config(struct namedobj_instance *ni, struct table_config *tc)
 {
 
 	KASSERT(tc->linked == 0, ("free() on linked config"));
+	/* UH lock MUST NOT be held */
 
 	/*
 	 * We're using ta without any locking/referencing.

sys/netpfil/ipfw/ip_fw_table_algo.c

@@ -97,7 +97,7 @@ __FBSDID("$FreeBSD$");
  *
  * -destroy: request to destroy table instance.
  * typedef void (ta_destroy)(void *ta_state, struct table_info *ti);
- * MANDATORY, may be locked (UH+WLOCK). (M_NOWAIT).
+ * MANDATORY, unlocked. (M_WAITOK).
  *
  * Frees all table entries and all tables structures allocated by -init.
  *
@@ -2134,6 +2134,7 @@ destroy_ifidx_locked(struct namedobj_instance *ii, struct named_object *no,
 	ife = (struct ifentry *)no;
 
 	ipfw_iface_del_notify(ch, &ife->ic);
+	ipfw_iface_unref(ch, &ife->ic);
 	free(ife, M_IPFW_TBL);
 }
 
@@ -2153,7 +2154,9 @@ ta_destroy_ifidx(void *ta_state, struct table_info *ti)
 	if (icfg->main_ptr != NULL)
 		free(icfg->main_ptr, M_IPFW);
 
+	IPFW_UH_WLOCK(ch);
 	ipfw_objhash_foreach(icfg->ii, destroy_ifidx_locked, ch);
+	IPFW_UH_WUNLOCK(ch);
 
 	ipfw_objhash_destroy(icfg->ii);
 
@@ -2333,8 +2336,9 @@ ta_del_ifidx(void *ta_state, struct table_info *ti, struct tentry_info *tei,
 
 	/* Unlink from local list */
 	ipfw_objhash_del(icfg->ii, &ife->no);
-	/* Unlink notifier */
+	/* Unlink notifier and deref */
 	ipfw_iface_del_notify(icfg->ch, &ife->ic);
+	ipfw_iface_unref(icfg->ch, &ife->ic);
 
 	icfg->count--;
 	tei->value = ife->value;
@@ -2357,11 +2361,8 @@ ta_flush_ifidx_entry(struct ip_fw_chain *ch, struct tentry_info *tei,
 
 	tb = (struct ta_buf_ifidx *)ta_buf;
 
-	if (tb->ife != NULL) {
-		/* Unlink first */
-		ipfw_iface_unref(ch, &tb->ife->ic);
+	if (tb->ife != NULL)
 		free(tb->ife, M_IPFW_TBL);
-	}
 }