Re: IPFilter issue in -current

[Geoff Adams <gadams+netbsd%avernus.com@localhost>]

On Dec 22, 2012, at 11:05 PM, Darren Reed <darrenr%netbsd.org@localhost> wrote:

> 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.

But it's still allocated as a lock. Running the kernel with LOCKDEBUG enabled 
picks this up, and dutifully panics when the pointer containing an allocated 
(but inactive, in this case) lock is freed.

> 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.

I moved that based on these comments in ipf_ht_node_add and ipf_ht_node_del:

/* NOTE: THIS FUNCTION MUST BE CALLED WITH AN EXCLUSIVE LOCK THAT PREVENTS  */
/*       ipf_ht_node_del FROM RUNNING CONCURRENTLY ON THE SAME htp.         */

The call to ipf_ht_node_add on line 1078 in ip_state.c seems to be protected by 
is->is_lock, and so it seemed that that was the correct lock to use to 
guarantee exclusivity with ipf_ht_node_del, which moving the lock exit down a 
few lines accomplishes. I didn't notice the locks on ipf_state being held a 
couple function calls above.

So, with a read lock on ipf_state, multiple calls to ipf_ht_node_add could 
still occur simultaneously, right? Would that be a potential problem? Or would 
that never occur in the same rb tree?

> 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.

You're very likely correct. My locking changes didn't seem sufficient to cure 
the hangs and panics. Combined with the rb tree swap, it seems happy. I didn't 
go back and undo that latter locking change to see whether it was necessary, 
since it seemed correct to me, anyway.

About the RB tree bug--I posted some stack traces as best as I could get them 
earlier in this thread. Here's one, for instance:

Nov 21 00:20:57 tho /netbsd: fatal page fault in supervisor mode
Nov 21 00:20:57 tho /netbsd: trap type 6 code 0 rip ffffffff8018a269 cs 8 
rflags 10207 cr2 21 ilevel 4 rsp fffffe8002d03460
Nov 21 00:20:57 tho /netbsd: curlwp 0xfffffe803fe29420 pid 0 lid 3 lowest 
kstack 0xfffffe8002d00000
Nov 21 00:20:57 tho /netbsd: panic: trap
Nov 21 00:20:57 tho /netbsd: cpu0: Begin traceback...
Nov 21 00:20:57 tho /netbsd: printf_nolog() at netbsd:printf_nolog
Nov 21 00:20:57 tho /netbsd: startlwp() at netbsd:startlwp
Nov 21 00:20:57 tho /netbsd: alltraps() at netbsd:alltraps+0x9e
Nov 21 00:20:57 tho /netbsd: ipf_ht_node_add() at netbsd:ipf_ht_node_add+0x3b
Nov 21 00:20:57 tho /netbsd: ipf_state_insert() at netbsd:ipf_state_insert+0x1be
Nov 21 00:20:57 tho /netbsd: ipf_state_add() at netbsd:ipf_state_add+0xb3f
Nov 21 00:20:57 tho /netbsd: ipf_scanlist() at netbsd:ipf_scanlist+0x296
Nov 21 00:20:57 tho /netbsd: ipf_scanlist() at netbsd:ipf_scanlist+0x24a
Nov 21 00:20:57 tho /netbsd: ipf_check() at netbsd:ipf_check+0x637
Nov 21 00:20:57 tho /netbsd: pfil_run_hooks() at netbsd:pfil_run_hooks+0x9d
Nov 21 00:20:57 tho /netbsd: ip6_input() at netbsd:ip6_input+0x3d9
Nov 21 00:20:57 tho /netbsd: ip6intr() at netbsd:ip6intr+0x77
Nov 21 00:20:57 tho /netbsd: softint_dispatch() at netbsd:softint_dispatch+0xd9
Nov 21 00:20:57 tho /netbsd: cpu0: End traceback...

That really doesn't seem to help much, though. Besides the lack of source lines 
(and I wasn't able to get any using kernel gdb, either). I'm a little surprised 
why stack frames don't appear for the rb functions; even though they're 
implemented as macros, the macros define proper functions. Perhaps they get 
inlined away.

Do you have unit tests for the RB tree code that stress test it?

> 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.

Yeah, it took some digging to figure out what was going on. :)

Thanks a lot for looking at my patch!

- Geoff