ipfilter: Do not reinitialize lock in timer function
ClosedPublic
Actions

Authored by jlduran on Tue, Nov 12, 6:31 PM.

Details

Reviewers

cy
emaste
jhb
markj

Commits

rG5006b9b4223c: ipfilter: Avoid holding a lock while stopping
rG8b29ab3aff13: ipfilter: Avoid holding a lock while stopping
rG650900cc2f60: ipfilter: Avoid holding a lock while stopping

Summary

This is unnecessary and may result in a deadlock.

PR: 282478
Reported by: markj
Fixes: 1fa6daaafd74 ("ipfilter: Avoid stopping with a lock held")

Test Plan

My interaction with ipfilter is limited to the following tests:

Testing the original PR:

service ipfw onestart
service ipfw onestop
dmesg -a

Running the test suite:

kldload if_bridge
kldload if_epair
cd /usr/tests
kyua test -k /usr/tests/Kyuafile sys/netpfil/common | grep -v skipped:
sys/netpfil/common/forward:ipf_v4  ->  passed  [8.042s]
sys/netpfil/common/fragments:ipf_fragments  ->  passed  [11.408s]
sys/netpfil/common/nat:ipfnat_basic  ->  passed  [4.115s]
sys/netpfil/common/pass_block:ipf_v4  ->  passed  [1.511s]
sys/netpfil/common/pass_block:ipf_v6  ->  passed  [3.136s]
sys/netpfil/common/rdr:ipfnat_basic  ->  passed  [4.921s]
sys/netpfil/common/rdr:ipfnat_local_redirect  ->  passed  [4.727s]

...

7/42 passed (0 broken, 0 failed, 35 skipped)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 60507
Build 57391: arc lint + arc unit

Event Timeline

jlduran created this revision.Tue, Nov 12, 6:31 PM

Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro, imp. · View Herald TranscriptTue, Nov 12, 6:31 PM

jlduran requested review of this revision.Tue, Nov 12, 6:31 PM

Harbormaster completed remote builds in B60507: Diff 146330.Tue, Nov 12, 6:32 PM

jhb accepted this revision.Tue, Nov 12, 8:16 PM

This revision is now accepted and ready to land.Tue, Nov 12, 8:16 PM

This is the ticket. You will need to reapply the revert. Probably makes more sense to reapply while squashing this. I don't know if you need still mentor approval or if my approval is good enough.

Approved

My apologies, I forgot that it's always ok for a rwlock read lock to recurse, so there is no deadlock. Most of our lock types don't have that property.

In D47530#1084128, @cy wrote:

This is the ticket. You will need to reapply the revert. Probably makes more sense to reapply while squashing this. I don't know if you need still mentor approval or if my approval is good enough.

Will do then. If you have other tests that you run, I would gladly try to add them to the suite (once I finish with other fixes), I feel ipfilter is "undertested", but I may be wrong.
I was explained that the approval of my mentor is an "administrative approval", and may not necessarily be a technical one, of course, having both is much more reassuring.

Thank you!

In D47530#1084133, @jlduran wrote:

In D47530#1084128, @cy wrote:

This is the ticket. You will need to reapply the revert. Probably makes more sense to reapply while squashing this. I don't know if you need still mentor approval or if my approval is good enough.

Will do then. If you have other tests that you run, I would gladly try to add them to the suite (once I finish with other fixes), I feel ipfilter is "undertested", but I may be wrong.
I was explained that the approval of my mentor is an "administrative approval", and may not necessarily be a technical one, of course, having both is much more reassuring.

Thank you!

I simply test on my sandbox machine. It's working there, no panics. Should have tested it first before accepting the review. (And if I'm ready to commit I always install on my prod firewall here at home. It panicking or not working in some way impacts my ability to work from home, as the company has become a WFH company since the pandemic.)

This + the previous patch works perfectly.

I'm sorry for not testing the patch before accepting it. That's on me.

Closed by commit rG650900cc2f60: ipfilter: Avoid holding a lock while stopping (authored by jlduran). · Explain WhyTue, Nov 12, 9:33 PM

This revision was automatically updated to reflect the committed changes.

jlduran added a commit: rG650900cc2f60: ipfilter: Avoid holding a lock while stopping.

jlduran added a commit: rG8b29ab3aff13: ipfilter: Avoid holding a lock while stopping.Tue, Nov 19, 2:39 AM

jlduran added a commit: rG5006b9b4223c: ipfilter: Avoid holding a lock while stopping.