On 18/07/17(Tue) 15:49, Alexander Bluhm wrote:
> On Tue, Jul 18, 2017 at 09:03:00AM +0200, Martin Pieuchot wrote:
> > Diff below extends the scope of the socket lock. It has been previously
> > introduced in sys_connect(), first as NET_LOCK() then renamed, where old
> > splsofnet() were used. But we now need to "move the line up" in order to
> > protect fields currently relying on the KERNEL_LOCK().
>
> We were also discussing to move the lock down to the network stack.
> Eventually we need locks for the sockets and a lock for the stack.
> The first must move up and the latter down. As we have only one
> lock at the moment, I am fine with the upward direction for now.
>
> > Moving the line up means that soconnect() and soconnect2() will now
> > assert for the socket lock.
>
> The soconnect2() is only used for local sockets that run with the
> kernel lock. So we could not lock this function. I guess your
> goal is per socket locks, so your change is fine.
Updated diff addressing your comments.
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) &&
