On 24/07/17(Mon) 14:34, Alexander Bluhm wrote:
> 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
Nice.
So here's a fixed diff.
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 12:58:09 -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 12:56:42 -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)
@@ -446,7 +443,7 @@ sys_socketpair(struct proc *p, void *v,
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,
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);
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 12:53:06 -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 12:53:06 -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) &&
