On Mon, Jul 24, 2017 at 11:56:20AM +0200, Martin Pieuchot wrote: > Updated diff addressing your comments.
Yes, previous issues are fixed. But static code analysis shows that you missed a lock in sys_socketpair() for soconnect2(). Analyzing locks for soconnect2 No lock: [soconnect2, sys_socketpair] I could convince Zaur Molotnikov to publish his tool: https://github.com/qutorial/lockfish bluhm > Index: kern/uipc_socket.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > retrieving revision 1.196 > diff -u -p -r1.196 uipc_socket.c > --- kern/uipc_socket.c 20 Jul 2017 09:49:45 -0000 1.196 > +++ kern/uipc_socket.c 24 Jul 2017 09:52:01 -0000 > @@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf > 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 > 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); > } > > Index: kern/uipc_syscalls.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v > retrieving revision 1.155 > diff -u -p -r1.155 uipc_syscalls.c > --- kern/uipc_syscalls.c 20 Jul 2017 18:40:16 -0000 1.155 > +++ kern/uipc_syscalls.c 24 Jul 2017 09:53:13 -0000 > @@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg > 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, reg > 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, reg > 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, reg > 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) > Index: miscfs/fifofs/fifo_vnops.c > =================================================================== > RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v > retrieving revision 1.57 > diff -u -p -r1.57 fifo_vnops.c > --- miscfs/fifofs/fifo_vnops.c 8 Jul 2017 09:19:02 -0000 1.57 > +++ miscfs/fifofs/fifo_vnops.c 24 Jul 2017 09:54:45 -0000 > @@ -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); > Index: nfs/nfs_socket.c > =================================================================== > RCS file: /cvs/src/sys/nfs/nfs_socket.c,v > retrieving revision 1.119 > diff -u -p -r1.119 nfs_socket.c > --- nfs/nfs_socket.c 27 Jun 2017 12:02:43 -0000 1.119 > +++ nfs/nfs_socket.c 24 Jul 2017 09:52:01 -0000 > @@ -297,16 +297,18 @@ nfs_connect(struct nfsmount *nmp, struct > 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) &&
