pf: Add per-rule timestamps for rule and eth_rule

[kp]

Summary

Similar to ipfw rule timestamps, these timestamps internally are uint32_t snaps
of the system time in seconds. The timestamp is CPU local and updated each time
a rule or a state associated with a rule or state is matched.

[kp]

sbin/pfctl/pfctl.c

1022

I think we want to do this conversion in libpfctl, otherwise the last_active field in pfctl_eth_rule never gets populated.
Although arguably if the last_active_timestamp and last_active are just the same value, only different types there may not be a lot of utility in keeping them both, and maybe we should only keep a time_t last_active in struct pfctl_eth_rule.

sys/netpfil/pf/pf.c

3974

ipfw does a check to see if the time_uptime changed before it does the write. (That was introduced in 7e767c791f65f8d07bf99ce1d33d7f3e36e269d5).

I wonder if it's worth doing that as well. time_uptime is only going to change once a second, and we can get a lot of packets pass through in a second. I'm not clear on the details of how the CPU cache will deal with this, but it's certainly something we should try to benchmark.

sys/netpfil/pf/pf_nv.c

741

Would it be easier to use time_second instead of time_uptime plus the boottime? time(9) describes that as 'The time_second variable is the system's “wall time” clock to the second.'

[mjg]

sys/netpfil/pf/pf.c

3974

this will be terrible for peformance. branching on time_uptime being different is probably tolerable for the time being, but preferably it would get sorted out to be per-cpu to avoid any stores to the shared area.

this needs to use atomic_store_int to save the value and atomic_load_int to read time_uptime. Bugs of this sort are all over the kernel right now and perhaps it would make sense to convert time_uptime to an opaque time which causes a compilation failure when read without a proper accessor.

[linnemannr_gmail.com]

sbin/pfctl/pfctl.c

1022

I actually had intended to remove the last_active field from the rules structs, which is why this code just uses a stack var. I'll fix that.

sys/netpfil/pf/pf.c

3974

I also agree that branching is unwanted here. I've been toying with the idea of the rule timestamp update being an asynchronous task since the resolution is only one second. The rule / state match would only need to add the rule address to a collection of rules that need updating, and the async task would do the actual work of updating the rules. How does that idea strike you?

sys/netpfil/pf/pf_nv.c

741

Probably, I think we only used time_uptime because ipfw already did. I'll make this change.

[kp]

sys/netpfil/pf/pf.c

3974

I'm pretty sure that any async mechanism you'd use is going to be (a lot) more heavyweight than the branch, or maintaining per-CPU values.

The per-CPU values are almost certainly the way to go. We'd trade memory (but not a whole lot of it) for cache performance, and reading the rules is a relatively rare operation, so that being infinitesimally slower is not an issue.

[mjg]

sys/netpfil/pf/pf.c

3974

you can easily allocate it with uma_zalloc_pcpu. then you can blindly store it to your per-cpu var.

[linnemannr_gmail.com]

• pf: Add per-rule timestamps for rule and eth_rule
• pf: Switch timestamp to time_second and remove last_active from libpfctl structs
• pf: Make rule timestamps cpu local, update only on change

[linnemannr_gmail.com]

• pf: Make rule timestamps cpu local, update only on change

[linnemannr_gmail.com]

• pf: Make rule timestamps cpu local, update only on change

[linnemannr_gmail.com]

• pf: Remove no longer needed include of timeconv.h from libpfctl.c

[kp]

sys/net/pfvar.h

295

I'd be tempted to just initialise *__ts to 0 and then remove the cpu == 0 check.

sys/netpfil/pf/pf.c

3974

I don't think it's worth doing this check now that it's a per-cpu value. We can just assign it unconditionally.

sys/netpfil/pf/pf_ioctl.c

40

That should probably be '#include <vm/uma.h>` and be lower down in the include list.

2837

I don't think we need this M_ZERO.

[linnemannr_gmail.com]

• pf: Assign timestamp value unconditionally

[kp]

sys/net/pfvar.h

38

#include <sys/types.h> would be better, I think.
We'd usually avoid including headers starting with _.

sys/netpfil/pf/pf_ioctl.c

40

Still needs cleanup.

[linnemannr_gmail.com]

• pf: Assign timestamp value unconditionally

[linnemannr_gmail.com]

• pf: Protect timestamp assignment in critical section, fix null pointer dereference

[linnemannr_gmail.com]

Something is really wrong when I run the ether:anchor test, it immediately traps:

Fatal trap 9: general protection fault while in kernel mode
cpuid = 0; apic id = 00
instruction pointer     = 0x20:0xffffffff80deb3c0
stack pointer           = 0x28:0xfffffe00c5b45d40
frame pointer           = 0x28:0xfffffe00c5b45d50
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = resume, IOPL = 0
current process         = 11 (idle: cpu0)
trap number             = 9

[kp]

I'm going to propose a tweaked version.

[kp]

• fix crash (The default rule V_pf_default_rule is initialised separately, and did not have timestamp allocated)
• Only print the date, not the timestamp for users
• If the rule was never hit show 'N/A'