Reloading of rules should not impact normal packet processing
Needs ReviewPublic
Actions

Authored by eri on Aug 27 2015, 12:39 PM.

Details

Reviewers

kp
glebius
gnn

Summary

pf(4) has the notion of full ruleset changes when you reload from pfctl utility and provides other mechanics for modifying the live ruleset.
The write semantics are not required during preparation of loading a full ruleset but only during commit phase.

This change reduces the overhead of reloading a full ruleset with regard to packets being processed by the host.
Though more work is required in breaking down the locks to specific rulesets since ALTQ, tables, route-to/reply-to, anchors provide the same overhead.

Also this patch fixes some possible memory leaks on rules without existing interfaces.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

eri updated this revision to Diff 8255.Aug 27 2015, 12:39 PM

eri retitled this revision from to Reloading of rules should not impact normal packet processing.

eri updated this object.

eri edited the test plan for this revision. (Show Details)

eri added reviewers: glebius, kp.

eri set the repository for this revision to rS FreeBSD src repository - subversion.

eri added a project: network.

eri changed the edit policy from "All Users" to "network (Project)".

eri added a subscriber: network.

Herald added a subscriber: imp. · View Herald TranscriptAug 27 2015, 12:39 PM

eri added a reviewer: gnn.Aug 27 2015, 12:45 PM

First thing I noticed. I still need to find some more time to look at this in depth.

pf_ioctl.c
1290	You seem to be missing a semicolon here.

I've just hit the following panic while enabling pf. I suspect it's related to this change:

panic: Lock pf rulesets not exclusively locked @ /usr/src/sys/modules/pf/../../netpfil/pf/pf_if.c:190

cpuid = 4
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe023781d1e0
vpanic() at vpanic+0x189/frame 0xfffffe023781d260
panic() at panic+0x43/frame 0xfffffe023781d2c0
rw_assert() at rw_assert+0xa1/frame 0xfffffe023781d2d0
pfi_kif_attach() at pfi_kif_attach+0x2d/frame 0xfffffe023781d300
pfioctl() at pfioctl+0x26ee/frame 0xfffffe023781d800
devfs_ioctl_f() at devfs_ioctl_f+0x122/frame 0xfffffe023781d860
kern_ioctl() at kern_ioctl+0x230/frame 0xfffffe023781d8c0
sys_ioctl() at sys_ioctl+0x153/frame 0xfffffe023781d9a0
amd64_syscall() at amd64_syscall+0x282/frame 0xfffffe023781dab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe023781dab0

syscall (54, FreeBSD ELF64, sys_ioctl), rip = 0x800ddecca, rsp = 0x7fffffffcc18, rbp = 0x7fffffffd840 ---

Yeah i noticed this and am working on fixing it.
Mostly this is from the relaxed requirement to accept rules without existing interface name!

Reduce the diff to only rule handling for now.

ALTQ and dynamic addresses will be a next step to keep the change small and review-able.

Fix possible memory leaks during operation referencing not existing interfaces.

kp added inline comments.Sep 4 2015, 9:10 AM

pf_ioctl.c
1168	I think you need to at least hold the PF_RULES read lock here. pfi_kif_find() has PF_RULES_ASSERT(). I also wonder if it wouldn't be better to do this whole block under the PF_RULES_WLOCK(). That way we'd never have the rule->kif != kif case.
1307	I think it'd be best to set rule->kif to NULL after this.

kp added inline comments.Sep 4 2015, 10:45 PM

pf_ioctl.c
1290	It's also not clear to me why this is safe. We only have the PF_RULES_RLOCK here, so other threads can be iterating ruleset->rules[rs_num].inactive.ptr (pf_get_pool() for example does this with the RLOCK held.