Date: Tue, 12 Nov 2024 07:52:19 -0800 From: Cy Schubert <Cy.Schubert@cschubert.com> To: John Baldwin <jhb@FreeBSD.org> Cc: Jose Luis Duran <jlduran@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 1fa6daaafd74 - main - ipfilter: Avoid stopping with a lock held Message-ID: <20241112155219.2E15A2DF@slippy.cwsent.com> In-Reply-To: <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org> References: <202411120347.4AC3lPOA015167@gitrepo.freebsd.org> <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org>, John Baldwin wri tes: > On 11/11/24 19:47, Jose Luis Duran wrote: > > The branch main has been updated by jlduran: > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=1fa6daaafd74c1a457dcfe26e0a594 > 3b5441dc9d > > > > commit 1fa6daaafd74c1a457dcfe26e0a5943b5441dc9d > > Author: Jose Luis Duran <jlduran@FreeBSD.org> > > AuthorDate: 2024-11-02 17:58:59 +0000 > > Commit: Jose Luis Duran <jlduran@FreeBSD.org> > > CommitDate: 2024-11-12 03:46:15 +0000 > > > > ipfilter: Avoid stopping with a lock held > > > > Avoid calling _callout_stop_safe with a non-sleepable lock held when > > detaching by initializing callout_init_rw() with CALLOUT_SHAREDLOCK. > > > > It avoids the following WITNESS warning when stopping the service: > > > > # service ipfilter stop > > calling _callout_stop_safe with the following non-sleepable locks > held: > > shared rw ipf filter load/unload mutex (ipf filter load/unload mut > ex) r = 0 (0xffff0000417c7530) locked @ /usr/src/sys/netpfil/ipfilter/netinet > /fil.c:7926 > > stack backtrace: > > #0 0xffff00000052d394 at witness_debugger+0x60 > > #1 0xffff00000052e620 at witness_warn+0x404 > > #2 0xffff0000004d4ffc at _callout_stop_safe+0x8c > > #3 0xffff0000f7236674 at ipfdetach+0x3c > > #4 0xffff0000f723fa4c at ipf_ipf_ioctl+0x788 > > #5 0xffff0000f72367e0 at ipfioctl+0x144 > > #6 0xffff00000034abd8 at devfs_ioctl+0x100 > > #7 0xffff0000005c66a0 at vn_ioctl+0xbc > > #8 0xffff00000034b2cc at devfs_ioctl_f+0x24 > > #9 0xffff0000005331ec at kern_ioctl+0x2e0 > > #10 0xffff000000532eb4 at sys_ioctl+0x140 > > #11 0xffff000000880480 at do_el0_sync+0x604 > > #12 0xffff0000008579ac at handle_el0_sync+0x4c > > > > PR: 282478 > > Suggested by: markj > > Reviewed by: cy > > Approved by: emaste (mentor) > > MFC after: 1 week > > --- > > sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c b/sys/netpfil/ip > filter/netinet/ip_fil_freebsd.c > > index bcde0d2c7323..b3dea40c3d8c 100644 > > --- a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c > > +++ b/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c > > @@ -181,7 +181,7 @@ ipf_timer_func(void *arg) > > #if 0 > > softc->ipf_slow_ch = timeout(ipf_timer_func, softc, hz/2); > > #endif > > - callout_init(&softc->ipf_slow_ch, 1); > > + callout_init_rw(&softc->ipf_slow_ch, &softc->ipf_global.ipf_lk, > CALLOUT_SHAREDLOCK); > > callout_reset(&softc->ipf_slow_ch, > > (hz / IPF_HZ_DIVIDE) * IPF_HZ_MULT, > > ipf_timer_func, softc); > > @@ -221,7 +221,7 @@ ipfattach(ipf_main_softc_t *softc) > > softc->ipf_slow_ch = timeout(ipf_timer_func, softc, > > (hz / IPF_HZ_DIVIDE) * IPF_HZ_MULT); > > #endif > > - callout_init(&softc->ipf_slow_ch, 1); > > + callout_init_rw(&softc->ipf_slow_ch, &softc->ipf_global.ipf_lk, CALLOUT > _SHAREDLOCK); > > callout_reset(&softc->ipf_slow_ch, (hz / IPF_HZ_DIVIDE) * IPF_HZ_MULT, > > ipf_timer_func, softc); > > return (0); > > But this means the callout is now called with the lock held, and I don't see > any changes > to ipf_timer_func. Hmm, so you now recurse on the lock? You need to take ou > t the locking > in ipf_timer_func I think as it is now spurious. You can also axe the #if 0' > d code around > timeout() while you are at it. Reviewing all the #if 0's in ipfilter, discovering why they are there, and either removing them or implementing what Darren had initially intended is in my queue. The upstream git log isn't helpful for reasons I preach about proper commit logs. Given that this one is in ip_fil_freebsd.c (specific to FreeBSD), it can be removed. Others require more grokking. > > -- > John Baldwin > -- Cheers, Cy Schubert <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org NTP: <cy@nwtime.org> Web: https://nwtime.org e^(i*pi)+1=0
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20241112155219.2E15A2DF>