Hello,
</snip>
new diff still looks good to me. I could just catch typos
in comment.
>
> I've been running versions of this diff in production at work, and have
> hit a few panics and asserts. All the issues we've hit should be
> addressed in this diff.
>
> The first issue was that pfsync could be in the processing of sending a
> deferred packet while it's being removed from the state tree. Part of
> that removal process is stripping the state keys from the state, which
> pfsync uses to determine the address family so it knows which ip output
> routine to use. The quick and dirty fix to this is to have pfsync check
> if timeout state to see if the state is unlinked or not. This currently
> relies on pfsync undefer and pf being serialised by the NET_LOCK.
>
> The second is that the timeout member on a state can change while the
> purge task is looking at it. We hit this assert in pf_state_expires:
>
> KASSERT(state->timeout != PFTM_UNLINKED);
>
> pf_state_expires was called from the purge code like this:
>
> if ((cur->timeout == PFTM_UNLINKED) ||
> (pf_state_expires(cur) <= getuptime()))
> SLIST_INSERT_HEAD(&gcl, cur, gc_list);
>
I see. I've missed those in earlier review. the pf(4) we have
in tree uses PF_STATE_ lock to keep cur->timeout consistent.
pf_purge_expired_states() executes the check under PF_STATE_ENTER_READ().
>
> With my new locking scheme here, the state purge code is called without
> any of the locks that would serialise access the state->timeout
> variable. I think I found a solution to this without having to
> reintroduce extra locking, which should allow us to keep the purge scan
> running concurrently with pf actually handling packets.
I think it should work, because we make sure pf_remove_state()
gets called only once.
I've found two typos in comment, see below. otherwise looks good.
OK sashan
> + /*
> + * pf_state_expires is used by the state purge task to
> + * decide if a state is a candidate for cleanup, and by the
> + * pfsync state export code to populate an expiry time.
> + *
> + * this function may be called by the state purge task while
> + * the state is being modified. avoid inconsistent reads of
> + * state->timeout by having the caller do the read (and any
> + * chacks it needs to do on the same variable) and then pass
s/chacks/checks
> + * their view of the timeout in here for this function to use.
> + * the only consequnce of using a stale timeout value is
s/consequnce/consequence
OK sashan
