Re: IPFilter issue in -current

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

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:

int
ipf_ht_node_del(host_track_t *htp, int family, i6addr_t *addr)
{
        host_node_t *h;
        host_node_t k;

        ipf_ht_node_make_key(htp, &k, family, addr);

        h = RBI_SEARCH(ipf_rb, &htp->ht_root, &k);
        if (h == NULL) {
                return -1;
        } else {
                h->hn_active--;
                if (h->hn_active == 0) {
      === AFTER THIS LINE ===
                        (void) RBI_DELETE(ipf_rb, &htp->ht_root, h);
                        htp->ht_cur_nodes--;
                        KFREE(h);
                }
        }
      === BEFORE THIS LINE ===

        return 0;
}

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

- Geoff