On Mon, Dec 05, 2022 at 12:45:29AM +0100, Alexandr Nedvedicky wrote:
> 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.
ah, yes. lemme think about this a sec.
>
> otherwise the diff looks good to me.
>
> thanks and
> regards
> sashan
