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

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=1fa6daaafd74c1a457dcfe26e0a5943b5441dc9d
> 
> 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 mutex) 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/ipfilter/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.

>  		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);

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.