Re: panic: knlist locked, but should not be

Fredrik Lindberg wrote this message on Mon, Jul 03, 2006 at 16:20 +0200:
> I'm hitting this "knlist locked"-panic when using BPF descriptors 
> together with kqueue/kevent.
> 
> panic: knlist locked, but should not be

Yeh, csjp broke it in v1.161 by adjusting the locking w/o fixing the
respective issues.

> #10 0xc051a1e3 in knlist_add (knl=0xc3b69460, kn=0xc3ad65d8, islocked=0)
>     at /usr/src/sys/kern/kern_event.c:1571
> #11 0xc059c5de in bpfkqfilter (dev=0x12, kn=0xc3ad65d8) at 
> /usr/src/sys/net/bpf.c:1194
> #12 0xc050d9e6 in giant_kqfilter (dev=0xc38f9d00, kn=0xc3ad65d8)
>     at /usr/src/sys/kern/kern_conf.c:336
> #13 0xc04e9580 in devfs_kqfilter_f (fp=0xc3ad40d8, kn=0xc3ad65d8)
>     at /usr/src/sys/fs/devfs/devfs_vnops.c:444
> #14 0xc0517e6d in filt_fileattach (kn=0x12) at file.h:305
> #15 0xc0518eb0 in kqueue_register (kq=0xc3923780, kev=0xe3a67b9c, 
> td=0xc3714a20, waitok=1)
>     at /usr/src/sys/kern/kern_event.c:902
> #16 0xc05185f4 in kern_kevent (td=0xc3714a20, fd=3, nchanges=1, 
> nevents=0, k_ops=0xe3a67c70,
>     timeout=0x0) at /usr/src/sys/kern/kern_event.c:640
> #17 0xc0518491 in kevent (td=0xc3714a20, uap=0xe3a67d04) at 
> /usr/src/sys/kern/kern_event.c:572
> 
> I thinks the problem is as follows, islocked is 0 which triggers
> the macro KNL_ASSERT_LOCK(knl, islocked);
> (It will only bite you if you run with INVARIANTS on)
> 
> The knlist is initialized with the same mutex that protects the
> rest of the bpf_d structure.
> bpfopen(), sys/net/bpf.c:383
> 
> 	knlist_init(&d->bd_sel.si_note, &d->bd_mtx, NULL, NULL, NULL);
> 
> In bpfkqfilter() and filt_bpfdetach() this mutex is acquired with
> BPFD_LOCK() before calling knlist_add()/knlist_remove()
> 
> 	#define BPFD_LOCK(bd)           mtx_lock(&(bd)->bd_mtx)
> 
> Hence, the knlist lock IS locked despite being called with islocked = 0.
> If using the same mutex doesn't cause any additional problems,
> (I haven't noticed anything yet, but there could of course be issues
> that I've overlooked), I think knlist_add should be called
> with islocked = 1. A suggested patch is attached.
> 
> 
> Fredrik Lindberg

> Index: bpf.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/net/bpf.c,v
> retrieving revision 1.168
> diff -u -r1.168 bpf.c
> --- bpf.c	15 Jun 2006 15:39:12 -0000	1.168
> +++ bpf.c	3 Jul 2006 11:36:30 -0000
> _at__at_ -1152,7 +1152,7 _at__at_
>  	d->bd_pid = curthread->td_proc->p_pid;
>  	kn->kn_fop = &bpfread_filtops;
>  	kn->kn_hook = d;
> -	knlist_add(&d->bd_sel.si_note, kn, 0);
> +	knlist_add(&d->bd_sel.si_note, kn, 1);
>  	BPFD_UNLOCK(d);
>  
>  	return (0);
> _at__at_ -1164,7 +1164,7 _at__at_
>  	struct bpf_d *d = (struct bpf_d *)kn->kn_hook;
>  
>  	BPFD_LOCK(d);
> -	knlist_remove(&d->bd_sel.si_note, kn, 0);
> +	knlist_remove(&d->bd_sel.si_note, kn, 1);
>  	BPFD_UNLOCK(d);
>  }

Why not drop the lock lines and keep the 0?  As you said since it's
the same lock, locking it a bit later won't hurt...

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."