Hello,
On Sat, Dec 03, 2022 at 09:53:45AM +1000, David Gwynne wrote:
> we (mostly sashan@ and me) have a problem where pfsync can be holding a
> reference to a pf_state that pf has decided to purge, and then pfsync
> crashes because it tries to read the pf_state_key parts of the state,
> but they been removed by the purge process.
>
> the background to this is that pf_state structures don't contain ip
> addresses, protocols, ports, etc. that information is stored in a
> pf_state_key struct, which is used to wire a state into the state table.
> when pfsync wants to export information about a state, particularly the
> addresses on it, it needs the pf_state_key struct to read from.
>
> the pf purge code current assumes that when a state is removed from the
> state table, the pf_state_key structs that are used to wire it in can
> also be removed and freed. this has obvious bad consequences for pfsync,
> which could still be holding a reference to the pf_state struct with the
> intention of reading from it.
>
> this diff tweaks the lifetime of pf_state_structs so they at least match
> the lifetime of pf_states.
>
> just so i'm clear, the main idea is that once pf_state_insert succeeds,
> a pf_state struct will always have pf_state_key structs hanging off it.
> it's only after the pf_state struct itself is being torn down that its
> references to pf_state_keys are released. if you're holding a
> pf_state reference, you can use that to access the pf_state_key
> structs hanging off it.
>
> this is largely accomplished by taking more advantage of the pf_state
> and pf_state_key refcnts. pf_states get properly accounted references to
> the pf_state_keys, and the state table gets its own reference counts to
> the pf_state_keys too. when a state is removed from the state table, the
> state table reference counts drop. however, it's not until all pf_state
> references drop that pf_state will give up its references to the
> pf_states.
>
> this is a very new diff, but i've been kicking it around with pfsync a
> bit and the pf_state_export crashes i used to get are gone now.
>
> if you're going to test the things to look for are if NAT still works,
> and if the pfstate and pfstkey pool numbers still look right.
all above makes sense to me. I'm fine with the approach.
I think I could just one single glitch here in pf_state_key_attach():
>
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1156
> diff -u -p -r1.1156 pf.c
> --- pf.c 25 Nov 2022 20:27:53 -0000 1.1156
> +++ pf.c 2 Dec 2022 23:20:36 -0000
</snip>
> @@ -766,44 +777,52 @@ pf_state_key_attach(struct pf_state_key
> /* remove late or sks can go away */
> olds = si->s;
> } else {
> - pool_put(&pf_state_key_pl, sk);
> - return (-1); /* collision! */
> + pf_state_key_unref(sk);
> + return (NULL); /* collision! */
> }
> }
> - pool_put(&pf_state_key_pl, sk);
> - s->key[idx] = cur;
> - } else
> - s->key[idx] = sk;
> + }
> +
> + /* reuse the existing state key */
> + pf_state_key_unref(sk);
> + sk = cur;
> + }
>
> if ((si = pool_get(&pf_state_item_pl, PR_NOWAIT)) == NULL) {
> - pf_state_key_detach(s, idx);
> - return (-1);
> + if (TAILQ_EMPTY(&sk->states)) {
> + RB_REMOVE(pf_state_tree, &pf_statetbl, sk);
> + sk->removed = 1;
> + }
> +
> + pf_state_key_unref(sk);
I think pf_state_key_unref() above needs to happen if and only if
sk != cur.
otherwise the diff looks good to me.
thanks and
regards
sashan
