Page MenuHomeFreeBSD

Reloading of rules should not impact normal packet processing
Needs ReviewPublic

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
Lint
Lint Skipped
Unit
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.
eri added a project: network.
eri changed the edit policy from "All Users" to "network (Project)".
eri added a subscriber: network.
eri added a reviewer: gnn.Aug 27 2015, 12:45 PM
kp edited edge metadata.Aug 30 2015, 3:36 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.

kp added a comment.Aug 30 2015, 4:16 PM

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 ---
eri added a comment.Aug 31 2015, 9:19 AM

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

eri updated this revision to Diff 8367.EditedAug 31 2015, 10:35 AM
eri updated this object.
eri edited edge metadata.

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.