Created attachment 196197 [details]
Use counter(9) in pf tables.
Counters of pf tables are updated out of rule lock. That means state updates might overwrite eachother. Furthermore there is allocation and freing counters happening in lockless manner too.
Proposed patch modifies table counters behaviour. counter(9) facility is used and table element counters are always allocated so that race conditions are impossible now.
It seems you don't check the result of counter allocation, that with M_NOWAIT can fail. And then you are doing an access to such pointers. I'm not familiar with PF, but what happens if you try to limit UMA zone used for these counters and try to create enough number of entries? I suspect it will just panic. Also, PCPU counters are very expensive memory consumers, on modern machines with tens CPU cores, they require a lot of memory. And tables usually used to keep large number of entries, at least for ipfw. Is it really needed feature for PF for such cost?
It's a tradeoff. pfr_update_stats() is currently called without any relevant locks held, so there's a risk of both a memory leak and incorrect counting.
Using PCPU counters (and always allocating them) mitigates this.
One alternative would be to take the rules lock, which is usually used to protect tables, but we'd have to take it for writing, to ensure no other threads are updating the counters at the same time, which I would expect to be devastating for throughput.
We might be able to get away with a per-table (but there are throughput concerns for that too), or even per pfr_kentry lock, but the locking structure of pf is already complex, and I'm not immediately clear on how it would interact with the rest of the locking.
Andrey, you are right about allocation. I will change it to M_WAITOK just as other counters in pf are done.
Created attachment 196214 [details]
Use counter(9) in pf tables.
Updated version of patch using M_WAITOK.
Apologies for taking this long to get back to this. I've had other priorities in the past few months.
I'm not sure about the M_WAITOK in pfr_create_kentry(), because the initial allocation there (for the pfr_kentry) is M_NOWAIT. It'll have to be another M_NOWAIT allocation, with appropriate error handling.
The same applies to pfr_create_ktable().
In 'case PFRW_GET_ASTATS' it might make sense to move that code into its own function, if only to avoid the line length issues. It's so broken up now that it's not very readable any more.
Other than those minor points, I think this is ready to go in.
I totally forgot about this patch too :) I'll fix the memory allocation flag and run it in testing environment and come back to you in a few days.
Patch posted for review: https://reviews.freebsd.org/D19558
A commit references this bug:
Date: Fri Mar 15 11:08:45 UTC 2019
New revision: 345177
pf :Use counter(9) in pf tables.
The counters of pf tables are updated outside the rule lock. That means state
updates might overwrite each other. Furthermore allocation and
freeing of counters happens outside the lock as well.
Use counter(9) for the counters, and always allocate the counter table
element, so that the race condition cannot happen any more.
Submitted by: Kajetan Staszkiewicz <firstname.lastname@example.org>
Reviewed by: glebius
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D19558