svn commit: r315136 - head/sys/netpfil/pf

[Gleb Smirnoff]

  Kristof,

On Sun, Mar 12, 2017 at 05:42:57AM +0000, Kristof Provost wrote:
K> Log:
K>   pf: Fix incorrect rw_sleep() in pf_unload()
K>   
K>   When we unload we don't hold the pf_rules_lock, so we cannot call rw_sleep()
K>   with it, because it would release a lock we do not hold. There's no need for the
K>   lock either, so we can just tsleep().
K>   
K>   While here also make the same change in pf_purge_thread(), because it explicitly
K>   takes the lock before rw_sleep() and then immediately releases it afterwards.

The correct change would to be grab lock in pf_unload(), exactly as pf_purge_thread()
does. With your change you introduces a possible infinite sleep due to race, since
there is no timeout and no lock.

No... Actually both cases should PF_RULES_WLOCK(), and read/write the pf_end_threads
variable under this lock. And use rw_sleep.

K> Modified:
K>   head/sys/netpfil/pf/pf.c
K>   head/sys/netpfil/pf/pf_ioctl.c
K> 
K> Modified: head/sys/netpfil/pf/pf.c
K> ==============================================================================
K> --- head/sys/netpfil/pf/pf.c	Sun Mar 12 05:36:31 2017	(r315135)
K> +++ head/sys/netpfil/pf/pf.c	Sun Mar 12 05:42:57 2017	(r315136)
K> @@ -1429,9 +1429,7 @@ pf_purge_thread(void *unused __unused)
K>  	u_int idx = 0;
K>  
K>  	for (;;) {
K> -		PF_RULES_RLOCK();
K> -		rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftm", hz / 10);
K> -		PF_RULES_RUNLOCK();
K> +		tsleep(pf_purge_thread, 0, "pftm", hz / 10);
K>  
K>  		VNET_LIST_RLOCK();
K>  		VNET_FOREACH(vnet_iter) {
K> 
K> Modified: head/sys/netpfil/pf/pf_ioctl.c
K> ==============================================================================
K> --- head/sys/netpfil/pf/pf_ioctl.c	Sun Mar 12 05:36:31 2017	(r315135)
K> +++ head/sys/netpfil/pf/pf_ioctl.c	Sun Mar 12 05:42:57 2017	(r315136)
K> @@ -3791,7 +3791,7 @@ pf_unload(void)
K>  	pf_end_threads = 1;
K>  	while (pf_end_threads < 2) {
K>  		wakeup_one(pf_purge_thread);
K> -		rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftmo", 0);
K> +		tsleep(pf_purge_thread, 0, "pftmo", 0);
K>  	}
K>  
K>  	if (pf_dev != NULL)
K> _______________________________________________
K> svn-src-all at freebsd.org mailing list
K> https://lists.freebsd.org/mailman/listinfo/svn-src-all
K> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"

-- 
Totus tuus, Glebius.