Re: Panic in netnatm

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;
> _at__at_ -252,9 +256,11 _at__at_
> 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
> _at__at_ -269,10 +275,12 _at__at_
> 		 * 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_at_crodrigues.org
>