Date: Wed, 15 Mar 2017 22:10:40 -0700 From: John Baldwin <jhb@freebsd.org> To: Kristof Provost <kp@freebsd.org> Cc: Gleb Smirnoff <glebius@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r315136 - head/sys/netpfil/pf Message-ID: <2786760.AftPCYT2ud@ralph.baldwin.cx> In-Reply-To: <00566E96-2330-4354-9095-6D8F77C3AB26@FreeBSD.org> References: <201703120542.v2C5gvM4075391@repo.freebsd.org> <1803226.Igex2bR0P8@ralph.baldwin.cx> <00566E96-2330-4354-9095-6D8F77C3AB26@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, March 16, 2017 11:32:38 AM Kristof Provost wrote:
> On 15 Mar 2017, at 15:45, John Baldwin wrote:
> > You are ignoring interrupts and preemption. Suppose you get an=20
> > interrupt
> > after 'wakeup_one(pf_purge_thread)' and before 'tsleep(..., 0)' in
> > pf_unload(). If the interrupt preempts and results in the purge=20=
> > thread
> > running and issuing its wakeup before the thread executing pf_unloa=
d()
> > resumes, then eventually when pf_unload() resumes it will do a=20
> > tsleep() with
> > no timeout that will never be awoken.
> >
> Thank you for the thorough explanation. I=E2=80=99ll work on a fix an=
d post it=20
> for
> review as soon as I can.
>=20
> > You obviously didn't test this in a debug kernel since there is a=20=
> > KASSERT
> > explicitly to catch obvious tsleep races in _sleep():
> >
> > KASSERT(sbt !=3D 0 || mtx_owned(&Giant) || lock !=3D NULL,
> > ("sleeping without a lock"));
> >
> I=E2=80=99m sure I did test this with both INVARIANTS and WITNESS ena=
bled.
> Is Giant held during module load/unload?
Oh fooey, it is. :-P That's one of the few holdouts is peeling Giant
off of that, but my apologies as that would indeed prevent this KASSERT=
from firing for module load/unload.
--=20
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2786760.AftPCYT2ud>