From nobody Tue Nov 12 16:02:43 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 4Xnrm63Ym3z5ck3R; Tue, 12 Nov 2024 16:02:46 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from omta002.cacentral1.a.cloudfilter.net (omta002.cacentral1.a.cloudfilter.net [3.97.99.33]) (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 4Xnrm61J0Jz4Xvy; Tue, 12 Nov 2024 16:02:46 +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 AnxEtmZw5MArNAtLpttnEK; Tue, 12 Nov 2024 16:02:45 +0000 Received: from spqr.komquats.com ([70.66.152.170]) by cmsmtp with ESMTPSA id AtLntgDt22M9qAtLotxnka; Tue, 12 Nov 2024 16:02:45 +0000 X-Auth-User: cschuber X-Authority-Analysis: v=2.4 cv=ce5xrWDM c=1 sm=1 tr=0 ts=67337c25 a=y8EK/9tc/U6QY+pUhnbtgQ==:117 a=y8EK/9tc/U6QY+pUhnbtgQ==:17 a=kj9zAlcOel0A:10 a=VlfZXiiP6vEA:10 a=VxmjJ2MpAAAA:8 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=EkcXrb_YAAAA:8 a=lxbSc3apC6ghUbFFKhkA:9 a=CjuIK1q_8ugA:10 a=7gXAzLPJhVmCkEl4_tsf:22 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 BAA7F9F4; Tue, 12 Nov 2024 08:02:43 -0800 (PST) Received: by slippy.cwsent.com (Postfix, from userid 1000) id B372532F; Tue, 12 Nov 2024 08:02:43 -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: Cy Schubert cc: John Baldwin , 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: <20241112155219.2E15A2DF@slippy.cwsent.com> References: <202411120347.4AC3lPOA015167@gitrepo.freebsd.org> <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org> <20241112155219.2E15A2DF@slippy.cwsent.com> Comments: In-reply-to Cy Schubert message dated "Tue, 12 Nov 2024 07:52:19 -0800." 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 08:02:43 -0800 Message-Id: <20241112160243.B372532F@slippy.cwsent.com> X-CMAE-Envelope: MS4xfCW7NLTpqnsRdqW1TxzTj7s79b01JqrDtmj3qWrqw9mu39E50V5aorvipPMpv19Ifm6LUPSwMkZlm3B1lTQR7c5vHZ5Oyze/zlQ4f0hDIplGp8Ibv3Zt Aer+mEQ6+bT9rH2btZbibaEBXbLDljZpIphfrf5wjzyRidPFLP8DZ6KNsgxJMOrsku/Z4b47sIgpj1FOdP0vERr30ZpNcUgtEO1TPRs9CWwNlMHLq9rZS+Hc Y6cQiZkPpmKC/Kso/HTwG1fOF5SYn9snGY0ldo/cPcglfSa4IM6eVbmTFwh9XT6st/EN4IrZUgw8Uip5Mp3zAezzPN763ip+NUpL8r9JvM5UAMIU/wTdu95b Vy8SDw2N 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: 4Xnrm61J0Jz4Xvy X-Spamd-Bar: ---- 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 > > > 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 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 FreeBSD UNIX: Web: https://FreeBSD.org NTP: Web: https://nwtime.org e^(i*pi)+1=0