Re: IPFilter issue in -current

[Darren Reed <darrenr%netbsd.org@localhost>]

ok, now that I've studied the patches...

The change to ip_nat.c (adding a MUTEX_DESTROY()) is a no-op.

The reason for this is that when ipf_nat_insert() fails (as is
with the case for which you are adding code), it is assumed that
the "nat" pointer can be free'd without any further action. So
calling MUTEX_DESTROY() there serves no purpose as nothing is
going to touch that lock again.

The change to ip_state.c that moves MUTEX_EXIT(&is->is_lock)
I'm also unsure of. The code below that MUTEX_EXIT() is always
going to be running solo - nothing else will be using that
ipstate_t structure as is_ref == 0.  The line that has
"is->is_ref = 0" should be replaced with an ASSERT around
that statement. That leaves us with how is fr_srctrack
guarded. When ipf_state_insert() is called, a read lock
on ipf_state is held and ipf_stinsert is held across the
call to ipf_ht_node_add(). When ipf_state_del() is called,
a write lock on ipf_state must be held. This should mean
that there are no modification operations on the rbtree
that happen in a manner that does not guarantee the caller
exclusivity.

So the bugs being fixed are most likely due to the actual RB
tree code being better rather than any of the locking changes
you made to locking. That is unless I'm missing something.

If there's one thing I really hate about how the IPFilter
design has progressed it is the number of locks and the
locking complexity. It needs to be simpler. Much simpler.
But that probably requires rebuilding the data model.

Darren