Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 8 Jul 2016 02:52:39 +0000 (UTC)
From:      Don Lewis <truckman@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org
Subject:   svn commit: r302422 - stable/10/sys/netpfil/ipfw
Message-ID:  <201607080252.u682qdx7059411@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: truckman
Date: Fri Jul  8 02:52:39 2016
New Revision: 302422
URL: https://svnweb.freebsd.org/changeset/base/302422

Log:
  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 <ralsaadi@swin.edu.au>
  Differential Revision:	https://reviews.freebsd.org/D6928

Modified:
  stable/10/sys/netpfil/ipfw/dn_aqm_pie.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/netpfil/ipfw/dn_aqm_pie.c
==============================================================================
--- stable/10/sys/netpfil/ipfw/dn_aqm_pie.c	Fri Jul  8 02:34:04 2016	(r302421)
+++ stable/10/sys/netpfil/ipfw/dn_aqm_pie.c	Fri Jul  8 02:52:39 2016	(r302422)
@@ -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;
 		}
@@ -615,6 +597,22 @@ aqm_pie_init(struct dn_queue *q)
 }
 
 /* 
+ * 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;
 }

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