pf tables locking

[Kristof Provost]

On 13 Aug 2018, at 17:06, Kajetan Staszkiewicz wrote:
> On Monday, 13 August 2018 15:22:33 CEST Kristof Provost wrote:
>> 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.
>
pf keeps rules around until there are no more states left referencing the rule.
Look at pf_commit_rules(): The old rules are unlinked rather than removed. They’re kept on the V_pf_unlinked rules list.
Every so often pf runs through all states (in pf_purge_thread()) to mark their associated rules as still referenced. Only rules which are not referenced by any state are removed.

This means that while there’s still a state which was created by the rule (and can thus put packets towards its table), the rule will exist.
Once the state goes away it’ll still take one full iteration through all states before the rule can be freed.
Hence my statement that it’s highly unlikely (pretty much impossible) for us to run into a situation where the rule no longer exists.


>> 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.
>
Do you have a bit more information about your use case? What are you trying to accomplish with this change?

> 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.
>
Yeah, that bug is still on my todo list somewhere, but things are extremely hectic at the moment, and I can’t make any promises about when I’ll have time for it.

Regards,
Kristof
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 549 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-pf/attachments/20180813/6524abc2/attachment.sig>