Page MenuHomeFreeBSD

Make pf more VNET safe
ClosedPublic

Authored by bz on Jun 22 2016, 12:59 PM.

Details

Summary

Update pf(4) and pflog(4) to survive basic VNET testing, which includes
proper virtualisation, teardown, avoiding use-after-free, race conditions,
no longer creating a thread per VNET (which could easily be a couple of
thousand threads), handling global events (e.g., eventhandlers) on teardown,
clearing various globally cached pointers and checking them before use.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bz updated this revision to Diff 17762.Jun 22 2016, 12:59 PM
bz retitled this revision from to Make pf more VNET safe.
bz updated this object.
bz edited the test plan for this revision. (Show Details)
bz added reviewers: gnn, emaste, kp, nvass-gmx.com.
bz added a subscriber: network.
robak added a subscriber: robak.Jun 22 2016, 1:20 PM

I can't discuss the code, I can only test it and see if it makes the difference in system stability. Any chance for this to land in 11.0?

bz added a comment.Jun 22 2016, 2:01 PM
In D6924#145160, @robak wrote:

I can't discuss the code, I can only test it and see if it makes the difference in system stability. Any chance for this to land in 11.0?

Yes, that's the plan. Needs all the testing it can get and I am still looking at pfsync and a possible very old security issue (don't run with untrusted customers in VNET yet) which I don't know if FreeBSD is vulnerable to.

kp added inline comments.Jun 22 2016, 8:15 PM
sys/netpfil/pf/pf.c
302 ↗(On Diff #17762)

There's a '#define V_pf_end_threads VNET(pf_end_threads)' in net/pfvar.h.
Presumably you'll want to get rid of that too.

sys/netpfil/pf/pf_if.c
911 ↗(On Diff #17762)

Minor typo here and in the others like it. 'expensie'.

sys/netpfil/pf/pf_ioctl.c
3782 ↗(On Diff #17762)

When is this ever not true?
As far as I can tell you removed the only pf_end_threads++; from pf_purge_thread()

bz marked 3 inline comments as done.Jun 23 2016, 12:39 AM

Should all be addressed in the updated diff to come.

sys/netpfil/pf/pf_ioctl.c
3782 ↗(On Diff #17762)

Good catch; that would be an endless loop...

bz updated this revision to Diff 17783.Jun 23 2016, 12:41 AM
bz marked an inline comment as done.

Address comments from @kristof .
Especially get rid of an endless loooCp on unload hopefully.p

kp accepted this revision.Jun 23 2016, 6:03 AM
kp edited edge metadata.
This revision is now accepted and ready to land.Jun 23 2016, 6:03 AM
This revision was automatically updated to reflect the committed changes.