Re: IPFilter issue in -current

[christos%astron.com@localhost (Christos Zoulas)]

In article <4A50BE64-5828-4D4E-9B28-0DA681199B25%avernus.com@localhost>,
Geoff Adams  <gadams+netbsd%avernus.com@localhost> wrote:
>On Nov 27, 2012, at 6:15 AM, Christos Zoulas <christos%astron.com@localhost> 
>wrote:
>
>> Add some printf("%s, %d\n", __FILE__, __LINE__); in ipf_ht_node_{add,del}
>> to find out exactly where it crashes....
>
>Good thinking.
>
>I was surprised at how seldom these functions are actually called.
>Still, I've managed to catch a hang occurring somewhere in these lines
>in ipf_ht_node_del:

Heh, I was doing the same thing a few days ago in kevent1() :-)

>I also noticed that both ipf_ht_node_{add,del} include a comment to this 
>effect:
>
>/* NOTE: THIS FUNCTION MUST BE CALLED WITH AN EXCLUSIVE LOCK THAT PREVENTS  */
>/*       ipf_ht_node_add FROM RUNNING CONCURRENTLY ON THE SAME htp.         */
>
>Yet I note that ipf_state_insert and ipf_state_del in ip_state.c don't
>ensure that a lock wraps the call to ipf_ht_node_del. I suspect that
>this is the heart of the problem. That would presumably cause deadlock
>or crash in the red-black tree insertion and removal code.
>
>I'll try changing the locking so that ipf_ht_node_del is called within
>the hold on is->is_lock.
>
>(And, as I was about to send this message, I just got a panic during the
>call to RBI_SEARCH in the same function listed above.)

Sounds good to me. If it is called infrequently there is a good chance
that you are dealing with a race. By adding printfs you are probably
exacerbating the problem, making it easier to debug.

christos