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

From: Cy Schubert <Cy.Schubert_at_cschubert.com>
Date: Tue, 12 Nov 2024 18:41:11 UTC
I'm building a test patch on my sandbox machine. The patches in my git repo 
will,

a) remove another #if 0 from 2013,
b) revert this commit,
c) fix the "stopping with lock held" bug

Testing on the sandbox is needed before I submit a review.

You can revert this if you want, or I can push my three commits when my 
testing is complete.


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


In message <CAPwQLccpVdjUKQbCVQ0Vg5yRnmYSeKh+b+ThJwQY=z8OCZGOAw@mail.gmail.c
om>
, Jose Luis Duran writes:
> 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 <Cy.Schubert@cschubert.=
> com> 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 <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 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 <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=3D0
> >
> >
> >
> >
> >
>
>
> --=20
> Jose Luis Duran