From d1eabbf235ee13a4b2e38637baa7e31e3a637856 Mon Sep 17 00:00:00 2001 From: truckman Date: Fri, 8 Jul 2016 02:52:39 +0000 Subject: [PATCH] MFC r302338 Fix a race condition between the main thread in aqm_pie_cleanup() and the callout thread that can cause a kernel panic. Always do the final cleanup in the callout thread by passing a separate callout function for that task to callout_reset_sbt(). Protect the ref_count decrement in the callout with DN_BH_WLOCK(). All other ref_count manipulation is protected with this lock. There is still a tiny window between ref_count reaching zero and the end of the callout function where it is unsafe to unload the module. Fixing this would require the use of callout_drain(), but this can't be done because dummynet holds a mutex and callout_drain() might sleep. Remove the callout_pending(), callout_active(), and callout_deactivate() calls from calculate_drop_prob(). They are not needed because this callout uses callout_init_mtx(). Submitted by: Rasool Al-Saadi Differential Revision: https://reviews.freebsd.org/D6928 git-svn-id: svn://svn.freebsd.org/base/stable/10@302422 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f --- sys/netpfil/ipfw/dn_aqm_pie.c | 61 ++++++++++++++++------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/sys/netpfil/ipfw/dn_aqm_pie.c b/sys/netpfil/ipfw/dn_aqm_pie.c index c4b9401d0..c2e4d43ca 100644 --- a/sys/netpfil/ipfw/dn_aqm_pie.c +++ b/sys/netpfil/ipfw/dn_aqm_pie.c @@ -207,24 +207,6 @@ calculate_drop_prob(void *x) struct dn_aqm_pie_parms *pprms; struct pie_status *pst = (struct pie_status *) x; - /* dealing with race condition */ - if (callout_pending(&pst->aqm_pie_callout)) { - /* callout was reset */ - mtx_unlock(&pst->lock_mtx); - return; - } - - if (!callout_active(&pst->aqm_pie_callout)) { - /* callout was stopped */ - mtx_unlock(&pst->lock_mtx); - mtx_destroy(&pst->lock_mtx); - free(x, M_DUMMYNET); - //pst->pq->aqm_status = NULL; - pie_desc.ref_count--; - return; - } - callout_deactivate(&pst->aqm_pie_callout); - pprms = pst->parms; prob = pst->drop_prob; @@ -576,7 +558,7 @@ aqm_pie_init(struct dn_queue *q) do { /* exit with break when error occurs*/ if (!pprms){ - D("AQM_PIE is not configured"); + DX(2, "AQM_PIE is not configured"); err = EINVAL; break; } @@ -614,6 +596,22 @@ aqm_pie_init(struct dn_queue *q) return err; } +/* + * Callout function to destroy pie mtx and free PIE status memory + */ +static void +pie_callout_cleanup(void *x) +{ + struct pie_status *pst = (struct pie_status *) x; + + mtx_unlock(&pst->lock_mtx); + mtx_destroy(&pst->lock_mtx); + free(x, M_DUMMYNET); + DN_BH_WLOCK(); + pie_desc.ref_count--; + DN_BH_WUNLOCK(); +} + /* * Clean up PIE status for queue 'q' * Destroy memory allocated for PIE status. @@ -640,22 +638,19 @@ aqm_pie_cleanup(struct dn_queue *q) return 1; } + /* + * Free PIE status allocated memory using pie_callout_cleanup() callout + * function to avoid any potential race. + * We reset aqm_pie_callout to call pie_callout_cleanup() in next 1um. This + * stops the scheduled calculate_drop_prob() callout and call pie_callout_cleanup() + * which does memory freeing. + */ mtx_lock(&pst->lock_mtx); + callout_reset_sbt(&pst->aqm_pie_callout, + SBT_1US, 0, pie_callout_cleanup, pst, 0); + q->aqm_status = NULL; + mtx_unlock(&pst->lock_mtx); - /* stop callout timer */ - if (callout_stop(&pst->aqm_pie_callout) || !(pst->sflags & PIE_ACTIVE)) { - mtx_unlock(&pst->lock_mtx); - mtx_destroy(&pst->lock_mtx); - free(q->aqm_status, M_DUMMYNET); - q->aqm_status = NULL; - pie_desc.ref_count--; - return 0; - } else { - q->aqm_status = NULL; - mtx_unlock(&pst->lock_mtx); - DX(2, "PIE callout has not been stoped from cleanup!"); - return EBUSY; - } return 0; } -- 2.45.0