git: 9front

Download patch

ref: 0cc561973d6197822713364e073dfa07fa423789
parent: cf9a3c2ad5a1e1a371cad0b6d5fc4164a68caf76
author: cinap_lenrek <cinap_lenrek@felloff.net>
date: Sun Jan 5 13:20:47 EST 2020 

devip: fix packet loss when interface is wlocked

to prevent deadlock on media unbind (which is called with
the interface wlock()'ed), the medias reader processes
that unbind was waiting for used to discard packets when
the interface could not be rlocked.

this has the unfortunate side effect that when we change
addresses on a interface that packets are getting lost.
this is problematic for the processing of ipv6 router
advertisements when multiple RA's are getting received
in quick succession.

this change removes that packet dropping behaviour and
instead changes the unbind process to avoid the deadlock
by wunlock()ing the interface temporarily while waiting
for the reader processes to finish. the interface media
is also changed to the mullmedium before unlocking (see
the comment).


--- a/sys/src/9/ip/ethermedium.c
+++ b/sys/src/9/ip/ethermedium.c
@@ -218,6 +218,9 @@
 {
 	Etherrock *er = ifc->arg;
 
+	while(waserror())
+		;
+
 	/* wait for readers to start */
 	while(er->arpp == (void*)-1 || er->read4p == (void*)-1 || er->read6p == (void*)-1)
 		tsleep(&up->sleep, return0, 0, 300);
@@ -229,10 +232,19 @@
 	if(er->arpp != nil)
 		postnote(er->arpp, 1, "unbind", 0);
 
+	poperror();
+
+	wunlock(ifc);
+	while(waserror())
+		;
+
 	/* wait for readers to die */
 	while(er->arpp != nil || er->read4p != nil || er->read6p != nil)
 		tsleep(&up->sleep, return0, 0, 300);
 
+	poperror();
+	wlock(ifc);
+
 	if(er->mchan4 != nil)
 		cclose(er->mchan4);
 	if(er->cchan4 != nil)
@@ -319,16 +331,12 @@
 	ifc = a;
 	er = ifc->arg;
 	er->read4p = up;	/* hide identity under a rock for unbind */
-	if(waserror()){
-		er->read4p = nil;
-		pexit("hangup", 1);
-	}
+	if(!waserror())
 	for(;;){
 		bp = devtab[er->mchan4->type]->bread(er->mchan4, ifc->maxtu, 0);
-		if(!canrlock(ifc)){
-			freeb(bp);
-			continue;
-		}
+		if(bp == nil)
+			break;
+		rlock(ifc);
 		if(waserror()){
 			runlock(ifc);
 			nexterror();
@@ -343,6 +351,8 @@
 		runlock(ifc);
 		poperror();
 	}
+	er->read4p = nil;
+	pexit("hangup", 1);
 }
 
 
@@ -359,16 +369,12 @@
 	ifc = a;
 	er = ifc->arg;
 	er->read6p = up;	/* hide identity under a rock for unbind */
-	if(waserror()){
-		er->read6p = nil;
-		pexit("hangup", 1);
-	}
+	if(!waserror())
 	for(;;){
 		bp = devtab[er->mchan6->type]->bread(er->mchan6, ifc->maxtu, 0);
-		if(!canrlock(ifc)){
-			freeb(bp);
-			continue;
-		}
+		if(bp == nil)
+			break;
+		rlock(ifc);
 		if(waserror()){
 			runlock(ifc);
 			nexterror();
@@ -383,6 +389,8 @@
 		runlock(ifc);
 		poperror();
 	}
+	er->read6p = nil;
+	pexit("hangup", 1);
 }
 
 static void
@@ -559,10 +567,7 @@
 	if(ebp == nil)
 		return;
 
-	if(!canrlock(ifc)){
-		freeb(ebp);
-		return;
-	}
+	rlock(ifc);
 
 	e = (Etherarp*)ebp->rp;
 	switch(nhgets(e->op)) {
--- a/sys/src/9/ip/ip.c
+++ b/sys/src/9/ip/ip.c
@@ -123,8 +123,7 @@
 	else
 		gate = r->v4.gate;
 
-	if(!canrlock(ifc))
-		goto free;
+	rlock(ifc);
 	if(waserror()){
 		runlock(ifc);
 		nexterror();
--- a/sys/src/9/ip/ipifc.c
+++ b/sys/src/9/ip/ipifc.c
@@ -201,8 +201,11 @@
 static char*
 ipifcunbind(Ipifc *ifc)
 {
+	Medium *m;
+
 	wlock(ifc);
-	if(ifc->m == nil){
+	m = ifc->m;
+	if(m == nil){
 		wunlock(ifc);
 		return Eunbound;
 	}
@@ -212,9 +215,18 @@
 		ipifcremlifc(ifc, &ifc->lifc);
 
 	/* disassociate device */
-	if(ifc->m->unbind != nil){
+	if(m->unbind != nil){
+		extern Medium nullmedium;
+
+		/*
+		 * unbind() might unlock the ifc, so change the medium
+		 * to the nullmedium to prevent packets from getting
+		 * sent while the medium is shutting down.
+		 */
+		ifc->m = &nullmedium;
+
 		if(!waserror()){
-			(*ifc->m->unbind)(ifc);
+			(*m->unbind)(ifc);
 			poperror();
 		}
 	}
@@ -232,7 +244,7 @@
 
 	/* dissociate routes */
 	ifc->ifcid++;
-	if(ifc->m->unbindonclose == 0)
+	if(m->unbindonclose == 0)
 		ifc->conv->inuse--;
 	ifc->m = nil;
 	wunlock(ifc);
@@ -370,10 +382,7 @@
 		return;
 
 	ifc = (Ipifc*)c->ptcl;
-	if(!canrlock(ifc)){
-		freeb(bp);
-		return;
-	}
+	rlock(ifc);
 	if(waserror()){
 		runlock(ifc);
 		nexterror();
@@ -622,9 +631,12 @@
 	}
 
 done:
-	ipifcregisteraddr(f, ifc, lifc, ip);
 	wunlock(ifc);
 	poperror();
+
+	rlock(ifc);
+	ipifcregisteraddr(f, ifc, lifc, ip);
+	runlock(ifc);
 
 	return nil;
 }
--- a/sys/src/9/ip/ipv6.c
+++ b/sys/src/9/ip/ipv6.c
@@ -76,9 +76,7 @@
 	else
 		gate = r->v6.gate;
 
-	if(!canrlock(ifc))
-		goto free;
-
+	rlock(ifc);
 	if(waserror()){
 		runlock(ifc);
 		nexterror();
--- a/sys/src/9/ip/loopbackmedium.c
+++ b/sys/src/9/ip/loopbackmedium.c
@@ -42,6 +42,9 @@
 {
 	LB *lb = ifc->arg;
 
+	while(waserror())
+		;
+
 	/* wat for reader to start */
 	while(lb->readp == (void*)-1)
 		tsleep(&up->sleep, return0, 0, 300);
@@ -49,10 +52,19 @@
 	if(lb->readp != nil)
 		postnote(lb->readp, 1, "unbind", 0);
 
+	poperror();
+
+	wunlock(ifc);
+	while(waserror())
+		;
+
 	/* wait for reader to die */
 	while(lb->readp != nil)
 		tsleep(&up->sleep, return0, 0, 300);
 
+	poperror();
+	wlock(ifc);
+
 	/* clean up */
 	qfree(lb->q);
 	free(lb);
@@ -79,18 +91,9 @@
 	ifc = a;
 	lb = ifc->arg;
 	lb->readp = up;	/* hide identity under a rock for unbind */
-	if(waserror()){
-		lb->readp = nil;
-		pexit("hangup", 1);
-	}
-	for(;;){
-		bp = qbread(lb->q, Maxtu);
-		if(bp == nil)
-			continue;
-		if(!canrlock(ifc)){
-			freeb(bp);
-			continue;
-		}
+	if(!waserror())
+	while((bp = qbread(lb->q, Maxtu)) != nil){
+		rlock(ifc);
 		if(waserror()){
 			runlock(ifc);
 			nexterror();
@@ -103,6 +106,8 @@
 		runlock(ifc);
 		poperror();
 	}
+	lb->readp = nil;
+	pexit("hangup", 1);
 }
 
 Medium loopbackmedium =
--- a/sys/src/9/ip/netdevmedium.c
+++ b/sys/src/9/ip/netdevmedium.c
@@ -66,6 +66,9 @@
 {
 	Netdevrock *er = ifc->arg;
 
+	while(waserror())
+		;
+
 	/* wait for reader to start */
 	while(er->readp == (void*)-1)
 		tsleep(&up->sleep, return0, 0, 300);
@@ -73,10 +76,19 @@
 	if(er->readp != nil)
 		postnote(er->readp, 1, "unbind", 0);
 
+	poperror();
+
+	wunlock(ifc);
+	while(waserror())
+		;
+
 	/* wait for reader to die */
 	while(er->readp != nil)
 		tsleep(&up->sleep, return0, 0, 300);
 
+	poperror();
+	wlock(ifc);
+
 	if(er->mchan != nil)
 		cclose(er->mchan);
 
@@ -107,33 +119,22 @@
 	Ipifc *ifc;
 	Block *bp;
 	Netdevrock *er;
-	char *argv[1];
 
 	ifc = a;
 	er = ifc->arg;
 	er->readp = up;	/* hide identity under a rock for unbind */
-	if(waserror()){
-		er->readp = nil;
-		pexit("hangup", 1);
-	}
+	if(!waserror())
 	for(;;){
 		bp = devtab[er->mchan->type]->bread(er->mchan, ifc->maxtu, 0);
 		if(bp == nil){
-			/*
-			 * get here if mchan is a pipe and other side hangs up
-			 * clean up this interface & get out
-			 */
 			poperror();
-			er->readp = nil;
-			argv[0] = "unbind";
-			if(!waserror())
+			if(!waserror()){
+				static char *argv[]  = { "unbind" };
 				ifc->conv->p->ctl(ifc->conv, argv, 1);
-			pexit("hangup", 1);
+			}
+			break;
 		}
-		if(!canrlock(ifc)){
-			freeb(bp);
-			continue;
-		}
+		rlock(ifc);
 		if(waserror()){
 			runlock(ifc);
 			nexterror();
@@ -146,6 +147,8 @@
 		runlock(ifc);
 		poperror();
 	}
+	er->readp = nil;
+	pexit("hangup", 1);
 }
 
 void
-- 
⑨