Bug 251414
| Summary: | pf sometimes panics when reloading ruleset with tables | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | sigsys | ||||||
| Component: | kern | Assignee: | Mark Johnston <markj> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | kp, markj | ||||||
| Priority: | --- | Keywords: | crash | ||||||
| Version: | 12.2-RELEASE | ||||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
|
Description
sigsys
2020-11-26 20:25:46 UTC
Please include your pf.conf and the table file. Created attachment 220012 [details]
pf.conf
Created attachment 220013 [details]
table
(In reply to Kristof Provost from comment #1) I must admit, even though the ruleset isn't too big, there might be A LITTLE BIT of weirdness going on there with FIBs and firewalled VMs on separate bridges and routed taps. But it was working fine before the big table. Also I already had tables in it, but with just a few entries and they very rarely changed. They weren't being reloaded automatically with the rest of the ruleset unlike that new <firehol-level1> one I tried to add. Do you have any pf_flags or anything else set in /etc/rc.conf? (In reply to Kristof Provost from comment #5) No pf_flags. Only pf settings are: pf_enable="YES" pflog_enable="YES" pflog_flags="-s 0" OOOPS. Sorry, I made a mistake. I kept editing my pf.conf while trying to figure this out and got confused as to what edits I had on exactly when the crashes happen. It seems to be related to the "counters" keyword of tables. I had it on at some point and it can cause crashes later on when you reload. table <firehol-level1> persist counters file "/etc/pf/firehol-level1-blocklist.tab" If I've been reading this stack trace and the disassembly correctly, the crash must be happening right there: https://github.com/freebsd/freebsd/blob/releng/12.2/sys/netpfil/pf/pf_table.c#L2112-L2113 2c228: 65 48 83 01 01 addq $1, %gs:(%rcx) This one is my fault I think, but I haven't been able to trigger it on head by reloading a table while toggling the "counters" keyword. (In reply to Mark Johnston from comment #8) Looks like I found an easy way to reproduce on -CURRENT. It doesn't even need a big table loaded from a file. Looks like it's related to toggling the counters support like you said. I guess I somehow managed to keep messing with this flag while trying out rulesets. Load this ruleset and enable pf: pass all table <tab> { self } pass in log to <tab> Then switch to this ruleset: pass all table <tab> counters { self } pass in log to <tab> Then send in some traffic that should create a new state. Panics every time for me. If the table already existed it might need to be destroyed first. I think the problem is in pfr_commit_ktable(). When it transfers the addresses from the "shadow" inactive table to the active table, in some cases the existing entries will be reused and those won't have their counters if the active table wasn't created with counters support to begin with. Maybe it could end up leaking counters too if you try to disabling them in the new table? I think the table can end up with a mix of entries with and without counters. (In reply to sigsys from comment #9) Thanks for the analysis, I believe you are right. This patch fixes the bug for me: https://reviews.freebsd.org/D27440 (In reply to Mark Johnston from comment #10) Works great here. Switching between two rulesets with and without counters for the table in a loop while killing the table in a loop while running iperf works. I didn't think of exchanging the counters like that. It's a nice solution, it avoids having to allocate new ones in a function that must not fail and it gets rid of the existing counters when they're not needed anymore. A commit references this bug: Author: markj Date: Wed Dec 2 16:01:43 UTC 2020 New revision: 368276 URL: https://svnweb.freebsd.org/changeset/base/368276 Log: pf: Fix table entry counter toggling When updating a table, pf will keep existing table entry structures corresponding to addresses that are in both of the old and new tables. However, the update may also enable or disable per-entry counters which are allocated separately. Thus when toggling PFR_TFLAG_COUNTERS, the entries may be missing counters or may have unused counters allocated. Fix the problem by modifying pfr_ina_commit() to transfer counters from or to entries in the shadow table. PR: 251414 Reported by: sigsys@gmail.com Reviewed by: kp MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D27440 Changes: head/sys/netpfil/pf/pf_table.c A commit references this bug: Author: kp Date: Wed Dec 2 16:33:23 UTC 2020 New revision: 368277 URL: https://svnweb.freebsd.org/changeset/base/368277 Log: pf tests: Test case for bug #251414 Changing a table from not having counters to having counters (or vice versa) may trigger panics. PR: 251414 MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D27441 Changes: head/tests/sys/netpfil/pf/table.sh A commit references this bug: Author: markj Date: Wed Dec 9 00:56:15 UTC 2020 New revision: 368470 URL: https://svnweb.freebsd.org/changeset/base/368470 Log: MFC r368276: pf: Fix table entry counter toggling PR: 251414 Changes: _U stable/12/ stable/12/sys/netpfil/pf/pf_table.c Thanks for the report and analysis. A commit references this bug: Author: kp Date: Wed Dec 9 17:17:45 UTC 2020 New revision: 368488 URL: https://svnweb.freebsd.org/changeset/base/368488 Log: MFC r368277: pf tests: Test case for bug #251414 Changing a table from not having counters to having counters (or vice versa) may trigger panics. PR: 251414 Changes: _U stable/12/ stable/12/tests/sys/netpfil/pf/table.sh |
