From owner-freebsd-current@FreeBSD.ORG Mon Jul 3 18:37:21 2006 Return-Path: X-Original-To: freebsd-current@freebsd.org Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5209316A528; Mon, 3 Jul 2006 18:37:21 +0000 (UTC) (envelope-from jmg@hydrogen.funkthat.com) Received: from hydrogen.funkthat.com (gate.funkthat.com [69.17.45.168]) by mx1.FreeBSD.org (Postfix) with ESMTP id 42EE844B0D; Mon, 3 Jul 2006 18:14:12 +0000 (GMT) (envelope-from jmg@hydrogen.funkthat.com) Received: from hydrogen.funkthat.com (ycmrx6ikrvo14wwc@localhost.funkthat.com [127.0.0.1]) by hydrogen.funkthat.com (8.13.6/8.13.3) with ESMTP id k63IEBri067413; Mon, 3 Jul 2006 11:14:11 -0700 (PDT) (envelope-from jmg@hydrogen.funkthat.com) Received: (from jmg@localhost) by hydrogen.funkthat.com (8.13.6/8.13.3/Submit) id k63IE8iD067412; Mon, 3 Jul 2006 11:14:08 -0700 (PDT) (envelope-from jmg) Date: Mon, 3 Jul 2006 11:14:08 -0700 From: John-Mark Gurney To: Fredrik Lindberg Message-ID: <20060703181408.GB734@funkthat.com> Mail-Followup-To: Fredrik Lindberg , freebsd-current@freebsd.org, "Christian S. J. Peron" References: <44A927AC.7080807@shapeshifter.se> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <44A927AC.7080807@shapeshifter.se> User-Agent: Mutt/1.4.2.1i X-Operating-System: FreeBSD 5.4-RELEASE-p6 i386 X-PGP-Fingerprint: B7 EC EF F8 AE ED A7 31 96 7A 22 B3 D8 56 36 F4 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html Cc: freebsd-current@freebsd.org, "Christian S. J. Peron" Subject: Re: panic: knlist locked, but should not be X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: John-Mark Gurney List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jul 2006 18:37:21 -0000 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 > @@ -1152,7 +1152,7 @@ > 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); > @@ -1164,7 +1164,7 @@ > 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."