From e779c79b7575e6147883a199f9249072d95632ff Mon Sep 17 00:00:00 2001 From: melifaro Date: Thu, 5 Feb 2015 13:49:04 +0000 Subject: [PATCH] * 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 | 8 ++++---- sys/netpfil/ipfw/ip_fw_private.h | 1 + sys/netpfil/ipfw/ip_fw_table.c | 17 +++++++++++++++-- sys/netpfil/ipfw/ip_fw_table_algo.c | 13 +++++++------ 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/sys/netpfil/ipfw/ip_fw_iface.c b/sys/netpfil/ipfw/ip_fw_iface.c index 7e9c992032da6c..b7c450c6dc4654 100644 --- a/sys/netpfil/ipfw/ip_fw_iface.c +++ b/sys/netpfil/ipfw/ip_fw_iface.c @@ -24,7 +24,7 @@ */ #include -__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); } /* diff --git a/sys/netpfil/ipfw/ip_fw_private.h b/sys/netpfil/ipfw/ip_fw_private.h index ddb73e7f155ee0..3f46ddd48f6c1e 100644 --- a/sys/netpfil/ipfw/ip_fw_private.h +++ b/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) diff --git a/sys/netpfil/ipfw/ip_fw_table.c b/sys/netpfil/ipfw/ip_fw_table.c index 5dc8ddab0e2517..4498ace88d40c3 100644 --- a/sys/netpfil/ipfw/ip_fw_table.c +++ b/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. diff --git a/sys/netpfil/ipfw/ip_fw_table_algo.c b/sys/netpfil/ipfw/ip_fw_table_algo.c index 50ef305bcf62e6..06a46410813ab7 100644 --- a/sys/netpfil/ipfw/ip_fw_table_algo.c +++ b/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); - } }