Date: Wed, 27 Jul 2005 22:35:34 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Craig Rodrigues <rodrigc@crodrigues.org> Cc: freebsd-current@freebsd.org, Harti Brandt <harti@freebsd.org> Subject: Re: Panic in netnatm Message-ID: <20050727223351.A54330@fledge.watson.org> In-Reply-To: <20050726145334.GA1826@crodrigues.org> References: <20050726124222.GA1109@crodrigues.org> <20050726131712.GC46538@darkness.comp.waw.pl> <20050726145334.GA1826@crodrigues.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050727223351.A54330>