Re: git: 1fa6daaafd74 - main - ipfilter: Avoid stopping with a lock held
Date: Tue, 12 Nov 2024 16:02:43 UTC
In message <20241112155219.2E15A2DF@slippy.cwsent.com>, Cy Schubert writes: > 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=1fa6daaafd74c1a457dcfe26e0a5 > 94 > > 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 lock > s > > held: > > > shared rw ipf filter load/unload mutex (ipf filter load/unload m > ut > > ex) r = 0 (0xffff0000417c7530) locked @ /usr/src/sys/netpfil/ipfilter/netin > et > > /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_H > Z_MULT, > > > ipf_timer_func, softc); > > > return (0); > > > > But this means the callout is now called with the lock held, and I don't se > e > > 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. It can go. It was #if 0'd by ea3022cbbd3f5, IIRC in discussion with you at the time (2013). ea3022cbbd3f5 converted timeout(9) to callout(9). This code remained as an artifact of the conversion. It was sent by you to me following the ipfilter upgrade from 4.1.28 to 5.1.2 earlier that year. -- 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