svn commit: r315741 - in head/sys: net netpfil/pf

[Kristof Provost]

Author: kp
Date: Wed Mar 22 21:18:18 2017
New Revision: 315741
URL: https://svnweb.freebsd.org/changeset/base/315741

Log:
  pf: Fix possible shutdown race
  
  Prevent possible races in the pf_unload() / pf_purge_thread() shutdown
  code. Lock the pf_purge_thread() with the new pf_end_lock to prevent
  these races.
  
  Use a shared/exclusive lock, as we need to also acquire another sx lock
  (VNET_LIST_RLOCK). It's fine for both pf_purge_thread() and pf_unload()
  to sleep,
  
  Pointed out by: eri, glebius, jhb
  Differential Revision:	https://reviews.freebsd.org/D10026

Modified:
  head/sys/net/pfvar.h
  head/sys/netpfil/pf/pf.c
  head/sys/netpfil/pf/pf_ioctl.c

Modified: head/sys/net/pfvar.h
==============================================================================
--- head/sys/net/pfvar.h	Wed Mar 22 20:51:52 2017	(r315740)
+++ head/sys/net/pfvar.h	Wed Mar 22 21:18:18 2017	(r315741)
@@ -154,6 +154,8 @@ extern struct rwlock pf_rules_lock;
 #define	PF_RULES_RASSERT()	rw_assert(&pf_rules_lock, RA_RLOCKED)
 #define	PF_RULES_WASSERT()	rw_assert(&pf_rules_lock, RA_WLOCKED)
 
+extern struct sx pf_end_lock;
+
 #define	PF_MODVER	1
 #define	PFLOG_MODVER	1
 #define	PFSYNC_MODVER	1

Modified: head/sys/netpfil/pf/pf.c
==============================================================================
--- head/sys/netpfil/pf/pf.c	Wed Mar 22 20:51:52 2017	(r315740)
+++ head/sys/netpfil/pf/pf.c	Wed Mar 22 21:18:18 2017	(r315741)
@@ -302,6 +302,7 @@ static void		 pf_route6(struct mbuf **, 
 int in4_cksum(struct mbuf *m, u_int8_t nxt, int off, int len);
 
 extern int pf_end_threads;
+extern struct proc *pf_purge_proc;
 
 VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]);
 
@@ -1428,19 +1429,14 @@ pf_purge_thread(void *unused __unused)
 	VNET_ITERATOR_DECL(vnet_iter);
 	u_int idx = 0;
 
-	for (;;) {
-		tsleep(pf_purge_thread, 0, "pftm", hz / 10);
+	sx_xlock(&pf_end_lock);
+	while (pf_end_threads == 0) {
+		sx_sleep(pf_purge_thread, &pf_end_lock, 0, "pftm", hz / 10);
 
 		VNET_LIST_RLOCK();
 		VNET_FOREACH(vnet_iter) {
 			CURVNET_SET(vnet_iter);
 
-			if (pf_end_threads) {
-				pf_end_threads++;
-				wakeup(pf_purge_thread);
-				VNET_LIST_RUNLOCK();
-				kproc_exit(0);
-			}
 
 			/* Wait until V_pf_default_rule is initialized. */
 			if (V_pf_vnet_active == 0) {
@@ -1474,7 +1470,10 @@ pf_purge_thread(void *unused __unused)
 		}
 		VNET_LIST_RUNLOCK();
 	}
-	/* not reached */
+
+	pf_end_threads++;
+	sx_xunlock(&pf_end_lock);
+	kproc_exit(0);
 }
 
 void

Modified: head/sys/netpfil/pf/pf_ioctl.c
==============================================================================
--- head/sys/netpfil/pf/pf_ioctl.c	Wed Mar 22 20:51:52 2017	(r315740)
+++ head/sys/netpfil/pf/pf_ioctl.c	Wed Mar 22 21:18:18 2017	(r315741)
@@ -198,9 +198,11 @@ VNET_DEFINE(int, pf_vnet_active);
 #define V_pf_vnet_active	VNET(pf_vnet_active)
 
 int pf_end_threads;
+struct proc *pf_purge_proc;
 
 struct rwlock			pf_rules_lock;
 struct sx			pf_ioctl_lock;
+struct sx			pf_end_lock;
 
 /* pfsync */
 pfsync_state_import_t 		*pfsync_state_import_ptr = NULL;
@@ -3730,6 +3732,7 @@ pf_load(void)
 
 	rw_init(&pf_rules_lock, "pf rulesets");
 	sx_init(&pf_ioctl_lock, "pf ioctl");
+	sx_init(&pf_end_lock, "pf end thread");
 
 	pf_mtag_initialize();
 
@@ -3738,7 +3741,7 @@ pf_load(void)
 		return (ENOMEM);
 
 	pf_end_threads = 0;
-	error = kproc_create(pf_purge_thread, NULL, NULL, 0, 0, "pf purge");
+	error = kproc_create(pf_purge_thread, NULL, &pf_purge_proc, 0, 0, "pf purge");
 	if (error != 0)
 		return (error);
 
@@ -3788,11 +3791,13 @@ pf_unload(void)
 {
 	int error = 0;
 
+	sx_xlock(&pf_end_lock);
 	pf_end_threads = 1;
 	while (pf_end_threads < 2) {
 		wakeup_one(pf_purge_thread);
-		tsleep(pf_purge_thread, 0, "pftmo", 0);
+		sx_sleep(pf_purge_proc, &pf_end_lock, 0, "pftmo", 0);
 	}
+	sx_xunlock(&pf_end_lock);
 
 	if (pf_dev != NULL)
 		destroy_dev(pf_dev);
@@ -3801,6 +3806,7 @@ pf_unload(void)
 
 	rw_destroy(&pf_rules_lock);
 	sx_destroy(&pf_ioctl_lock);
+	sx_destroy(&pf_end_lock);
 
 	return (error);
 }