From nobody Tue Nov 12 18:35:55 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 4Xnw952NYnz5cvyl; Tue, 12 Nov 2024 18:36:09 +0000 (UTC) (envelope-from jlduran@gmail.com) Received: from mail-yw1-f170.google.com (mail-yw1-f170.google.com [209.85.128.170]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Xnw945d0jz4trw; Tue, 12 Nov 2024 18:36:08 +0000 (UTC) (envelope-from jlduran@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-yw1-f170.google.com with SMTP id 00721157ae682-6ea5cc68944so727267b3.1; Tue, 12 Nov 2024 10:36:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731436567; x=1732041367; h=content-transfer-encoding:cc:to:subject:message-id:date:from :reply-to:in-reply-to:references:mime-version:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=a1ISLPTtHmqXfO4952p6takPms+ko5QSCXT7XinyAXY=; b=C8HiKE/J7D7U6KkMCIkFEyZN0m4X61ELMCDmHWJnqcibVEjGo6ESM5Q+nmk5FOrzBC WRrI+pKmPMcFTtv7cSeXT9TQIUAToqfdO8k6B7XnvS1Vw1F+8egjfpr+k29FN7sXj3pv +6uhWRS+xN7d2ra6A6vwCKUrjYqEwNnY8CAzO820js+oIFd13M46OikhFtMUvYVLI6S3 EP1HHczgn/0DN7Q5AVU83OIPzPjkkPlXLP1bdM5K//VDn8rCrDu+P4QvXp2V21f2i9gT J+Ek2N8l3qGkfHeBlhQkRCBUUXl7FRLmHcaUewsd8cuvNsFG4kDx2H3IEQMEDW/6Oh19 dqaQ== X-Forwarded-Encrypted: i=1; AJvYcCUMGJCnZ7tsXIs3vhly7YGQOaspFSIF21T/qK1gKMhJMXVvYNOWXMABJDoTLnXUCeLWYTUZyyAbO9Pyrck0Hpo6mS79Pyc=@freebsd.org, AJvYcCVKICxaRSrCrMa1rgpSSHdBp3jm04nWxz6k83S7s8K1aozwBvjLNSCmqeNe2Kr9PkbYltuR14UbfvpiWA2JjL0=@freebsd.org, AJvYcCWBWaRROnmwh5g6yk8pnA5c2r6jMtvWV2uVq1vNGKqWX2HA8yVHc8zfkin6N0YjQX7Nx/Uq0u4JhWMwVKrsWeflLQtO@freebsd.org X-Gm-Message-State: AOJu0YzAgUt0pPe7UoL0jeKyGKGnrDWZPbsmRovsDaaRK/hGTvNwo5Y2 Uw06Np5y3DbdpF6N7kOeTn2nsQvf38WZvjVaE0VQCpgJrMMbD3zvBj8PjTaGm0E= X-Google-Smtp-Source: AGHT+IFKLkgABr3yC2ET35sK7PTt0fq8Mg/2DkTlSjfwEPvRXRW3Kj6viYZ0DXi9OEf1oHqB8kZoRA== X-Received: by 2002:a05:690c:a85:b0:6ea:85ee:b5bb with SMTP id 00721157ae682-6eaddd9fbbamr59953827b3.3.1731436567149; Tue, 12 Nov 2024 10:36:07 -0800 (PST) Received: from mail-yb1-f176.google.com (mail-yb1-f176.google.com. [209.85.219.176]) by smtp.gmail.com with ESMTPSA id 00721157ae682-6eaceb7a6f0sm26937477b3.104.2024.11.12.10.36.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Nov 2024 10:36:06 -0800 (PST) Received: by mail-yb1-f176.google.com with SMTP id 3f1490d57ef6-e30ca1726c5so110562276.0; Tue, 12 Nov 2024 10:36:06 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCV8v8XFD8/ksPIprsEwX6Dk065qIzLLZguG5NtOxg5jz49RogV08VoAtkL9RkdIq7aIz9nMXw8Q8CwPeSRsTRXiSmH50Vw=@freebsd.org, AJvYcCWN2njTeXemiBCjNenzzGjlJoeCgT4OvMDk9pAaWBVOcBK0sTFkIbYaiTZ28LIi08WF6kDxIR/p+KRxm3dsfUinsQhw@freebsd.org, AJvYcCWoixw6yWxloxRLTHiH3v64rMLTIOzTQcU5lNoz138DWuozC4bN90Vy9doHtseM2O1e4cxBJiV+FpkIDh9qTak=@freebsd.org X-Received: by 2002:a05:6902:2290:b0:e30:de32:8362 with SMTP id 3f1490d57ef6-e337f901c8bmr6027980276.11.1731436566532; Tue, 12 Nov 2024 10:36:06 -0800 (PST) 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 References: <202411120347.4AC3lPOA015167@gitrepo.freebsd.org> <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org> <20241112155219.2E15A2DF@slippy.cwsent.com> <20241112160243.B372532F@slippy.cwsent.com> In-Reply-To: <20241112160243.B372532F@slippy.cwsent.com> Reply-To: jlduran@freebsd.org From: Jose Luis Duran Date: Tue, 12 Nov 2024 15:35:55 -0300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: git: 1fa6daaafd74 - main - ipfilter: Avoid stopping with a lock held To: Cy Schubert Cc: John Baldwin , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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:15169, ipnet:209.85.128.0/17, country:US] X-Rspamd-Queue-Id: 4Xnw945d0jz4trw X-Spamd-Bar: ---- What would be the recommended approach in this case? Revert, and commit with the fixes, or just fix the problematic code. At any rate, I have created https://reviews.freebsd.org/D47530. Feel free to commandeer it, as my testing is limited to what's currently in the "Test Plan". I was not able to recreate the deadlock described here, but it definitely makes sense. On Tue, Nov 12, 2024 at 1:02=E2=80=AFPM Cy Schubert wrote: > > In message <20241112155219.2E15A2DF@slippy.cwsent.com>, Cy Schubert write= s: > > In message <09923ad3-4065-4a31-b35f-74f84e09cff1@FreeBSD.org>, John Bal= dwin > > 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=3D1fa6daaafd74c1a457dc= fe26e0a5 > > 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 hel= d when > > > > detaching by initializing callout_init_rw() with CALLOUT_SHARE= DLOCK. > > > > > > > > It avoids the following WITNESS warning when stopping the serv= ice: > > > > > > > > # service ipfilter stop > > > > calling _callout_stop_safe with the following non-sleepabl= e lock > > s > > > held: > > > > shared rw ipf filter load/unload mutex (ipf filter load/un= load m > > ut > > > ex) r =3D 0 (0xffff0000417c7530) locked @ /usr/src/sys/netpfil/ipfilt= er/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/ne= tpfil/ > > 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 =3D timeout(ipf_timer_func, so= ftc, hz/ > > 2); > > > > #endif > > > > - callout_init(&softc->ipf_slow_ch, 1); > > > > + callout_init_rw(&softc->ipf_slow_ch, &softc->ipf_global.i= pf_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 =3D timeout(ipf_timer_func, softc, > > > > (hz / IPF_HZ_DIVIDE) * IPF_H= Z_MULT > > ); > > > > #endif > > > > - callout_init(&softc->ipf_slow_ch, 1); > > > > + callout_init_rw(&softc->ipf_slow_ch, &softc->ipf_global.ipf_lk, C= ALLOUT > > > _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 do= n'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 th= e #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 ab= out > > proper commit logs. > > > > Given that this one is in ip_fil_freebsd.c (specific to FreeBSD), it ca= n be > > removed. Others require more grokking. > > It can go. It was #if 0'd by ea3022cbbd3f5, IIRC in discussion with you a= t > the time (2013). > > ea3022cbbd3f5 converted timeout(9) to callout(9). This code remained as a= n > artifact of the conversion. It was sent by you to me following the ipfilt= er > 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=3D0 > > > > > --=20 Jose Luis Duran