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