Re: connect(2), KERNEL_LOCK() vs socket lock
On Mon, Jul 24, 2017 at 03:06:06PM +0200, Martin Pieuchot wrote:
> So here's a fixed diff.
OK 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.c20 Jul 2017 09:49:45 - 1.196
> +++ kern/uipc_socket.c24 Jul 2017 12:58:09 -
> @@ -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 - 1.155
> +++ kern/uipc_syscalls.c 24 Jul 2017 12:56:42 -
> @@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
> if ((error = getsock(p, SCARG(uap, s), )) != 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(, 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 *), );
> - 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_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)
>
Re: connect(2), KERNEL_LOCK() vs socket lock
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 - 1.196
+++ kern/uipc_socket.c 24 Jul 2017 12:58:09 -
@@ -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.c20 Jul 2017 18:40:16 - 1.155
+++ kern/uipc_syscalls.c24 Jul 2017 12:56:42 -
@@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
if ((error = getsock(p, SCARG(uap, s), )) != 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(, 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 *), );
- 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_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,
Re: connect(2), KERNEL_LOCK() vs socket lock
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.c20 Jul 2017 09:49:45 - 1.196
> +++ kern/uipc_socket.c24 Jul 2017 09:52:01 -
> @@ -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 - 1.155
> +++ kern/uipc_syscalls.c 24 Jul 2017 09:53:13 -
> @@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
> if ((error = getsock(p, SCARG(uap, s), )) != 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(, 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 *), );
> - 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_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.c8 Jul 2017 09:19:02 - 1.57
> +++ miscfs/fifofs/fifo_vnops.c24 Jul 2017 09:54:45 -
> @@ -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);
>
Re: connect(2), KERNEL_LOCK() vs socket lock
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 - 1.196
+++ kern/uipc_socket.c 24 Jul 2017 09:52:01 -
@@ -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.c20 Jul 2017 18:40:16 - 1.155
+++ kern/uipc_syscalls.c24 Jul 2017 09:53:13 -
@@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
if ((error = getsock(p, SCARG(uap, s), )) != 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(, 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 *), );
- 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_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:
Re: connect(2), KERNEL_LOCK() vs socket lock
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.
> @@ -373,9 +373,10 @@ sys_connect(struct proc *p, void *v, reg
> if ((error = getsock(p, SCARG(uap, s), )) != 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(, SCARG(uap, name), SCARG(uap, namelen),
> MT_SONAME);
> @@ -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 *), );
> - if (error) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (error);
> - }
> + if (error)
> + goto out;
> nam->m_len = namelen;
> }
>
Now we have a mixture fo goto bad and goto out. I think everything
before soconnect() should goto out. Please change the error handling
of sockargs() and pledge_socket() to goto out.
> @@ -135,6 +135,11 @@ fifo_open(void *v)
> return (error);
> }
> fip->fi_readsock = rso;
> + /*
> + * XXXSMP
> + * We only lock `wso' because AF_LOCAL sockets are
> + * still relying on the KERNEL_LOCK().
> + */
> if ((error = socreate(AF_LOCAL, , SOCK_STREAM, 0)) != 0) {
> (void)soclose(rso);
> free(fip, M_VNODE, sizeof *fip);
Could you move this comment before the solock(wso) line?
> @@ -168,11 +179,12 @@ fifo_open(void *v)
> 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(>fi_readers);
> }
> }
> + sounlock(s);
> if ((ap->a_mode & O_NONBLOCK) == 0) {
> if ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
> VOP_UNLOCK(vp, p);
The goto bad after error = ENXIO has no sounlock().
bluhm
