netif: simplify locking and cleanup

We do not need a QLock per Netfile, just
serializing open/close/write with Netif.QLock
should be sufficient. This avoids the
lock ordering violation in netifclose().

Netfile's are accessed without locking by
ethermux(), so ensure coherence before
setting the pointer. This also ensures:
Netif.f[x] == nil || Netif.f[x]->in != nil.

Netaddr's for multicast are accessed without
locking by activemulti(), so do coherence()
before linking them into the hashtable.

Remove "Lock netlock" for netown() permission
check. This is now all serialized thru
Netif.QLock.

Put waserror() guards for all the Netif
driver callbacks and wifi qwrite().

sys/src/9/bcm/ether4330.c

@@ -1401,13 +1401,17 @@ linkdown(Ctlr *ctl)
 		return;
 	ctl->status = Disconnected;
 	ethersetlink(edev, 0);
+
 	/* send eof to aux/wpa */
 	for(i = 0; i < edev->nfile; i++){
 		f = edev->f[i];
-		if(f == nil || f->in == nil || f->inuse == 0 || f->type != 0x888e)
+		if(f == nil || !f->inuse || f->type != 0x888e || waserror())
 			continue;
+		qflush(f->in);
 		qwrite(f->in, 0, 0);
+		poperror();
 	}
+	qflush(edev->oq);
 }
 
 /*

sys/src/9/port/netif.c

@@ -7,9 +7,9 @@
 #include	"../port/netif.h"
 
 static int netown(Netfile*, char*, int);
-static int openfile(Netif*, int);
+static int openfile(Netif*, int, int);
 static char* matchtoken(char*, char*);
-static char* netmulti(Netif*, Netfile*, uchar*, int);
+static void netmulti(Netif*, Netfile*, uchar*, int);
 static int parseaddr(uchar*, char*, int);
 
 /*
@@ -43,11 +43,9 @@ netifsetlimit(Netif *nif, int limit)
 		nif->limit = limit;
 		for(i = 0; i < nif->nfile; i++){
 			f = nif->f[i];
-			if(f == nil)
+			if(f == nil || !f->inuse)
 				continue;
-			qlock(f);
 			qsetlimit(f->in, nif->limit);
-			qunlock(f);
 		}
 	}
 	qunlock(nif);
@@ -118,7 +116,7 @@ netifgen(Chan *c, char*, Dirtab *vp, int, int i, Dir *dp)
 			i -= 4;
 			if(i >= nif->nfile)
 				return -1;
-			if(nif->f[i] == 0)
+			if(nif->f[i] == nil)
 				return 0;
 			q.type = QTDIR;
 			q.path = NETQID(i, N3rdqid);
@@ -131,7 +129,7 @@ netifgen(Chan *c, char*, Dirtab *vp, int, int i, Dir *dp)
 
 	/* third level */
 	f = nif->f[NETID(c->qid.path)];
-	if(f == 0)
+	if(f == nil)
 		return 0;
 	if(*f->owner){
 		o = f->owner;
@@ -183,9 +181,7 @@ Chan*
 netifopen(Netif *nif, Chan *c, int omode)
 {
 	int id;
-	Netfile *f;
 
-	id = 0;
 	if(c->qid.type & QTDIR){
 		if(omode != OREAD)
 			error(Eperm);
@@ -194,24 +190,16 @@ netifopen(Netif *nif, Chan *c, int omode)
 		case Ndataqid:
 		case Nctlqid:
 			id = NETID(c->qid.path);
-			openfile(nif, id);
+			openfile(nif, id, omode);
 			break;
 		case Ncloneqid:
-			id = openfile(nif, -1);
+			id = openfile(nif, -1, omode);
 			c->qid.path = NETQID(id, Nctlqid);
 			break;
 		default:
 			if(omode != OREAD)
 				error(Ebadarg);
 		}
-		switch(NETTYPE(c->qid.path)){
-		case Ndataqid:
-		case Nctlqid:
-			f = nif->f[id];
-			if(netown(f, up->user, omode&7) < 0)
-				error(Eperm);
-			break;
-		}
 	}
 	c->mode = openmode(omode);
 	c->flag |= COPEN;
@@ -317,7 +305,7 @@ typeinuse(Netif *nif, int type)
 	efp = &nif->f[nif->nfile];
 	for(fp = nif->f; fp < efp; fp++){
 		f = *fp;
-		if(f == 0)
+		if(f == nil)
 			continue;
 		if(f->type == type)
 			return 1;
@@ -348,8 +336,8 @@ netifwrite(Netif *nif, Chan *c, void *a, long n)
 		qunlock(nif);
 		nexterror();
 	}
-
 	qlock(nif);
+
 	f = nif->f[NETID(c->qid.path)];
 	if((p = matchtoken(buf, "connect")) != 0){
 		type = strtoul(p, 0, 0);
@@ -361,7 +349,7 @@ netifwrite(Netif *nif, Chan *c, void *a, long n)
 	} else if(matchtoken(buf, "promiscuous")){
 		if(f->prom == 0){
 			if(nif->prom == 0 && nif->promiscuous != nil)
-				nif->promiscuous(nif->arg, 1);
+				(*nif->promiscuous)(nif->arg, 1);
 			f->prom = 1;
 			nif->prom++;
 		}
@@ -372,7 +360,7 @@ netifwrite(Netif *nif, Chan *c, void *a, long n)
 			if(type < 5)
 				type = 5;
 			if(nif->scanbs != nil)
-				nif->scanbs(nif->arg, type);
+				(*nif->scanbs)(nif->arg, type);
 			f->scan = type;
 			nif->scan++;
 		}
@@ -388,16 +376,12 @@ netifwrite(Netif *nif, Chan *c, void *a, long n)
 	} else if((p = matchtoken(buf, "addmulti")) != nil){
 		if(parseaddr(binaddr, p, nif->alen) < 0)
 			error("bad address");
-		p = netmulti(nif, f, binaddr, 1);
+		netmulti(nif, f, binaddr, 1);
-		if(p)
-			error(p);
 	} else if((p = matchtoken(buf, "delmulti")) != nil
 		||(p = matchtoken(buf, "remmulti")) != nil){
 		if(parseaddr(binaddr, p, nif->alen) < 0)
 			error("bad address");
-		p = netmulti(nif, f, binaddr, 0);
+		netmulti(nif, f, binaddr, 0);
-		if(p)
-			error(p);
 	} else
 		n = -1;
 	qunlock(nif);
@@ -408,17 +392,10 @@ netifwrite(Netif *nif, Chan *c, void *a, long n)
 int
 netifwstat(Netif *nif, Chan *c, uchar *db, int n)
 {
-	Dir *dir;
 	Netfile *f;
+	Dir *dir;
 	int m;
 
-	f = nif->f[NETID(c->qid.path)];
-	if(f == 0)
-		error(Enonexist);
-
-	if(netown(f, up->user, OWRITE) < 0)
-		error(Eperm);
-
 	dir = smalloc(sizeof(Dir)+n);
 	if(waserror()){
 		free(dir);
@@ -427,14 +404,26 @@ netifwstat(Netif *nif, Chan *c, uchar *db, int n)
 	m = convM2D(db, n, &dir[0], (char*)&dir[1]);
 	if(m == 0)
 		error(Eshortstat);
+	if(waserror()){
+		qunlock(nif);
+		nexterror();
+	}
+	qlock(nif);
+	f = nif->f[NETID(c->qid.path)];
+	if(f == nil)
+		error(Enonexist);
+	if(netown(f, up->user, OWRITE) < 0)
+		error(Eperm);
 	if(!emptystr(dir[0].uid)){
 		strncpy(f->owner, dir[0].uid, KNAMELEN-1);
 		f->owner[KNAMELEN-1] = 0;
 	}
 	if(dir[0].mode != ~0UL)
 		f->mode = dir[0].mode;
+	qunlock(nif);
 	free(dir);
 	poperror();
+	poperror();
 	return m;
 }
 
@@ -458,56 +447,51 @@ netifclose(Netif *nif, Chan *c)
 	if(t != Ndataqid && t != Nctlqid)
 		return;
 
+	qlock(nif);
 	f = nif->f[NETID(c->qid.path)];
-	qlock(f);
+	if(f == nil || --(f->inuse) != 0){
-	if(--(f->inuse) == 0){
+		qunlock(nif);
-		if(f->bypass){
+		return;
-			qlock(nif);
-			nif->bypass = nil;
-			qunlock(nif);
-			f->bypass = 0;
-		}
-		if(f->prom){
-			qlock(nif);
-			if(--(nif->prom) == 0 && nif->promiscuous != nil)
-				nif->promiscuous(nif->arg, 0);
-			qunlock(nif);
-			f->prom = 0;
-		}
-		if(f->scan){
-			qlock(nif);
-			if(--(nif->scan) == 0 && nif->scanbs != nil)
-				nif->scanbs(nif->arg, 0);
-			qunlock(nif);
-			f->prom = 0;
-			f->scan = 0;
-		}
-		if(f->nmaddr){
-			qlock(nif);
-			t = 0;
-			for(ap = nif->maddr; ap; ap = ap->next){
-				if(f->maddr[t/8] & (1<<(t%8)))
-					netmulti(nif, f, ap->addr, 0);
-			}
-			qunlock(nif);
-			f->nmaddr = 0;
-		}
-		if(f->type < 0){
-			qlock(nif);
-			--(nif->all);
-			qunlock(nif);
-		}
-		f->owner[0] = 0;
-		f->type = 0;
-		f->bridge = 0;
-		f->headersonly = 0;
-		qclose(f->in);
 	}
-	qunlock(f);
+	if(f->bypass){
+		nif->bypass = nil;
+		f->bypass = 0;
+	}
+	if(f->prom){
+		if(--(nif->prom) == 0 && nif->promiscuous != nil && !waserror()){
+			(*nif->promiscuous)(nif->arg, 0);
+			poperror();
+		}
+		f->prom = 0;
+	}
+	if(f->scan){
+		if(--(nif->scan) == 0 && nif->scanbs != nil && !waserror()){
+			(*nif->scanbs)(nif->arg, 0);
+			poperror();
+		}
+		f->prom = 0;
+		f->scan = 0;
+	}
+	if(f->nmaddr){
+		t = 0;
+		for(ap = nif->maddr; ap != nil; ap = ap->next){
+			if(f->maddr[t/8] & (1<<(t%8)) && !waserror()){
+				netmulti(nif, f, ap->addr, 0);
+				poperror();
+			}
+		}
+		f->nmaddr = 0;
+	}
+	if(f->type < 0)
+		nif->all--;
+	f->owner[0] = 0;
+	f->type = 0;
+	f->bridge = 0;
+	f->headersonly = 0;
+	qclose(f->in);
+	qunlock(nif);
 }
 
-Lock netlock;
-
 static int
 netown(Netfile *p, char *o, int omode)
 {
@@ -515,7 +499,6 @@ netown(Netfile *p, char *o, int omode)
 	int mode;
 	int t;
 
-	lock(&netlock);
 	if(*p->owner){
 		if(strncmp(o, p->owner, KNAMELEN) == 0)	/* User */
 			mode = p->mode;
@@ -525,18 +508,14 @@ netown(Netfile *p, char *o, int omode)
 			mode = p->mode<<6;		/* Other */
 
 		t = access[omode&3];
-		if((t & mode) == t){
+		if((t & mode) == t)
-			unlock(&netlock);
 			return 0;
-		} else {
+		else
-			unlock(&netlock);
 			return -1;
-		}
 	}
 	strncpy(p->owner, o, KNAMELEN-1);
 	p->owner[KNAMELEN-1] = 0;
 	p->mode = 0660;
-	unlock(&netlock);
 	return 0;
 }
 
@@ -545,51 +524,48 @@ netown(Netfile *p, char *o, int omode)
  *  If id < 0, return an unused ether device.
  */
 static int
-openfile(Netif *nif, int id)
+openfile(Netif *nif, int id, int omode)
 {
 	Netfile *f, **fp, **efp;
 
-	if(id >= 0){
-		f = nif->f[id];
-		if(f == 0)
-			error(Enodev);
-		qlock(f);
-		qreopen(f->in);
-		f->inuse++;
-		qunlock(f);
-		return id;
-	}
-
 	qlock(nif);
 	if(waserror()){
 		qunlock(nif);
 		nexterror();
 	}
+	if(id >= 0){
+		f = nif->f[id];
+		if(f == nil)
+			error(Enodev);
+		if(netown(f, up->user, omode&7) < 0)
+			error(Eperm);
+		f->inuse++;
+		qreopen(f->in);
+		qsetlimit(f->in, nif->limit);
+		qunlock(nif);
+		poperror();
+		return id;
+	}
 	efp = &nif->f[nif->nfile];
 	for(fp = nif->f; fp < efp; fp++){
 		f = *fp;
-		if(f == 0){
+		if(f == nil){
 			f = malloc(sizeof(Netfile));
-			if(f == 0)
+			if(f == nil)
 				exhausted("memory");
 			f->in = qopen(nif->limit, Qmsg, 0, 0);
 			if(f->in == nil){
 				free(f);
 				exhausted("memory");
 			}
+			coherence();
 			*fp = f;
-			qlock(f);
+		} else if(f->inuse)
-		} else {
+			continue;
-			qlock(f);
-			if(f->inuse){
-				qunlock(f);
-				continue;
-			}
-		}
 		f->inuse = 1;
 		qreopen(f->in);
+		qsetlimit(f->in, nif->limit);
 		netown(f, up->user, 0);
-		qunlock(f);
 		qunlock(nif);
 		poperror();
 		return fp - nif->f;
@@ -693,13 +669,9 @@ activemulti(Netif *nif, uchar *addr, int alen)
 {
 	Netaddr *hp;
 
-	for(hp = nif->mhash[hash(addr, alen)]; hp; hp = hp->hnext)
+	for(hp = nif->mhash[hash(addr, alen)]; hp != nil; hp = hp->hnext)
-		if(memcmp(addr, hp->addr, alen) == 0){
+		if(memcmp(addr, hp->addr, alen) == 0)
-			if(hp->ref)
+			return hp->ref > 0;
-				return 1;
-			else
-				break;
-		}
 	return 0;
 }
 
@@ -729,19 +701,19 @@ parseaddr(uchar *to, char *from, int alen)
 /*
  *  keep track of multicast addresses
  */
-static char*
+static void
 netmulti(Netif *nif, Netfile *f, uchar *addr, int add)
 {
 	Netaddr **l, *ap;
-	int i;
 	ulong h;
+	int i;
 
 	if(nif->multicast == nil)
-		return "interface does not support multicast";
+		error("interface does not support multicast");
 
-	l = &nif->maddr;
 	i = 0;
-	for(ap = *l; ap; ap = *l){
+	l = &nif->maddr;
+	for(ap = *l; ap != nil; ap = *l){
 		if(memcmp(addr, ap->addr, nif->alen) == 0)
 			break;
 		i++;
@@ -749,20 +721,23 @@ netmulti(Netif *nif, Netfile *f, uchar *addr, int add)
 	}
 
 	if(add){
-		if(ap == 0){
+		if(ap == nil){
-			*l = ap = smalloc(sizeof(*ap));
+			ap = malloc(sizeof(*ap));
+			if(ap == nil)
+				error(Enomem);
 			memmove(ap->addr, addr, nif->alen);
-			ap->next = 0;
+			h = hash(ap->addr, nif->alen);
-			ap->ref = 1;
-			h = hash(addr, nif->alen);
 			ap->hnext = nif->mhash[h];
+			ap->ref = 1;
+			coherence();
 			nif->mhash[h] = ap;
+			*l = ap;
 		} else {
 			ap->ref++;
 		}
 		if(ap->ref == 1){
 			nif->nmaddr++;
-			nif->multicast(nif->arg, addr, 1);
+			(*nif->multicast)(nif->arg, addr, 1);
 		}
 		if(i < 8*sizeof(f->maddr)){
 			if((f->maddr[i/8] & (1<<(i%8))) == 0)
@@ -770,12 +745,12 @@ netmulti(Netif *nif, Netfile *f, uchar *addr, int add)
 			f->maddr[i/8] |= 1<<(i%8);
 		}
 	} else {
-		if(ap == 0 || ap->ref == 0)
+		if(ap == nil || ap->ref == 0)
-			return 0;
+			return;
 		ap->ref--;
 		if(ap->ref == 0){
 			nif->nmaddr--;
-			nif->multicast(nif->arg, addr, 0);
+			(*nif->multicast)(nif->arg, addr, 0);
 		}
 		if(i < 8*sizeof(f->maddr)){
 			if((f->maddr[i/8] & (1<<(i%8))) != 0)
@@ -783,5 +758,4 @@ netmulti(Netif *nif, Netfile *f, uchar *addr, int add)
 			f->maddr[i/8] &= ~(1<<(i%8));
 		}
 	}
-	return 0;
 }

sys/src/9/port/netif.h

@@ -31,8 +31,6 @@ enum
  */
 struct Netfile
 {
-	QLock;
-
 	int	inuse;
 	ulong	mode;
 	char	owner[KNAMELEN];

sys/src/9/port/wifi.c

@@ -580,21 +580,23 @@ wifideauth(Wifi *wifi, Wnode *wn)
 	freewifikeys(wifi, wn);
 	wn->aid = 0;
 
-	if(wn == wifi->bss){
+	if(wn != wifi->bss)
-		/* notify driver about node aid association */
+		return;
-		(*wifi->transmit)(wifi, wn, nil);
 
-		/* notify aux/wpa with a zero length packet that we got deassociated from the ap */
+	/* notify driver about node aid association */
-		ether = wifi->ether;
+	(*wifi->transmit)(wifi, wn, nil);
-		for(i=0; i<ether->nfile; i++){
+
-			f = ether->f[i];
+	/* notify aux/wpa with a zero length packet that we got deassociated from the ap */
-			if(f == nil || f->in == nil || f->inuse == 0 || f->type != 0x888e)
+	ether = wifi->ether;
-				continue;
+	for(i=0; i<ether->nfile; i++){
-			qflush(f->in);
+		f = ether->f[i];
-			qwrite(f->in, 0, 0);
+		if(f == nil || !f->inuse || f->type != 0x888e || waserror())
-		}
+			continue;
-		qflush(ether->oq);
+		qflush(f->in);
+		qwrite(f->in, 0, 0);
+		poperror();
 	}
+	qflush(ether->oq);
 }
 
 /* check if a node qualifies as our bss matching bssid and essid */