From owner-freebsd-current@FreeBSD.ORG Wed Jul 27 21:34:16 2005 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 38D8816A41F; Wed, 27 Jul 2005 21:34:16 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [204.156.12.53]) by mx1.FreeBSD.org (Postfix) with ESMTP id 965BA43D49; Wed, 27 Jul 2005 21:34:15 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by cyrus.watson.org (Postfix) with ESMTP id 08C0446B09; Wed, 27 Jul 2005 17:34:11 -0400 (EDT) Date: Wed, 27 Jul 2005 22:35:34 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Craig Rodrigues In-Reply-To: <20050726145334.GA1826@crodrigues.org> Message-ID: <20050727223351.A54330@fledge.watson.org> References: <20050726124222.GA1109@crodrigues.org> <20050726131712.GC46538@darkness.comp.waw.pl> <20050726145334.GA1826@crodrigues.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-current@freebsd.org, Harti Brandt Subject: Re: Panic in netnatm X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Jul 2005 21:34:16 -0000 On Tue, 26 Jul 2005, Craig Rodrigues wrote: > On Tue, Jul 26, 2005 at 03:17:12PM +0200, Pawel Jakub Dawidek wrote: >> +> panic: mutex natm_mtx not owned at /usr/src/sys/netnatm/natm_pcb.c:110 >> >> Trace will probably not be needed, as there are only two places where >> npcb_add() is called. >> It looks like NATM locking is missing in /sys/netinet/if_atm.c. > > Ahh, thanks Pawel! You saved me some time. It looks like locks must be > held before npcb_free() and npcb_add() are called. It looks like Harti > is on vacation, so can you help Robert? Is something like this needed? Sorry about the slow response -- I've been offline the last two days on a trip. > + NATM_LOCK(); > npcb = npcb_add(NULL, rt->rt_ifp, op.param.vci, op.param.vpi); > - if (npcb == NULL) > + if (npcb == NULL) { > + NATM_UNLOCK(); > goto failed; > + } I think it would be desirable not to unlock here, instead holding the lock through the 'failed' case below in order to avoid re-acquiring it and allowing other threads to gain access to the npcb. > npcb->npcb_flags |= NPCB_IP; > npcb->ipaddr.s_addr = sin->sin_addr.s_addr; > + NATM_UNLOCK(); > /* XXX: move npcb to llinfo when ATM ARP is ready */ > rt->rt_llinfo = (caddr_t) npcb; > rt->rt_flags |= RTF_LLINFO; > @@ -252,9 +256,11 @@ > failed: > #ifdef NATM > if (npcb) { > + NATM_LOCK(); I.e., not re-acquire here. > npcb_free(npcb, NPCB_DESTROY); > rt->rt_llinfo = NULL; > rt->rt_flags &= ~RTF_LLINFO; > + NATM_UNLOCK(); > } And move the unlock macro here outside of the block so it's always called. Otherwise, this looks good to me! (And I guess neither Harti or Bruce got a chance to test these code paths?) Robert N M Watson > #endif > /* mark as invalid. We cannot RTM_DELETE the route from > @@ -269,10 +275,12 @@ > * tell native ATM we are done with this VC > */ > if (rt->rt_flags & RTF_LLINFO) { > + NATM_LOCK(); > npcb_free((struct natmpcb *)rt->rt_llinfo, > NPCB_DESTROY); > rt->rt_llinfo = NULL; > rt->rt_flags &= ~RTF_LLINFO; > + NATM_UNLOCK(); > } > #endif > /* > > > > -- > Craig Rodrigues > rodrigc@crodrigues.org >