Created attachment 255466 [details] pfctl -T makezero patch There is a common pattern "keep an entry in pf table while it's active + TTL seconds more". This pattern is observed: > resetting the statistics for a single IP address in a table would allow me to > _continuously_ block repeat offenders, while releasing one-time offenders - #282877 > Is there a way to remove entries based on the last date accessed ? - https://forums.freebsd.org/threads/pf-firewall-expiretable.61827/ I need it for a policy-based routing based on a pf table that is filled with `unbound` ipset patch and is expired as soon as an address is silent for a while. I propose `makezero` command to pfctl that clears `pfras_tzero` for the entries with non-zero counters to implement that pattern. `pfctl -t tbl -T zero $ip1 $ip2 ...` is fine, but it means that "activity" is tracked somewhere else and this solution has it's pros and contras. - pflog might be dropping packets in case of consumer being somewhat slow - table counters are "unavoidable", but come with some performance penalty - both options are prone to TOCTOU race-condition "makezero" name combines semantics of `make` (doing things incrementally and only-as-necessary) and `zero` clearing statistics. :-) In this case the cronjob maintaining the table would be as simple as: > pfctl -t tbl -T makezero && pfctl -t tbl -T expire ${TTL} The patch depends on 6463b6b59152fb1695bbe0de78f6e2675c5a765a and https://reviews.freebsd.org/D47697
Couple initial remarks: - I dislike the name - it needs a test case - it needs a commit message - expand on "but come with some performance penalty"
(In reply to Kristof Provost from comment #1) I also dislike the name, but I failed to come up with a better one. I'd appreciate help here. "touch" is a bad one as it actually changes counters. "clear" sounds like an option, but might be confused with "flush", so IMO it's even worse than "makezero". "mark" might come from mark-n-sweep gc, but it's confusing in this context. "rearm" comes with watchdog/timer semantics that is kinda close, but still not 100% applicable. "reset" is almost the winner, but TCP has already taken the word for RST. I would say "reset" is my 2nd preferred option after "makezero". So I'm kinda out of reasonable options. > come with some performance penalty It's probably my mistake made under assumption that counter-aware tables have different memory layout and handling. I was unaware of pfr_get_astats() saying that > It was possible to have a table without per-entry counters. Now they are > always allocated, we just discard data when reading it if table is not > configured to have counters. ACK for tests & commit message. Will do.
Since I said in #282877 that I had some thoughts about this, I will share my grain of salt. But you guys should obviously feel free to do with it what you want. Honestly, I fail to see a use case here. As far as I can see, you have all the information and functionality to do what you want to do: `pfctl -t table -vT show' gives you the counter stats, and `pfctl -t table -T zero $ip' gives you the possibility to reset statistics for an IP address. It would be very simple to write a script to do this, which you can call from cron, followed by `pfctl -t table -T expire 12345' as you intended. Since you're not trying to do anything real-time, I don't see why you couldn't use the existing functionality. I would see the use for something real-time, but that should be in the kernel, and not in pfctl. If I could mark a table (or a rule, or the 'overload' feature) to reset statistics for IP addresses that hit the rule when they are in the table already, I would happily make use of that. But if it's going to be a cronjob anyway, then I think you have everything you need. But I may be missing something. Anyway, it's not my decision to make, so I'll let you guys to it. Have a nice day, Rob
Rob, thanks for your input. I completely agree with your reasoning, but I reach different conclusion starting with a similar idea. "expire" also falls under the category "one has all the information and functionality" combining "show" and "delete". Moreover, it's implemented in exactly that way: https://github.com/freebsd/freebsd-src/blob/435a5f94fbc09959c0a4e48b1e81a50fcfd45673/sbin/pfctl/pfctl_table.c#L259-L296 So, I concluded, if "expire" is eligible for pfctl as it codifies a helper for a reasonably popular policy, something like "makezero" is somewhat equally eligible. That's why I decided to submit a patch instead of making an ad-hoc for(;;){show|delete;sleep} script. I also agree, that it would be nice of kernel to keep track of time of last increment of a counter to make things more real-time (and, maybe, make it CLOCK_UPTIME_FAST instead of wall-clock), but I'm not in the position to develop alike patch as packet counters are on a critical path of network forwarding and I'm not capable of benchmarking the patch on a reasonably diverse set of hardware (L1/2/3 cache-sizes might matter) as alike patch may interfere with some structures being pushed out of CPU caches and introducing performance regression as a consequence.
Created attachment 255471 [details] Reset statistics for IP if counter In/Block > 0 Hi Leonid, I was not going to respond to this feature request anymore, but you're making me... :) I agree that 'expire' is also a superfluous function. But it can't be removed anymore, as too many people depend on it now. However, I don't think that that could/should be a reason to add more superfluous functionality. I've always been an advocate for lean and light software. Besides, the Unix way is to have small and simple building blocks, that the sysadmin/programmer/user ties together to obtain the result (s)he needs. The attached Bash script does what you need. I hereby release it into the public domain. Save it as /usr/local/sbin/pf-reset and make it executable. You will obviously need to have these patches for bug #282877 applied to your system: https://reviews.freebsd.org/D47698 https://reviews.freebsd.org/D47697 (They have already been committed to the main branch.) Call the script with a table name as first parameter, and the string 'noverify' as an optional second parameter. Without the 'noverify' parameter the script will display the IP addresses for which the statistics will be reset + the counter, and ask you if you want to continue. Examples: # pf-reset blocked # pf-reset blocked noverify && pfctl -t blocked -T expire 1209600 Have fun! Rob
Kristof, I'm asking for your verdict then as you're the maintainer. Should I proceed with making the patch commitable or should I drop it and settle for `-T zero $ip1 $ip2 $ip3` being good enough? Both options work for me. Rob, thanks. However, my router box complains about `bash: Command not found.` /etc/shells lists only sh, csh or tcsh. Kidding :-)
(In reply to Leonid Evdokimov from comment #6) I don't have any particularly strong opinions here. I'm unlikely to use either option myself. I will point out that even moving this operation entirely into the kernel doesn't make it race-free, because counters use counter_u64, which uses per-core counters, so there's never an atomic view of the actual counter value. It's inherently an approximate snapshot. So it's probably better to use the existing 'zero 1.2.3.4 5.6.7.8 ..' option.