This is unnecessary and may result in a deadlock.
PR: 282478
Reported by: markj
Fixes: 1fa6daaafd74 ("ipfilter: Avoid stopping with a lock held")
Differential D47530
ipfilter: Do not reinitialize lock in timer function jlduran on Tue, Nov 12, 6:31 PM. Authored by Tags None Referenced Files
Subscribers
Details This is unnecessary and may result in a deadlock. PR: 282478 My interaction with ipfilter is limited to the following tests:
service ipfw onestart service ipfw onestop dmesg -a
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
Event TimelineComment Actions 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. Comment Actions 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. Comment Actions 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. Thank you! Comment Actions 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. |