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.
the recent diff is OK by me.
sashan
