git: 9front

Download patch

ref: 3faa3dbcef3f2ece049afba27d7222c79f600eba
parent: 8bb3720b37e675e7ce0b49a045e5bd722b191403
author: cinap_lenrek <cinap_lenrek@felloff.net>
date: Tue Aug 12 17:35:31 EDT 2014 

ip: fix missed unlocks and waserror handlers

ipifcunbind() could error out from ipifcremlifc() and Medium.unbind()
*after* decrementing ifc->conv->inuse! move the decrement after
calling these functions.

make ipifcremlifc() never raise error but return error string.
the only places where it could error is when it calls into
medium functions like Medium.remroute() and Medium.remmulti().
Ignore these errors as they could happen when the ethernet driver
crashed (think imported ethernet device or usb ethernet
in userspace), so we will be able to unbind.

add waserror() handlers as neccesary to deal with errors from
Medium.addmulti(), Medium.areg() and arpenter() to properly
unlock the data structures.


--- a/sys/src/9/ip/ipifc.c
+++ b/sys/src/9/ip/ipifc.c
@@ -174,17 +174,12 @@
 {
 	char *err;
 
+	wlock(ifc);
 	if(waserror()){
 		wunlock(ifc);
 		nexterror();
 	}
-	wlock(ifc);
 
-	/* dissociate routes */
-	if(ifc->m != nil && ifc->m->unbindonclose == 0)
-		ifc->conv->inuse--;
-	ifc->ifcid++;
-
 	/* disassociate logical interfaces (before zeroing ifc->arg) */
 	while(ifc->lifc){
 		err = ipifcremlifc(ifc, ifc->lifc);
@@ -208,7 +203,12 @@
 	qclose(ifc->conv->wq);
 	qclose(ifc->conv->sq);
 
+	/* dissociate routes */
+	ifc->ifcid++;
+	if(ifc->m != nil && ifc->m->unbindonclose == 0)
+		ifc->conv->inuse--;
 	ifc->m = nil;
+
 	wunlock(ifc);
 	poperror();
 	return nil;
@@ -299,10 +299,10 @@
 		runlock(ifc);
 		nexterror();
 	}
-	if(ifc->m == nil || ifc->m->pktin == nil)
-		freeb(bp);
-	else
+	if(ifc->m && ifc->m->pktin)
 		(*ifc->m->pktin)(c->p->f, ifc, bp);
+	else
+		freeb(bp);
 	runlock(ifc);
 	poperror();
 }
@@ -416,7 +416,12 @@
 	}
 	if(isv4(ip))
 		tentative = 0;
+
 	wlock(ifc);
+	if(waserror()){
+		wunlock(ifc);
+		nexterror();
+	}
 
 	/* ignore if this is already a local address for this ifc */
 	for(lifc = ifc->lifc; lifc; lifc = lifc->next) {
@@ -526,11 +531,13 @@
 	}
 
 	/* register the address on this network for address resolution */
-	if(isv4(ip) && ifc->m->areg != nil)
+	if(isv4(ip) && ifc->m->areg)
 		(*ifc->m->areg)(ifc, ip);
 
 out:
 	wunlock(ifc);
+	poperror();
+
 	if(tentative && sendnbrdisc)
 		icmpns(f, 0, SRC_UNSPEC, ip, TARG_MULTI, ifc->mac);
 	return nil;
@@ -603,13 +610,12 @@
 		if (parseip(rem, argv[3]) == -1)
 			return Ebadip;
 
-	wlock(ifc);
-
 	/*
 	 *  find address on this interface and remove from chain.
 	 *  for pt to pt we actually specify the remote address as the
 	 *  addresss to remove.
 	 */
+	wlock(ifc);
 	for(lifc = ifc->lifc; lifc != nil; lifc = lifc->next) {
 		if (memcmp(ip, lifc->local, IPaddrlen) == 0
 		&& memcmp(mask, lifc->mask, IPaddrlen) == 0
@@ -616,7 +622,6 @@
 		&& memcmp(rem, lifc->remote, IPaddrlen) == 0)
 			break;
 	}
-
 	rv = ipifcremlifc(ifc, lifc);
 	wunlock(ifc);
 	return rv;
@@ -639,7 +644,7 @@
 			ifc = (Ipifc*)(*cp)->ptcl;
 			m = ifc->m;
 			if(m && m->addroute)
-				m->addroute(ifc, vers, addr, mask, gate, type);
+				(*m->addroute)(ifc, vers, addr, mask, gate, type);
 		}
 	}
 }
@@ -656,8 +661,12 @@
 		if(*cp != nil) {
 			ifc = (Ipifc*)(*cp)->ptcl;
 			m = ifc->m;
-			if(m && m->remroute)
-				m->remroute(ifc, vers, addr, mask);
+			if(m && m->remroute){
+				if(!waserror()){
+					(*m->remroute)(ifc, vers, addr, mask);
+					poperror();
+				}
+			}
 		}
 	}
 }
@@ -678,18 +687,15 @@
 	if(ifc->m == nil)
 		 return "ipifc not yet bound to device";
 
-	if(waserror()){
-		wunlock(ifc);
-		nexterror();
-	}
 	wlock(ifc);
 	while(ifc->lifc){
 		err = ipifcremlifc(ifc, ifc->lifc);
-		if(err)
-			error(err);
+		if(err){
+			wunlock(ifc);
+			return err;
+		}
 	}
 	wunlock(ifc);
-	poperror();
 
 	err = ipifcadd(ifc, argv, argc, 0, nil);
 	if(err)
@@ -839,6 +845,10 @@
 	int h;
 
 	qlock(f->self);
+	if(waserror()){
+		qunlock(f->self);
+		nexterror();
+	}
 
 	/* see if the address already exists */
 	h = hashipa(a);
@@ -882,12 +892,13 @@
 		else
 			v6addroute(f, tifc, a, IPallbits, a, type);
 
-		if((type & Rmulti) && ifc->m->addmulti != nil)
+		if((type & Rmulti) && ifc->m->addmulti)
 			(*ifc->m->addmulti)(ifc, a, lifc->local);
 	} else
 		lp->ref++;
 
 	qunlock(f->self);
+	poperror();
 }
 
 /*
@@ -994,8 +1005,12 @@
 	if(--(link->ref) != 0)
 		goto out;
 
-	if((p->type & Rmulti) && ifc->m->remmulti != nil)
-		(*ifc->m->remmulti)(ifc, a, lifc->local);
+	if((p->type & Rmulti) && ifc->m->remmulti){
+		if(!waserror()){
+			(*ifc->m->remmulti)(ifc, a, lifc->local);
+			poperror();
+		}
+	}
 
 	/* ref == 0, remove from both chains and free the link */
 	*l_lifc = link->lifclink;
@@ -1448,11 +1463,11 @@
 		if((*p)->inuse == 0)
 			continue;
 		ifc = (Ipifc*)(*p)->ptcl;
+		wlock(ifc);
 		if(waserror()){
 			wunlock(ifc);
 			nexterror();
 		}
-		wlock(ifc);
 		for(lifc = ifc->lifc; lifc; lifc = lifc->next)
 			if(ipcmp(ia, lifc->local) == 0)
 				addselfcache(f, ifc, lifc, ma, Rmulti);
@@ -1491,16 +1506,11 @@
 			continue;
 
 		ifc = (Ipifc*)(*p)->ptcl;
-		if(waserror()){
-			wunlock(ifc);
-			nexterror();
-		}
 		wlock(ifc);
 		for(lifc = ifc->lifc; lifc; lifc = lifc->next)
 			if(ipcmp(ia, lifc->local) == 0)
 				remselfcache(f, ifc, lifc, ma);
 		wunlock(ifc);
-		poperror();
 	}
 
 	free(multi);
@@ -1545,6 +1555,10 @@
 				runlock(nifc);
 				continue;
 			}
+			if(waserror()){
+				runlock(nifc);
+				nexterror();
+			}
 			for(lifc = nifc->lifc; lifc; lifc = lifc->next){
 				maskip(ip, lifc->mask, net);
 				if(ipcmp(net, lifc->remote) == 0) {
@@ -1557,6 +1571,7 @@
 				}
 			}
 			runlock(nifc);
+			poperror();
 		}
 	}
 	else {					/* V4 */
@@ -1569,6 +1584,10 @@
 				runlock(nifc);
 				continue;
 			}
+			if(waserror()){
+				runlock(nifc);
+				nexterror();
+			}
 			for(lifc = nifc->lifc; lifc; lifc = lifc->next){
 				maskip(ip, lifc->mask, net);
 				if(ipcmp(net, lifc->remote) == 0){
@@ -1577,6 +1596,7 @@
 				}
 			}
 			runlock(nifc);
+			poperror();
 		}
 	}
 }
@@ -1649,9 +1669,9 @@
 	lifc->origint = origint;
 
 	/* issue "add" ctl msg for v6 link-local addr and prefix len */
-	if(!ifc->m->pref2addr)
+	if(ifc->m->pref2addr == nil)
 		return Ebadarg;
-	ifc->m->pref2addr(prefix, ifc->mac);	/* mac → v6 link-local addr */
+	(*ifc->m->pref2addr)(prefix, ifc->mac);	/* mac → v6 link-local addr */
 	sprint(addr, "%I", prefix);
 	sprint(preflen, "/%d", plen);
 	params[0] = "add";
-- 
⑨