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.
Created attachment 255628 [details] pfctl: add -T reset command Hello Kristof, I'm adding the updated patch with better commit message & tests. While I agree that `-T zero ${ip}` is the right idea, I believe that `-T expire` might deserve both `zero ${ip}` and `reset` to be available as soon as timestamp is tracked in the kernel... unless there is a plan to obsolete `pfras_tzero` :-) Another option I can think of is to implement machine-readable TSV and/or json output for `-T show` to replace regexp-based parsing with `awk` or jq`.
(In reply to Leonid Evdokimov from comment #8) > Another option I can think of is to implement machine-readable TSV and/or json output for `-T show` to replace regexp-based parsing with `awk` or jq`. I'd quite like to see libxo (https://wiki.freebsd.org/LibXo) support for that short of thing. I'm not sure if it's reasonable to add that to pfctl itself or not. It may or may not be a better idea to have a standalone tool for table management grow that support. A number, but not all, of the pf ioctl calls (and now also netlink calls) have been moved to libpfctl, making this work easier. I still intend to move all of them into libpfctl in due course.
(In reply to Kristof Provost from comment #9) Libxo? Yes, please! That's exactly how CLI glue should be done in the better world! I've completely forgotten about libxo. Thanks for reminder! *excited* I'd be glad to make an attempt to contribute pfctl+libxo patch in a few months after solving some other problems in my backlog. Meanwhile, I'll be waiting for verdict on `-T reset` patch. My opinion is that it's "as good as -T expire is" and that it makes the feature somewhat more "complete", but I'm obviously biased here.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5b59b0c61e29f684a019afdd2848ffe2d5604e0c commit 5b59b0c61e29f684a019afdd2848ffe2d5604e0c Author: Leonid Evdokimov <leon+freebsd@darkk.net.ru> AuthorDate: 2024-12-06 12:08:54 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-12-09 09:36:34 +0000 pfctl: add -T `reset` to touch pfras_tzero only for non-zero entries This will make it easier for scripts to detect idle hosts in tables. PR: 282984 Reviewed by: kp MFC after: 2 weeks sbin/pfctl/pfctl.8 | 7 +++- sbin/pfctl/pfctl.c | 2 +- sbin/pfctl/pfctl_radix.c | 2 +- sbin/pfctl/pfctl_table.c | 44 ++++++++++++++++++++++++ tests/sys/netpfil/pf/table.sh | 80 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 3 deletions(-)
(In reply to Leonid Evdokimov from comment #10) I've pushed your patch with minor tweaks to fix whitespace issues, and to reword the man page slightly. Thanks!
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3fa5d13c5be01da5796e0f3617d6da277cc16432 commit 3fa5d13c5be01da5796e0f3617d6da277cc16432 Author: Leonid Evdokimov <leon+freebsd@darkk.net.ru> AuthorDate: 2024-12-06 12:08:54 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-12-24 10:16:50 +0000 pfctl: add -T `reset` to touch pfras_tzero only for non-zero entries This will make it easier for scripts to detect idle hosts in tables. PR: 282984 Reviewed by: kp MFC after: 2 weeks (cherry picked from commit 5b59b0c61e29f684a019afdd2848ffe2d5604e0c) sbin/pfctl/pfctl.8 | 7 +++- sbin/pfctl/pfctl.c | 2 +- sbin/pfctl/pfctl_radix.c | 2 +- sbin/pfctl/pfctl_table.c | 44 ++++++++++++++++++++++++ tests/sys/netpfil/pf/table.sh | 80 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 3 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=30b9d8a73721ed762aaf02244e9429ed5fac7142 commit 30b9d8a73721ed762aaf02244e9429ed5fac7142 Author: Leonid Evdokimov <leon+freebsd@darkk.net.ru> AuthorDate: 2024-12-06 12:08:54 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-12-24 10:16:53 +0000 pfctl: add -T `reset` to touch pfras_tzero only for non-zero entries This will make it easier for scripts to detect idle hosts in tables. PR: 282984 Reviewed by: kp MFC after: 2 weeks (cherry picked from commit 5b59b0c61e29f684a019afdd2848ffe2d5604e0c) sbin/pfctl/pfctl.8 | 7 +++- sbin/pfctl/pfctl.c | 2 +- sbin/pfctl/pfctl_radix.c | 2 +- sbin/pfctl/pfctl_table.c | 44 ++++++++++++++++++++++++ tests/sys/netpfil/pf/table.sh | 80 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 3 deletions(-)
Created attachment 256136 [details] pf (tests): Set cleared time when zeroing stats for table addresses Hi Kristof, I see that the patch (as well as the one for bug #282877) landed in stable/14, thanks for that. Also the corresponding kernel fix (https://reviews.freebsd.org/D47697) is missing from stable/14 to the best of my understanding. So I'm attaching the patch with the test-case you requested in D47697.
(In reply to Leonid Evdokimov from comment #15) I've merged Kajetan's fix to stable/14 and stable/13. Can you post that patch in Phabricator? I'm always happy to see more tests, but I think I have a few remarks and that's easier to discuss in Phabricator than here.
(In reply to Kristof Provost from comment #16) Sure! I've never used Phabricator before and I'm not 100% what's the workflow there, but I tried to do my best per Committer's Guide. Here are the tests: https://reviews.freebsd.org/D48242
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0749d8134300b8e3c956e161890ab496247d2542 commit 0749d8134300b8e3c956e161890ab496247d2542 Author: Leonid Evdokimov <leon@darkk.net.ru> AuthorDate: 2025-01-02 09:30:06 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-01-02 12:34:50 +0000 pf tests: check cleared time when zeroing stats for table addresses Verify that we reset the cleared time when we zero an address' counters in a table. PR: 282877, 282984 Reviewed by: kp MFC after: 2 weeks Signed-off-by: Leonid Evdokimov <leon@darkk.net.ru> Differential Revision: https://reviews.freebsd.org/D48242 tests/sys/netpfil/pf/table.sh | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=635c2b82f60e7417746a03d4d22c7fafd08f7e7c commit 635c2b82f60e7417746a03d4d22c7fafd08f7e7c Author: Leonid Evdokimov <leon@darkk.net.ru> AuthorDate: 2025-01-02 09:30:06 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-01-16 08:42:04 +0000 pf tests: check cleared time when zeroing stats for table addresses Verify that we reset the cleared time when we zero an address' counters in a table. PR: 282877, 282984 Reviewed by: kp MFC after: 2 weeks Signed-off-by: Leonid Evdokimov <leon@darkk.net.ru> Differential Revision: https://reviews.freebsd.org/D48242 (cherry picked from commit 0749d8134300b8e3c956e161890ab496247d2542) tests/sys/netpfil/pf/table.sh | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3870483ec496544c5c1d1b29903cb2ca6872646d commit 3870483ec496544c5c1d1b29903cb2ca6872646d Author: Leonid Evdokimov <leon@darkk.net.ru> AuthorDate: 2025-01-02 09:30:06 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-01-16 15:20:34 +0000 pf tests: check cleared time when zeroing stats for table addresses Verify that we reset the cleared time when we zero an address' counters in a table. PR: 282877, 282984 Reviewed by: kp MFC after: 2 weeks Signed-off-by: Leonid Evdokimov <leon@darkk.net.ru> Differential Revision: https://reviews.freebsd.org/D48242 (cherry picked from commit 0749d8134300b8e3c956e161890ab496247d2542) tests/sys/netpfil/pf/table.sh | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)