From nobody Tue Nov 12 15:37:09 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XnrBd2cHcz5chCs; Tue, 12 Nov 2024 15:37:13 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from omta001.cacentral1.a.cloudfilter.net (omta001.cacentral1.a.cloudfilter.net [3.97.99.32]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XnrBc3qbRz4VKQ; Tue, 12 Nov 2024 15:37:12 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Authentication-Results: mx1.freebsd.org; none Received: from shw-obgw-4002a.ext.cloudfilter.net ([10.228.9.250]) by cmsmtp with ESMTPS id AqlStjniCAHbZAsx5tPHH1; Tue, 12 Nov 2024 15:37:11 +0000 Received: from spqr.komquats.com ([70.66.152.170]) by cmsmtp with ESMTPSA id Asx4tg76u2M9qAsx4txi9K; Tue, 12 Nov 2024 15:37:11 +0000 X-Auth-User: cschuber X-Authority-Analysis: v=2.4 cv=ce5xrWDM c=1 sm=1 tr=0 ts=67337627 a=y8EK/9tc/U6QY+pUhnbtgQ==:117 a=y8EK/9tc/U6QY+pUhnbtgQ==:17 a=kj9zAlcOel0A:10 a=VlfZXiiP6vEA:10 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=EkcXrb_YAAAA:8 a=qctbC0zNG9NHIYxnFvAA:9 a=CjuIK1q_8ugA:10 a=Ia-lj3WSrqcvXOmTRaiG:22 a=LK5xJRSDVpKd5WXXoEvA:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTP id BB95E9AC; Tue, 12 Nov 2024 07:37:09 -0800 (PST) Received: by slippy.cwsent.com (Postfix, from userid 1000) id B1B0C1F7; Tue, 12 Nov 2024 07:37:09 -0800 (PST) X-Mailer: exmh version 2.9.0 11/07/2018 with nmh-1.8+dev Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Mark Johnston cc: Jose Luis Duran , 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 In-reply-to: References: <202411120347.4AC3lPOA015167@gitrepo.freebsd.org> Comments: In-reply-to Mark Johnston message dated "Tue, 12 Nov 2024 10:05:43 -0500." List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Tue, 12 Nov 2024 07:37:09 -0800 Message-Id: <20241112153709.B1B0C1F7@slippy.cwsent.com> X-CMAE-Envelope: MS4xfN+fEwEXqtI+DIvH3BrQ2eyu4FPZdQ9IuL/KQIJkKpRKSDeHMrHHwg8zxS2RSRhp7GgVMjrLIR9QtY9pN7chD71DqXk5fOgeEVYF4FwWH8HAzsf9/qz3 P0n5OktfeJ8NeORQWHdAYe/VhxNEHuQju8/DpiTdyVSBeIH9UIW5mAdH6njA3a4ChOFeKPdoPBLukmOI8VenqlhvL3SWq69cME+Fy+kNYRbc9u2d9AZ0HJdB ZSacbKb8z9b6Xbsk/qTRNpyMGAW4zpJwZEFrgB9EVaQWHPoJnurJOv/GIfN5KYNPqf7qXGJSzBa6xpdaNet6TOKKiET5phuBJkwQRobtpEF6kKQvKaZkg70Y K7zK/bS1 X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:16509, ipnet:3.96.0.0/15, country:US] X-Rspamd-Queue-Id: 4XnrBc3qbRz4VKQ X-Spamd-Bar: ---- In message , 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 > > AuthorDate: 2024-11-02 17:58:59 +0000 > > Commit: Jose Luis Duran > > 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 FreeBSD UNIX: Web: https://FreeBSD.org NTP: Web: https://nwtime.org e^(i*pi)+1=0