pf tables locking

[Kajetan Staszkiewicz]

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     |
`------------------------^---------------------------------------'
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freebsd.org/pipermail/freebsd-pf/attachments/20180813/38f99bd4/attachment.sig>