Extend the scope of the socket lock to protect `so_state' in connect(2).

As a side effect, soconnect() and soconnect2() now expect a locked socket,
so update all the callers.

ok bluhm@

4 files changed, 50 insertions, 32 deletions

diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
index 572f704aec3..3bb22231867 100644
--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uipc_socket.c,v 1.196 2017/07/20 09:49:45 bluhm Exp $	*/
+/*	$OpenBSD: uipc_socket.c,v 1.197 2017/07/24 15:07:39 mpi Exp $	*/
 /*	$NetBSD: uipc_socket.c,v 1.21 1996/02/04 02:17:52 christos Exp $	*/
 
 /*
@@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf *nam)
 int
 soconnect(struct socket *so, struct mbuf *nam)
 {
-	int s, error;
+	int error;
+
+	soassertlocked(so);
 
 	if (so->so_options & SO_ACCEPTCONN)
 		return (EOPNOTSUPP);
-	s = solock(so);
 	/*
 	 * If protocol is connection-based, can only connect once.
 	 * Otherwise, if connected, try to disconnect first.
@@ -329,19 +330,17 @@ soconnect(struct socket *so, struct mbuf *nam)
 	else
 		error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
 		    NULL, nam, NULL, curproc);
-	sounlock(s);
 	return (error);
 }
 
 int
 soconnect2(struct socket *so1, struct socket *so2)
 {
-	int s, error;
+	int error;
 
-	s = solock(so1);
+	soassertlocked(so1);
 	error = (*so1->so_proto->pr_usrreq)(so1, PRU_CONNECT2, NULL,
 	    (struct mbuf *)so2, NULL, curproc);
-	sounlock(s);
 	return (error);
 }
 
diff --git a/sys/kern/uipc_syscalls.c b/sys/kern/uipc_syscalls.c
index 2b9676e32cf..ac47bef508d 100644
--- a/sys/kern/uipc_syscalls.c
+++ b/sys/kern/uipc_syscalls.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uipc_syscalls.c,v 1.155 2017/07/20 18:40:16 bluhm Exp $	*/
+/*	$OpenBSD: uipc_syscalls.c,v 1.156 2017/07/24 15:07:39 mpi Exp $	*/
 /*	$NetBSD: uipc_syscalls.c,v 1.19 1996/02/09 19:00:48 christos Exp $	*/
 
 /*
@@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, register_t *retval)
 	if ((error = getsock(p, SCARG(uap, s), &fp)) != 0)
 		return (error);
 	so = fp->f_data;
+	s = solock(so);
 	if (so->so_state & SS_ISCONNECTING) {
-		FRELE(fp, p);
-		return (EALREADY);
+		error = EALREADY;
+		goto out;
 	}
 	error = sockargs(&nam, SCARG(uap, name), SCARG(uap, namelen),
 	    MT_SONAME);
 	if (error)
-		goto bad;
+		goto out;
 	error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
 	    so->so_state);
 	if (error)
-		goto bad;
+		goto out;
 #ifdef KTRACE
 	if (KTRPOINT(p, KTR_STRUCT))
 		ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
@@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, register_t *retval)
 	if (isdnssocket(so)) {
 		u_int namelen = nam->m_len;
 		error = dns_portcheck(p, so, mtod(nam, void *), &namelen);
-		if (error) {
-			FRELE(fp, p);
-			m_freem(nam);
-			return (error);
-		}
+		if (error)
+			goto out;
 		nam->m_len = namelen;
 	}
 
@@ -405,11 +403,9 @@ sys_connect(struct proc *p, void *v, register_t *retval)
 	if (error)
 		goto bad;
 	if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) {
-		FRELE(fp, p);
-		m_freem(nam);
-		return (EINPROGRESS);
+		error = EINPROGRESS;
+		goto out;
 	}
-	s = solock(so);
 	while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
 		error = sosleep(so, &so->so_timeo, PSOCK | PCATCH,
 		    "netcon2", 0);
@@ -423,10 +419,11 @@ sys_connect(struct proc *p, void *v, register_t *retval)
 		error = so->so_error;
 		so->so_error = 0;
 	}
-	sounlock(s);
 bad:
 	if (!interrupted)
 		so->so_state &= ~SS_ISCONNECTING;
+out:
+	sounlock(s);
 	FRELE(fp, p);
 	m_freem(nam);
 	if (error == ERESTART)
@@ -446,7 +443,7 @@ sys_socketpair(struct proc *p, void *v, register_t *retval)
 	struct filedesc *fdp = p->p_fd;
 	struct file *fp1, *fp2;
 	struct socket *so1, *so2;
-	int type, cloexec, nonblock, fflag, error, sv[2];
+	int s, type, cloexec, nonblock, fflag, error, sv[2];
 
 	type  = SCARG(uap, type) & ~(SOCK_CLOEXEC | SOCK_NONBLOCK);
 	cloexec = (SCARG(uap, type) & SOCK_CLOEXEC) ? UF_EXCLOSE : 0;
@@ -460,14 +457,20 @@ sys_socketpair(struct proc *p, void *v, register_t *retval)
 	if (error)
 		goto free1;
 
-	if ((error = soconnect2(so1, so2)) != 0)
+	s = solock(so1);
+	error = soconnect2(so1, so2);
+	sounlock(s);
+	if (error != 0)
 		goto free2;
 
 	if ((SCARG(uap, type) & SOCK_TYPE_MASK) == SOCK_DGRAM) {
 		/*
 		 * Datagram socket connection is asymmetric.
 		 */
-		 if ((error = soconnect2(so2, so1)) != 0)
+		s = solock(so2);
+		error = soconnect2(so2, so1);
+		sounlock(s);
+		if (error != 0)
 			goto free2;
 	}
 	fdplock(fdp);
diff --git a/sys/miscfs/fifofs/fifo_vnops.c b/sys/miscfs/fifofs/fifo_vnops.c
index 62b19a40fb0..7502183f931 100644
--- a/sys/miscfs/fifofs/fifo_vnops.c
+++ b/sys/miscfs/fifofs/fifo_vnops.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: fifo_vnops.c,v 1.57 2017/07/08 09:19:02 mpi Exp $	*/
+/*	$OpenBSD: fifo_vnops.c,v 1.58 2017/07/24 15:07:39 mpi Exp $	*/
 /*	$NetBSD: fifo_vnops.c,v 1.18 1996/03/16 23:52:42 christos Exp $	*/
 
 /*
@@ -124,7 +124,7 @@ fifo_open(void *v)
 	struct fifoinfo *fip;
 	struct proc *p = ap->a_p;
 	struct socket *rso, *wso;
-	int error;
+	int s, error;
 
 	if ((fip = vp->v_fifoinfo) == NULL) {
 		fip = malloc(sizeof(*fip), M_VNODE, M_WAITOK);
@@ -142,7 +142,14 @@ fifo_open(void *v)
 			return (error);
 		}
 		fip->fi_writesock = wso;
+		/*
+		 * XXXSMP
+		 * We only lock `wso' because AF_LOCAL sockets are
+		 * still relying on the KERNEL_LOCK().
+		 */
+		s = solock(wso);
 		if ((error = soconnect2(wso, rso)) != 0) {
+			sounlock(s);
 			(void)soclose(wso);
 			(void)soclose(rso);
 			free(fip, M_VNODE, sizeof *fip);
@@ -152,11 +159,15 @@ fifo_open(void *v)
 		fip->fi_readers = fip->fi_writers = 0;
 		wso->so_state |= SS_CANTSENDMORE;
 		wso->so_snd.sb_lowat = PIPE_BUF;
+	} else {
+		rso = fip->fi_readsock;
+		wso = fip->fi_writesock;
+		s = solock(wso);
 	}
 	if (ap->a_mode & FREAD) {
 		fip->fi_readers++;
 		if (fip->fi_readers == 1) {
-			fip->fi_writesock->so_state &= ~SS_CANTSENDMORE;
+			wso->so_state &= ~SS_CANTSENDMORE;
 			if (fip->fi_writers > 0)
 				wakeup(&fip->fi_writers);
 		}
@@ -165,14 +176,16 @@ fifo_open(void *v)
 		fip->fi_writers++;
 		if ((ap->a_mode & O_NONBLOCK) && fip->fi_readers == 0) {
 			error = ENXIO;
+			sounlock(s);
 			goto bad;
 		}
 		if (fip->fi_writers == 1) {
-			fip->fi_readsock->so_state &= ~(SS_CANTRCVMORE|SS_ISDISCONNECTED);
+			rso->so_state &= ~(SS_CANTRCVMORE|SS_ISDISCONNECTED);
 			if (fip->fi_readers > 0)
 				wakeup(&fip->fi_readers);
 		}
 	}
+	sounlock(s);
 	if ((ap->a_mode & O_NONBLOCK) == 0) {
 		if ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
 			VOP_UNLOCK(vp, p);
@@ -246,6 +259,7 @@ fifo_write(void *v)
 	if (ap->a_uio->uio_rw != UIO_WRITE)
 		panic("fifo_write mode");
 #endif
+	/* XXXSMP changing state w/o lock isn't safe. */
 	if (ap->a_ioflag & IO_NDELAY)
 		wso->so_state |= SS_NBIO;
 	VOP_UNLOCK(ap->a_vp, p);
diff --git a/sys/nfs/nfs_socket.c b/sys/nfs/nfs_socket.c
index 748fd82d1fc..ae2ebb0fd35 100644
--- a/sys/nfs/nfs_socket.c
+++ b/sys/nfs/nfs_socket.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nfs_socket.c,v 1.119 2017/06/27 12:02:43 mpi Exp $	*/
+/*	$OpenBSD: nfs_socket.c,v 1.120 2017/07/24 15:07:39 mpi Exp $	*/
 /*	$NetBSD: nfs_socket.c,v 1.27 1996/04/15 20:20:00 thorpej Exp $	*/
 
 /*
@@ -297,16 +297,18 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
 			goto bad;
 		}
 	} else {
+		s = solock(so);
 		error = soconnect(so, nmp->nm_nam);
-		if (error)
+		if (error) {
+			sounlock(s);
 			goto bad;
+		}
 
 		/*
 		 * Wait for the connection to complete. Cribbed from the
 		 * connect system call but with the wait timing out so
 		 * that interruptible mounts don't hang here for a long time.
 		 */
-		s = solock(so);
 		while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
 			sosleep(so, &so->so_timeo, PSOCK, "nfscon", 2 * hz);
 			if ((so->so_state & SS_ISCONNECTING) &&