On Monday, 13 August 2018 15:22:33 CEST Kristof Provost wrote: > > I'm going through the code and I've found out that many table-related > > function > > are guarded by lock on pf ruleset. But that is not true for > > pfr_update_stats. > > This function is called from pf_test only after PF_RULES_RUNLOCK(). > > I think you’re right, this does look wrong. > > It’s very unlikely that this will actually lead to a crash, because
I don't like the word "unlikely". With my traffic and frequent ruleset and carp changes I'm catching all the fanciest locking bugs as it seems. > rules (and associated tables) won’t just go away while there’s still > state, This is mostly what I wanted to ask about in this message. How is it ensured that table and counters are gone only after everybody stops using them? What if I delete a table, then change ruleset, but there is still active connection keeping a state? I really had hard time finding how this is guarded in source. > but we could theoretically lose memory (in the pfrke_counters > allocation), and miscount. Pre-allocating counters seems a good idea, it will simplify some other code. > I don’t want to re-take the rules lock for this, so my current > thinking is that the best approach would be to already get rid of the > potential memory leak by just always allocating the pfrke_counters when > the table is created (i.e. when the rule is first set). That might waste > a little memory if we didn’t need it, but it should simplify things a > bit. > We can resolve the counting issue by using the counter_u64_*() functions > for them. We should be able to get away with not locking this. Sure, I can use counter(9). The question, as always with my patches, is what can go to FreeBSD and what won't go. My current goal is to modify round-robin pf target to always point to table entry with least amount of states. As I see it for now: 1. Modify pfrke_counters to be always allocated. 2. Rewrite pfrke_counters to use counter(9). 3. Provide state counter in pfrke_counters. 4. Modify round-robin target. 1. and 2. make a good PR. I'm not sure about 3. Do you want patches for least- connections target too? I want to just replace existing round-robin but if there is any chance of getting it into kernel code, I could make it work as new target in pf.conf. Point 3. is the puzzle for me. For now I just call pfr_update_stats (modified to handle state counter) in pf_create_state and pf_unlink_state. But again - how do I know if the table (I added a pointer in struct pf_state) is still allocated in memory? There are some more issues I found around pf_map_addr. Some of them I mentioned in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229092. Some more came out while working on this least-states loadbalancing. I will group them into something meaningful and make another PR for them. -- | pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS | | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net | | Vegeta | www: http://vegeta.tuxpowered.net | `------------------------^---------------------------------------'
signature.asc
Description: This is a digitally signed message part.