shithub: riscv

Download patch

ref: f12744b5db76e862de58aaa54cbd5ddfc63905b0
parent: 645b5f8724c0116d974680bdb591320256cc36e0
author: cinap_lenrek <[email protected]>
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