Re: git: 1fa6daaafd74 - main - ipfilter: Avoid stopping with a lock held

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