Skip site navigation (1)Skip section navigation (2) 
Date:      Thu, 5 Feb 2015 13:49:05 +0000 (UTC)
From:      "Alexander V. Chernikov" <melifaro@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r278259 - head/sys/netpfil/ipfw
Message-ID:  <201502051349.t15Dn58r032016@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: melifaro
Date: Thu Feb  5 13:49:04 2015
New Revision: 278259
URL: https://svnweb.freebsd.org/changeset/base/278259

Log:
  * 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

Modified:
  head/sys/netpfil/ipfw/ip_fw_iface.c   (contents, props changed)
  head/sys/netpfil/ipfw/ip_fw_private.h
  head/sys/netpfil/ipfw/ip_fw_table.c
  head/sys/netpfil/ipfw/ip_fw_table_algo.c

Modified: head/sys/netpfil/ipfw/ip_fw_iface.c
==============================================================================
--- head/sys/netpfil/ipfw/ip_fw_iface.c	Thu Feb  5 13:07:41 2015	(r278258)
+++ head/sys/netpfil/ipfw/ip_fw_iface.c	Thu Feb  5 13:49:04 2015	(r278259)
@@ -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
 
 /*
  * 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);
 }
 
 /*

Modified: head/sys/netpfil/ipfw/ip_fw_private.h
==============================================================================
--- head/sys/netpfil/ipfw/ip_fw_private.h	Thu Feb  5 13:07:41 2015	(r278258)
+++ head/sys/netpfil/ipfw/ip_fw_private.h	Thu Feb  5 13:49:04 2015	(r278259)
@@ -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)

Modified: head/sys/netpfil/ipfw/ip_fw_table.c
==============================================================================
--- head/sys/netpfil/ipfw/ip_fw_table.c	Thu Feb  5 13:07:41 2015	(r278258)
+++ head/sys/netpfil/ipfw/ip_fw_table.c	Thu Feb  5 13:49:04 2015	(r278259)
@@ -1198,7 +1198,7 @@ flush_table(struct ip_fw_chain *ch, stru
 	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, stru
 		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));
@@ -1237,6 +1240,14 @@ restart:
 	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.
 	 */
 	memset(&ti_new, 0, sizeof(struct table_info));
@@ -1262,7 +1273,8 @@ restart:
 	 * 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_instan
 {
 
 	KASSERT(tc->linked == 0, ("free() on linked config"));
+	/* UH lock MUST NOT be held */
 
 	/*
 	 * We're using ta without any locking/referencing.

Modified: head/sys/netpfil/ipfw/ip_fw_table_algo.c
==============================================================================
--- head/sys/netpfil/ipfw/ip_fw_table_algo.c	Thu Feb  5 13:07:41 2015	(r278258)
+++ head/sys/netpfil/ipfw/ip_fw_table_algo.c	Thu Feb  5 13:49:04 2015	(r278259)
@@ -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_ins
 	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 
 	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 tabl
 
 	/* 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 
 
 	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);
-	}
 }

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201502051349.t15Dn58r032016>