pf: Merge pf_clear_srcnodes() and pf_kill_srcnodes()

[vegeta_tuxpowered.net]

Summary

The functions pf_clear_srcnodes() and pf_kill_srcnodes() serve the same
purpose, however the former kills all source nodes while the later only a
selected subset of them.

They differ in how they reach that goal. Pf_clear_srcnodes() first iterates
over all states and detaches the source nodes from them. Then it iterates
over all source nodes and marks them as expired leaving the cleanup to
pf_purge_expired_src_nodes().

If a new state and a new source node are created between iterating over all
states and all source nodes, this source node will have its state counter
set to 0 and expiry to 1, marking it as expired without properly detaching
the state from it. Later the source node will be freed with the state sill
pointing to it.

The function pf_kill_srcnodes() performs the same operation in a safer
manner by first marking the required source nodes as expiring and then
iterating over all states and checking which states point to expiring nodes.
Any source node created between iterating over states and source nodes will
simply be ignored.

Add functionality of killing all source nodes to pf_kill_srcnodes(). Replace
all calls to pf_clear_srcnodes() with a calls to pf_kill_srcnodes(), and
remove the former.

Test Plan

Triggering a kernel panic is trivial. All you need is a "router" jail with pf ruleset with a single rule creating source nodes, in my case it was rdr … sticky-address, a "server" jail with nginx, and two shell oneliners: while true; do curl …; done and while true; do jexec router pfctl -FS; done and give it a few minutes. It will cause general protection fault in pf_counters_inc().

With this patch the panic does not happen anymore.

[kp]

This looks good to me.

I think we currently don't have any test cases for either DIOCCLRSRCNODES or DIOCCLRSRCNODES. I'll see if I can add some in the next few days. They probably won't be able to trigger the crash you describe, but basic tests are better than no tests at all.

[emaste]

sys/netpfil/pf/pf_ioctl.c

5453

I wonder if leaving the combined one named pf_clear_srcnodes makes sense for consistency with DIOCCLRSRCNODES

[kp]

sys/netpfil/pf/pf_ioctl.c

5453

We also call it from DIOCKILLSRCNODES, so I don't think I agree.
One of the two is going to have an inconsistent name either way (unless we go with pf_kill_clear_srcnodes(), but .. no), so I lean towards not renaming it, if only for the smaller diff.

[emaste]

sys/netpfil/pf/pf_ioctl.c

5453

Oh, indeed - I didn't notice that.