On Mon, Dec 05, 2022 at 11:26:21AM +0100, Alexandr Nedvedicky wrote:
> Hello,
> On Mon, Dec 05, 2022 at 11:46:07AM +1000, David Gwynne wrote:
> >
> > yes, you're right. the diff below includes the simple fix to that.
> >
> > this demonstrates how subtle the reference counting around the trees
> > is though. maybe it would be more obvious to take another refcnt
> > before giving the pf_state_key to the tree(s), and then give the
> > pf_state_key_attach callers reference to the pf_state struct. at
> > the moment we give the callers reference to the tree while holding
> > the PF_LOCK, and borrow it to give another one to the state while
> > the lock is still held.
>
> Either way is fine in my opinion.
>
> </snip>
> > @@ -766,44 +777,53 @@ 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)) {
> > + KASSERT(cur == NULL);
> > + RB_REMOVE(pf_state_tree, &pf_statetbl, sk);
> > + sk->removed = 1;
> > + pf_state_key_unref(sk);
> > + }
> > +
> > + return (NULL);
> > }
>
> I like your fix above.
>
> I also wonder if pf_state_insert() should be always setting *skwp, *sksp to
> NULL. Just to indicate to caller the 'reference ownership got transferred' and
> there is nothing left to worry about. This can be done right at the beginning
> of pf_state_insert() after we assign to local variables skw/sks. Just a
> thought/nit.
pf_test_rule uses the skw and sks pointers after pf_state_insert sets
them via pf_create_state. i would happily change pf_test rule so it
reads the pf_state_key pointers out of pf_state rather than carry them
around on the stack like that, but i figured the diff was big enough as
it was.
> the recent diff is OK by me.
thanks. i'll wait until i can be around to look after it before i commit
it.
