In message <ZzNux5nU7NNpEVxw@nuc>, Mark Johnston writes:
> On Tue, Nov 12, 2024 at 03:47:25AM +0000, 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 <[email protected]>
> > AuthorDate: 2024-11-02 17:58:59 +0000
> > Commit: Jose Luis Duran <[email protected]>
> > 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 h
> eld:
> > shared rw ipf filter load/unload mutex (ipf filter load/unload mute
> x) 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);
>
> I didn't notice this before, but it's unnecessary to reinitialize the
> callout each time the timeout function fires. The initialization only
> needs to be done once.
I didn't clue in either. It should be removed entirely. It's already
initialized in ipfattach().
>
> > 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);
The second callout_init_rw() call is fine. It's in ipfattach().
> > callout_reset(&softc->ipf_slow_ch, (hz / IPF_HZ_DIVIDE) * IPF_HZ_MULT,
> > ipf_timer_func, softc);
> > return (0);
>
> I think part of this diff is missing. The timeout function still tries
> to acquire this rwlock, and now it'll deadlock since the callout
> framework will have already acquired it as a result of this change.
>
The first callout_init_rw() should not be called as it's already
initialized when ipfattach() is called upon ipfilter initialization.
This will fix it:
diff --git a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
b/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
index b3dea40c3d8c..009b13955cbc 100644
--- a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
+++ b/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
@@ -181,7 +181,6 @@ ipf_timer_func(void *arg)
#if 0
softc->ipf_slow_ch = timeout(ipf_timer_func, softc, hz/2);
#endif
- 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);
--
Cheers,
Cy Schubert <[email protected]>
FreeBSD UNIX: <[email protected]> Web: https://FreeBSD.org
NTP: <[email protected]> Web: https://nwtime.org
e^(i*pi)+1=0
