Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 14 Mar 2017 15:09:17 -0700
From:      =?UTF-8?Q?Ermal_Lu=C3=A7i?= <eri@freebsd.org>
To:        Gleb Smirnoff <glebius@freebsd.org>
Cc:        Kristof Provost <kp@freebsd.org>, src-committers <src-committers@freebsd.org>,  svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r315136 - head/sys/netpfil/pf
Message-ID:  <CAPBZQG2dpu1w8fzVXzH6fvGN0JhqGAz13FggOSjcW6Vt_S-S_g@mail.gmail.com>
In-Reply-To: <20170314215706.GB1072@FreeBSD.org>
References:  <201703120542.v2C5gvM4075391@repo.freebsd.org> <20170314215706.GB1072@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Mar 14, 2017 at 2:57 PM, Gleb Smirnoff <glebius@freebsd.org> wrote:

>   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.
>

I already provided the same concerns privately and solutions to it.


>
> 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@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@freebsd.org"
>
> --
> Totus tuus, Glebius.
>
> --
> Ermal
>



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