changelog shortlog tags branches files raw gz bz2 help

Mercurial > hg > plan9front / changeset: devip: remove unused c->car qlock, avoid potential deadlock in ipifcregisterproxy()

changeset 7229: a0387db0dcba
parent 7228: 74d3b4699b3a
child 7230: 3a5ccd543798
author: cinap_lenrek@felloff.net
date: Sat, 11 May 2019 14:01:26 +0200
files: sys/src/9/ip/devip.c sys/src/9/ip/ip.h sys/src/9/ip/ipifc.c
description: devip: remove unused c->car qlock, avoid potential deadlock in ipifcregisterproxy()

remove references to the unused Conv.car qlock.

ipifcregisterproxy() is called with the proxy
ifc wlock'd, which means we cannot acquire the
rwlock of the interfaces that will proxy for us
because it is allowed to rlock() multiple ifc's
in any order. to get arround this, we use canrlock()
and skip the interface when we cannot acquire the
lock.

the ifc should get wlock'd only when we are about
to modify the ifc or its lifc chain. that is when
adding or removing addresses. wlock is not required
when we addresses to the selfcache, which has its
own qlock.
     1.1--- a/sys/src/9/ip/devip.c
     1.2+++ b/sys/src/9/ip/devip.c
     1.3@@ -1037,7 +1037,7 @@ bindctlmsg(Proto *x, Conv *c, Cmdbuf *cb
     1.4 	if(x->bind == nil)
     1.5 		p = Fsstdbind(c, cb->f, cb->nf);
     1.6 	else
     1.7-		p = x->bind(c, cb->f, cb->nf);
     1.8+		p = (*x->bind)(c, cb->f, cb->nf);
     1.9 	if(p != nil)
    1.10 		error(p);
    1.11 }
    1.12@@ -1148,7 +1148,7 @@ ipwrite(Chan* ch, void *v, long n, vlong
    1.13 				error(Ebadip);
    1.14 			ipifcremmulti(c, c->raddr, ia);
    1.15 		} else if(x->ctl != nil) {
    1.16-			p = x->ctl(c, cb->f, cb->nf);
    1.17+			p = (*x->ctl)(c, cb->f, cb->nf);
    1.18 			if(p != nil)
    1.19 				error(p);
    1.20 		} else
     2.1--- a/sys/src/9/ip/ip.h
     2.2+++ b/sys/src/9/ip/ip.h
     2.3@@ -210,7 +210,6 @@ struct Conv
     2.4 	Queue*	sq;			/* snooping queue */
     2.5 	Ref	snoopers;		/* number of processes with snoop open */
     2.6 
     2.7-	QLock	car;
     2.8 	Rendez	cr;
     2.9 	char	cerr[ERRMAX];
    2.10 
     3.1--- a/sys/src/9/ip/ipifc.c
     3.2+++ b/sys/src/9/ip/ipifc.c
     3.3@@ -61,7 +61,7 @@ static char tifc[] = "ifc ";
     3.4 
     3.5 static void	addselfcache(Fs *f, Ipifc *ifc, Iplifc *lifc, uchar *a, int type);
     3.6 static void	remselfcache(Fs *f, Ipifc *ifc, Iplifc *lifc, uchar *a);
     3.7-static void	ipifcregisteraddr(Fs*, Ipifc*, uchar *, uchar *);
     3.8+static void	ipifcregisteraddr(Fs*, Ipifc*, Iplifc*, uchar*);
     3.9 static void	ipifcregisterproxy(Fs*, Ipifc*, uchar*, int);
    3.10 static char*	ipifcremlifc(Ipifc*, Iplifc**);
    3.11 
    3.12@@ -194,29 +194,28 @@ ipifcbind(Conv *c, char **argv, int argc
    3.13 
    3.14 /*
    3.15  *  detach a device from an interface, close the interface
    3.16- *  called with ifc->conv closed
    3.17  */
    3.18 static char*
    3.19 ipifcunbind(Ipifc *ifc)
    3.20 {
    3.21-	char *err;
    3.22-
    3.23 	wlock(ifc);
    3.24-	if(waserror()){
    3.25+	if(ifc->m == nil){
    3.26 		wunlock(ifc);
    3.27-		nexterror();
    3.28+		return "interface not bound";
    3.29 	}
    3.30 
    3.31 	/* disassociate logical interfaces (before zeroing ifc->arg) */
    3.32-	while(ifc->lifc != nil){
    3.33-		err = ipifcremlifc(ifc, &ifc->lifc);
    3.34-		if(err != nil)
    3.35-			error(err);
    3.36+	while(ifc->lifc != nil)
    3.37+		ipifcremlifc(ifc, &ifc->lifc);
    3.38+
    3.39+	/* disassociate device */
    3.40+	if(ifc->m->unbind != nil){
    3.41+		if(!waserror()){
    3.42+			(*ifc->m->unbind)(ifc);
    3.43+			poperror();
    3.44+		}
    3.45 	}
    3.46 
    3.47-	/* disassociate device */
    3.48-	if(ifc->m != nil && ifc->m->unbind != nil)
    3.49-		(*ifc->m->unbind)(ifc);
    3.50 	memset(ifc->dev, 0, sizeof(ifc->dev));
    3.51 	ifc->arg = nil;
    3.52 
    3.53@@ -230,12 +229,11 @@ ipifcunbind(Ipifc *ifc)
    3.54 
    3.55 	/* dissociate routes */
    3.56 	ifc->ifcid++;
    3.57-	if(ifc->m != nil && ifc->m->unbindonclose == 0)
    3.58+	if(ifc->m->unbindonclose == 0)
    3.59 		ifc->conv->inuse--;
    3.60 	ifc->m = nil;
    3.61+	wunlock(ifc);
    3.62 
    3.63-	wunlock(ifc);
    3.64-	poperror();
    3.65 	return nil;
    3.66 }
    3.67 
    3.68@@ -400,15 +398,16 @@ ipifccreate(Conv *c)
    3.69 
    3.70 /*
    3.71  *  called after last close of ipifc data or ctl
    3.72- *  called with c locked, we must unlock
    3.73  */
    3.74 static void
    3.75 ipifcclose(Conv *c)
    3.76 {
    3.77 	Ipifc *ifc;
    3.78+	Medium *m;
    3.79 
    3.80 	ifc = (Ipifc*)c->ptcl;
    3.81-	if(ifc->m != nil && ifc->m->unbindonclose)
    3.82+	m = ifc->m;
    3.83+	if(m != nil && m->unbindonclose)
    3.84 		ipifcunbind(ifc);
    3.85 }
    3.86 
    3.87@@ -489,7 +488,7 @@ ipifcadd(Ipifc *ifc, char **argv, int ar
    3.88 	wlock(ifc);
    3.89 	if(ifc->m == nil){
    3.90 		wunlock(ifc);
    3.91-		return "ipifc not yet bound to device";
    3.92+		return "interface not yet bound to device";
    3.93 	}
    3.94 	f = ifc->conv->p->f;
    3.95 	if(waserror()){
    3.96@@ -615,17 +614,16 @@ ipifcadd(Ipifc *ifc, char **argv, int ar
    3.97 	}
    3.98 
    3.99 done:
   3.100+	ipifcregisteraddr(f, ifc, lifc, ip);
   3.101 	wunlock(ifc);
   3.102 	poperror();
   3.103 
   3.104-	ipifcregisteraddr(f, ifc, ip, ip);
   3.105-
   3.106 	return nil;
   3.107 }
   3.108 
   3.109 /*
   3.110  *  remove a logical interface from an ifc
   3.111- *  always called with ifc wlock'd
   3.112+ *	called with ifc wlock'd
   3.113  */
   3.114 static char*
   3.115 ipifcremlifc(Ipifc *ifc, Iplifc **l)
   3.116@@ -678,7 +676,6 @@ done:
   3.117 
   3.118 /*
   3.119  *  remove an address from an interface.
   3.120- *  called with c->car locked
   3.121  */
   3.122 char*
   3.123 ipifcrem(Ipifc *ifc, char **argv, int argc)
   3.124@@ -727,18 +724,9 @@ ipifcconnect(Conv* c, char **argv, int a
   3.125 	Ipifc *ifc;
   3.126 
   3.127 	ifc = (Ipifc*)c->ptcl;
   3.128-
   3.129-	if(ifc->m == nil)
   3.130-		 return "ipifc not yet bound to device";
   3.131-
   3.132 	wlock(ifc);
   3.133-	while(ifc->lifc != nil){
   3.134-		err = ipifcremlifc(ifc, &ifc->lifc);
   3.135-		if(err != nil){
   3.136-			wunlock(ifc);
   3.137-			return err;
   3.138-		}
   3.139-	}
   3.140+	while(ifc->lifc != nil)
   3.141+		ipifcremlifc(ifc, &ifc->lifc);
   3.142 	wunlock(ifc);
   3.143 
   3.144 	err = ipifcadd(ifc, argv, argc, 0, nil);
   3.145@@ -808,7 +796,6 @@ ipifcra6(Ipifc *ifc, char **argv, int ar
   3.146 
   3.147 /*
   3.148  *  non-standard control messages.
   3.149- *  called with c->car locked.
   3.150  */
   3.151 static char*
   3.152 ipifcctl(Conv* c, char **argv, int argc)
   3.153@@ -892,7 +879,6 @@ ipifcinit(Fs *f)
   3.154 
   3.155 /*
   3.156  *  add to self routing cache
   3.157- *	called with c->car locked
   3.158  */
   3.159 static void
   3.160 addselfcache(Fs *f, Ipifc *ifc, Iplifc *lifc, uchar *a, int type)
   3.161@@ -1011,7 +997,6 @@ ipselffree(Ipself *p)
   3.162 /*
   3.163  *  Decrement reference for this address on this link.
   3.164  *  Unlink from selftab if this is the last ref.
   3.165- *	called with c->car locked
   3.166  */
   3.167 static void
   3.168 remselfcache(Fs *f, Ipifc *ifc, Iplifc *lifc, uchar *a)
   3.169@@ -1468,7 +1453,7 @@ ipismulticast(uchar *ip)
   3.170 }
   3.171 
   3.172 /*
   3.173- *  add a multicast address to an interface, called with c->car locked
   3.174+ *  add a multicast address to an interface.
   3.175  */
   3.176 void
   3.177 ipifcaddmulti(Conv *c, uchar *ma, uchar *ia)
   3.178@@ -1487,14 +1472,14 @@ ipifcaddmulti(Conv *c, uchar *ma, uchar 
   3.179 
   3.180 	f = c->p->f;
   3.181 	if((ifc = findipifc(f, ia, ma, Rmulti)) != nil){
   3.182-		wlock(ifc);
   3.183+		rlock(ifc);
   3.184 		if(waserror()){
   3.185-			wunlock(ifc);
   3.186+			runlock(ifc);
   3.187 			nexterror();
   3.188 		}
   3.189 		if((lifc = iplocalonifc(ifc, ia)) != nil)
   3.190 			addselfcache(f, ifc, lifc, ma, Rmulti);
   3.191-		wunlock(ifc);
   3.192+		runlock(ifc);
   3.193 		poperror();
   3.194 	}
   3.195 
   3.196@@ -1507,7 +1492,7 @@ ipifcaddmulti(Conv *c, uchar *ma, uchar 
   3.197 
   3.198 
   3.199 /*
   3.200- *  remove a multicast address from an interface, called with c->car locked
   3.201+ *  remove a multicast address from an interface.
   3.202  */
   3.203 void
   3.204 ipifcremmulti(Conv *c, uchar *ma, uchar *ia)
   3.205@@ -1530,33 +1515,27 @@ ipifcremmulti(Conv *c, uchar *ma, uchar 
   3.206 
   3.207 	f = c->p->f;
   3.208 	if((ifc = findipifc(f, ia, ma, Rmulti)) != nil){
   3.209-		wlock(ifc);
   3.210+		rlock(ifc);
   3.211 		if(!waserror()){
   3.212 			if((lifc = iplocalonifc(ifc, ia)) != nil)
   3.213 				remselfcache(f, ifc, lifc, ma);
   3.214 			poperror();
   3.215 		}
   3.216-		wunlock(ifc);
   3.217+		runlock(ifc);
   3.218 	}
   3.219 	free(multi);
   3.220 }
   3.221 
   3.222 /* register the address on this network for address resolution */
   3.223 static void
   3.224-ipifcregisteraddr(Fs *f, Ipifc *ifc, uchar *ia, uchar *ip)
   3.225+ipifcregisteraddr(Fs *f, Ipifc *ifc, Iplifc *lifc, uchar *ip)
   3.226 {
   3.227-	Iplifc *lifc;
   3.228-
   3.229-	rlock(ifc);
   3.230 	if(waserror()){
   3.231-		runlock(ifc);
   3.232-		print("ipifcregisteraddr %s %I %I: %s\n", ifc->dev, ia, ip, up->errstr);
   3.233+		print("ipifcregisteraddr %s %I %I: %s\n", ifc->dev, lifc->local, ip, up->errstr);
   3.234 		return;
   3.235 	}
   3.236-	lifc = iplocalonifc(ifc, ia);
   3.237-	if(lifc != nil && ifc->m != nil && ifc->m->areg != nil)
   3.238+	if(ifc->m != nil && ifc->m->areg != nil)
   3.239 		(*ifc->m->areg)(f, ifc, lifc, ip);
   3.240-	runlock(ifc);
   3.241 	poperror();
   3.242 }
   3.243 
   3.244@@ -1571,15 +1550,14 @@ ipifcregisterproxy(Fs *f, Ipifc *ifc, uc
   3.245 	/* register the address on any interface that will proxy for the ip */
   3.246 	for(cp = f->ipifc->conv; *cp != nil; cp++){
   3.247 		nifc = (Ipifc*)(*cp)->ptcl;
   3.248-		if(nifc == ifc)
   3.249+		if(nifc == ifc || !canrlock(nifc))
   3.250 			continue;
   3.251 
   3.252-		wlock(nifc);
   3.253 		if(nifc->m == nil
   3.254 		|| (lifc = ipremoteonifc(nifc, ip)) == nil
   3.255 		|| (lifc->type & Rptpt) != 0
   3.256 		|| waserror()){
   3.257-			wunlock(nifc);
   3.258+			runlock(nifc);
   3.259 			continue;
   3.260 		}
   3.261 		if((lifc->type & Rv4) == 0){
   3.262@@ -1590,12 +1568,10 @@ ipifcregisterproxy(Fs *f, Ipifc *ifc, uc
   3.263 			else
   3.264 				remselfcache(f, nifc, lifc, a);
   3.265 		}
   3.266-		ipmove(a, lifc->local);
   3.267-		wunlock(nifc);
   3.268+		if(add)
   3.269+			ipifcregisteraddr(f, nifc, lifc, ip);
   3.270+		runlock(nifc);
   3.271 		poperror();
   3.272-
   3.273-		if(add)
   3.274-			ipifcregisteraddr(f, nifc, a, ip);
   3.275 	}
   3.276 }
   3.277