Re: NET_LOCK() ordering issue
On 23/01/17(Mon) 18:45, Philip Guenther wrote:
> On Mon, Jan 23, 2017 at 4:01 PM, Martin Pieuchot wrote:
> [...]
> > @@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
> > return (error);
> >
> > headfp = fp;
> > + head = headfp->f_data;
> > +
> > + fdplock(fdp);
> > + error = falloc(p, &fp, &tmpfd);
> > + if (!error && (flags & SOCK_CLOEXEC))
> > + fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
> > + fdpunlock(fdp);
> > + if (error) {
> > + /*
> > +* Probably ran out of file descriptors. Wakeup
> > +* so some other process might have a chance at it.
> > +*/
> > + wakeup_one(&head->so_timeo);
>
> This comment and wakeup_one() call should be left out. They were
> needed in the original location because the thread had consumed the
> original wakeup from the new connection being completed at that point
> and when it gave up (because falloc() failed) it needed to recreate
> that for the next process. Now, doing the falloc() before we don't
> need or want that extra wakeup.
>
> Otherwise, looks good.
Thanks, I'm going to commit the version below then.
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.144
diff -u -p -r1.144 uipc_syscalls.c
--- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
+++ kern/uipc_syscalls.c24 Jan 2017 03:39:56 -
@@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
{
struct filedesc *fdp = p->p_fd;
struct file *fp, *headfp;
- struct mbuf *nam;
+ struct mbuf *nam = NULL;
socklen_t namelen;
int error, s, tmpfd;
struct socket *head, *so;
@@ -277,19 +277,30 @@ doaccept(struct proc *p, int sock, struc
return (error);
headfp = fp;
+
+ fdplock(fdp);
+ error = falloc(p, &fp, &tmpfd);
+ if (!error && (flags & SOCK_CLOEXEC))
+ fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
+ fdpunlock(fdp);
+ if (error) {
+ FRELE(headfp, p);
+ return (error);
+ }
+
redo:
NET_LOCK(s);
head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
error = EINVAL;
- goto bad;
+ goto out;
}
if ((head->so_state & SS_NBIO) && head->so_qlen == 0) {
if (head->so_state & SS_CANTRCVMORE)
error = ECONNABORTED;
else
error = EWOULDBLOCK;
- goto bad;
+ goto out;
}
while (head->so_qlen == 0 && head->so_error == 0) {
if (head->so_state & SS_CANTRCVMORE) {
@@ -298,34 +309,19 @@ redo:
}
error = tsleep(&head->so_timeo, PSOCK | PCATCH,
"netcon", 0);
- if (error) {
- goto bad;
- }
+ if (error)
+ goto out;
}
if (head->so_error) {
error = head->so_error;
head->so_error = 0;
- goto bad;
+ goto out;
}
/* Figure out whether the new socket should be non-blocking. */
nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
- fdplock(fdp);
- error = falloc(p, &fp, &tmpfd);
- if (error == 0 && (flags & SOCK_CLOEXEC))
- fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
- fdpunlock(fdp);
- if (error != 0) {
- /*
-* Probably ran out of file descriptors. Wakeup
-* so some other process might have a chance at it.
-*/
- wakeup_one(&head->so_timeo);
- goto bad;
- }
-
nam = m_get(M_WAIT, MT_SONAME);
/*
@@ -336,10 +332,7 @@ redo:
if (head->so_qlen == 0) {
NET_UNLOCK(s);
m_freem(nam);
- fdplock(fdp);
- fdremove(fdp, tmpfd);
- closef(fp, p);
- fdpunlock(fdp);
+ nam = NULL;
goto redo;
}
@@ -360,24 +353,20 @@ redo:
error = soaccept(so, nam);
if (!error && name != NULL)
error = copyaddrout(p, nam, name, namelen, anamelen);
-
+ if (!error) {
+ (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
+ FILE_SET_MATURE(fp, p);
+ *retval = tmpfd;
+ }
+out:
+ NET_UNLOCK(s);
+ m_freem(nam);
if (error) {
- /* if an error occurred, free the file descriptor */
- NET_UNLOCK(s);
- m_freem(nam);
fdplock(fdp);
Re: NET_LOCK() ordering issue
On Mon, Jan 23, 2017 at 4:01 PM, Martin Pieuchot wrote:
> On 24/01/17(Tue) 00:51, Alexander Bluhm wrote:
>> On Sat, Jan 21, 2017 at 10:46:42AM +1000, Martin Pieuchot wrote:
...
> Updated diff, thanks for your review.
Very close...
> --- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
> +++ kern/uipc_syscalls.c23 Jan 2017 23:59:34 -
...
> @@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
> return (error);
>
> headfp = fp;
> + head = headfp->f_data;
> +
> + fdplock(fdp);
> + error = falloc(p, &fp, &tmpfd);
> + if (!error && (flags & SOCK_CLOEXEC))
> + fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
> + fdpunlock(fdp);
> + if (error) {
> + /*
> +* Probably ran out of file descriptors. Wakeup
> +* so some other process might have a chance at it.
> +*/
> + wakeup_one(&head->so_timeo);
This comment and wakeup_one() call should be left out. They were
needed in the original location because the thread had consumed the
original wakeup from the new connection being completed at that point
and when it gave up (because falloc() failed) it needed to recreate
that for the next process. Now, doing the falloc() before we don't
need or want that extra wakeup.
Otherwise, looks good.
Philip Guenther
Re: NET_LOCK() ordering issue
On Tue, Jan 24, 2017 at 01:48:49AM +0100, Alexander Bluhm wrote:
> On Tue, Jan 24, 2017 at 10:01:02AM +1000, Martin Pieuchot wrote:
> > Updated diff, thanks for your review.
>
> > @@ -360,24 +358,20 @@ redo:
> > error = soaccept(so, nam);
> > if (!error && name != NULL)
> > error = copyaddrout(p, nam, name, namelen, anamelen);
> > -
> > + if (!error) {
> > + (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
> > + FILE_SET_MATURE(fp, p);
> > + *retval = tmpfd;
> > + }
> > +out:
>
> Perhaps a goto out would be nicer than two if (!error).
No, there is a error = copyaddrout() assignment. Please disregard
my comment.
> error = soaccept(so, nam);
> if (error)
> goto out;
> if (name != NULL)
> error = copyaddrout(p, nam, name, namelen, anamelen);
> (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
> FILE_SET_MATURE(fp, p);
> *retval = tmpfd;
> out:
>
> anyway OK bluhm@
Still OK bluhm@
Re: NET_LOCK() ordering issue
On Tue, Jan 24, 2017 at 10:01:02AM +1000, Martin Pieuchot wrote:
> Updated diff, thanks for your review.
> @@ -360,24 +358,20 @@ redo:
> error = soaccept(so, nam);
> if (!error && name != NULL)
> error = copyaddrout(p, nam, name, namelen, anamelen);
> -
> + if (!error) {
> + (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
> + FILE_SET_MATURE(fp, p);
> + *retval = tmpfd;
> + }
> +out:
Perhaps a goto out would be nicer than two if (!error).
error = soaccept(so, nam);
if (error)
goto out;
if (name != NULL)
error = copyaddrout(p, nam, name, namelen, anamelen);
(*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
FILE_SET_MATURE(fp, p);
*retval = tmpfd;
out:
anyway OK bluhm@
Re: NET_LOCK() ordering issue
On 24/01/17(Tue) 00:51, Alexander Bluhm wrote:
> On Sat, Jan 21, 2017 at 10:46:42AM +1000, Martin Pieuchot wrote:
> > Here's another way to fix the problem, call falloc() before grabbing the
> > NET_LOCK().
> >
> > Comments?
>
> There a two bugs in the "goto redo" block that is not part of the
> diff. You could do a m_freem(nam), then goto redo, get an error
> and goto out. This will result in a double m_freem(nam).
Ok, I include this fix.
> Also as you have moved the falloc() before the redo label, you must
> not fdremove() and closef() before goto redo.
Nice catch.
> You you use only if (error) and if (!error) and not mix it with
> (error != 0) and (error == 0)? That makes checking the error paths
> easier.
Fine.
Updated diff, thanks for your review.
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.144
diff -u -p -r1.144 uipc_syscalls.c
--- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
+++ kern/uipc_syscalls.c23 Jan 2017 23:59:34 -
@@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
{
struct filedesc *fdp = p->p_fd;
struct file *fp, *headfp;
- struct mbuf *nam;
+ struct mbuf *nam = NULL;
socklen_t namelen;
int error, s, tmpfd;
struct socket *head, *so;
@@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
return (error);
headfp = fp;
+ head = headfp->f_data;
+
+ fdplock(fdp);
+ error = falloc(p, &fp, &tmpfd);
+ if (!error && (flags & SOCK_CLOEXEC))
+ fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
+ fdpunlock(fdp);
+ if (error) {
+ /*
+* Probably ran out of file descriptors. Wakeup
+* so some other process might have a chance at it.
+*/
+ wakeup_one(&head->so_timeo);
+ FRELE(headfp, p);
+ return (error);
+ }
+
redo:
NET_LOCK(s);
- head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
error = EINVAL;
- goto bad;
+ goto out;
}
if ((head->so_state & SS_NBIO) && head->so_qlen == 0) {
if (head->so_state & SS_CANTRCVMORE)
error = ECONNABORTED;
else
error = EWOULDBLOCK;
- goto bad;
+ goto out;
}
while (head->so_qlen == 0 && head->so_error == 0) {
if (head->so_state & SS_CANTRCVMORE) {
@@ -298,34 +314,19 @@ redo:
}
error = tsleep(&head->so_timeo, PSOCK | PCATCH,
"netcon", 0);
- if (error) {
- goto bad;
- }
+ if (error)
+ goto out;
}
if (head->so_error) {
error = head->so_error;
head->so_error = 0;
- goto bad;
+ goto out;
}
/* Figure out whether the new socket should be non-blocking. */
nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
- fdplock(fdp);
- error = falloc(p, &fp, &tmpfd);
- if (error == 0 && (flags & SOCK_CLOEXEC))
- fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
- fdpunlock(fdp);
- if (error != 0) {
- /*
-* Probably ran out of file descriptors. Wakeup
-* so some other process might have a chance at it.
-*/
- wakeup_one(&head->so_timeo);
- goto bad;
- }
-
nam = m_get(M_WAIT, MT_SONAME);
/*
@@ -336,10 +337,7 @@ redo:
if (head->so_qlen == 0) {
NET_UNLOCK(s);
m_freem(nam);
- fdplock(fdp);
- fdremove(fdp, tmpfd);
- closef(fp, p);
- fdpunlock(fdp);
+ nam = NULL;
goto redo;
}
@@ -360,24 +358,20 @@ redo:
error = soaccept(so, nam);
if (!error && name != NULL)
error = copyaddrout(p, nam, name, namelen, anamelen);
-
+ if (!error) {
+ (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
+ FILE_SET_MATURE(fp, p);
+ *retval = tmpfd;
+ }
+out:
+ NET_UNLOCK(s);
+ m_freem(nam);
if (error) {
- /* if an error occurred, free the file descriptor */
- NET_UNLOCK(s);
- m_freem(nam);
fdplock(fdp);
fdremove(fdp, tmpfd);
closef(fp, p);
fdpunlock(fdp);
- goto out;
}
- (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
- FILE
Re: NET_LOCK() ordering issue
On Sat, Jan 21, 2017 at 10:46:42AM +1000, Martin Pieuchot wrote:
> Here's another way to fix the problem, call falloc() before grabbing the
> NET_LOCK().
>
> Comments?
There a two bugs in the "goto redo" block that is not part of the
diff. You could do a m_freem(nam), then goto redo, get an error
and goto out. This will result in a double m_freem(nam).
Also as you have moved the falloc() before the redo label, you must
not fdremove() and closef() before goto redo.
You you use only if (error) and if (!error) and not mix it with
(error != 0) and (error == 0)? That makes checking the error paths
easier.
bluhm
>
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 uipc_syscalls.c
> --- kern/uipc_syscalls.c 29 Dec 2016 12:12:43 - 1.144
> +++ kern/uipc_syscalls.c 21 Jan 2017 00:38:18 -
> @@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
> {
> struct filedesc *fdp = p->p_fd;
> struct file *fp, *headfp;
> - struct mbuf *nam;
> + struct mbuf *nam = NULL;
> socklen_t namelen;
> int error, s, tmpfd;
> struct socket *head, *so;
> @@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
> return (error);
>
> headfp = fp;
> + head = headfp->f_data;
> +
> + fdplock(fdp);
> + error = falloc(p, &fp, &tmpfd);
> + if (error == 0 && (flags & SOCK_CLOEXEC))
> + fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
> + fdpunlock(fdp);
> + if (error != 0) {
> + /*
> + * Probably ran out of file descriptors. Wakeup
> + * so some other process might have a chance at it.
> + */
> + wakeup_one(&head->so_timeo);
> + FRELE(headfp, p);
> + return (error);
> + }
> +
> redo:
> NET_LOCK(s);
> - head = headfp->f_data;
> if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
> error = EINVAL;
> - goto bad;
> + goto out;
> }
> if ((head->so_state & SS_NBIO) && head->so_qlen == 0) {
> if (head->so_state & SS_CANTRCVMORE)
> error = ECONNABORTED;
> else
> error = EWOULDBLOCK;
> - goto bad;
> + goto out;
> }
> while (head->so_qlen == 0 && head->so_error == 0) {
> if (head->so_state & SS_CANTRCVMORE) {
> @@ -298,34 +314,19 @@ redo:
> }
> error = tsleep(&head->so_timeo, PSOCK | PCATCH,
> "netcon", 0);
> - if (error) {
> - goto bad;
> - }
> + if (error)
> + goto out;
> }
> if (head->so_error) {
> error = head->so_error;
> head->so_error = 0;
> - goto bad;
> + goto out;
> }
>
> /* Figure out whether the new socket should be non-blocking. */
> nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
> : (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
>
> - fdplock(fdp);
> - error = falloc(p, &fp, &tmpfd);
> - if (error == 0 && (flags & SOCK_CLOEXEC))
> - fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
> - fdpunlock(fdp);
> - if (error != 0) {
> - /*
> - * Probably ran out of file descriptors. Wakeup
> - * so some other process might have a chance at it.
> - */
> - wakeup_one(&head->so_timeo);
> - goto bad;
> - }
> -
> nam = m_get(M_WAIT, MT_SONAME);
>
> /*
> @@ -360,24 +361,20 @@ redo:
> error = soaccept(so, nam);
> if (!error && name != NULL)
> error = copyaddrout(p, nam, name, namelen, anamelen);
> -
> - if (error) {
> - /* if an error occurred, free the file descriptor */
> - NET_UNLOCK(s);
> - m_freem(nam);
> + if (!error) {
> + (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
> + FILE_SET_MATURE(fp, p);
> + *retval = tmpfd;
> + }
> +out:
> + NET_UNLOCK(s);
> + m_freem(nam);
> + if (error != 0) {
> fdplock(fdp);
> fdremove(fdp, tmpfd);
> closef(fp, p);
> fdpunlock(fdp);
> - goto out;
> }
> - (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
> - FILE_SET_MATURE(fp, p);
> - *retval = tmpfd;
> - m_freem(nam);
> -bad:
> - NET_UNLOCK(s);
> -out:
> FRELE(headfp, p);
> return (error);
> }
Re: NET_LOCK() ordering issue
On 20/01/17(Fri) 22:25, Alexander Bluhm wrote:
> On Thu, Jan 19, 2017 at 07:14:59PM +1000, Martin Pieuchot wrote:
> > Index: kern/uipc_syscalls.c
> > ===
> > RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> > retrieving revision 1.144
> > diff -u -p -r1.144 uipc_syscalls.c
> > --- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
> > +++ kern/uipc_syscalls.c19 Jan 2017 09:03:56 -
> > @@ -278,6 +278,7 @@ doaccept(struct proc *p, int sock, struc
> >
> > headfp = fp;
> > redo:
> > + fdplock(fdp);
> > NET_LOCK(s);
> > head = headfp->f_data;
> > if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
> ...
>
> There we have
> error = rwsleep(&head->so_timeo, &netlock, PSOCK | PCATCH,
> "netcon", 0);
>
> You release the netlock during sleep but still hold the fdplock.
> This leads to a deadlock of userland during
> /usr/src/regress/lib/libpthread/socket/1.
>
> 277134 socket1 native0x83accfd8 thrsleep
> 236121 socket1 native0xff00023eab56 netcon
> 220469 socket1 native0xff0004d53a98 fdlock
>
> ddb{0}> trace /p 0t236121
> sleep_finish() at sleep_finish+0xb1
> rwsleep() at rwsleep+0x8e
> doaccept() at doaccept+0x37b
> syscall() at syscall+0x27b
> --- syscall (number 30) ---
>
> ddb{0}> trace /p 0t220469
> sleep_finish() at sleep_finish+0xb1
> rw_enter() at rw_enter+0x1cb
> sys_socket() at sys_socket+0xbf
> syscall() at syscall+0x27b
> --- syscall (number 97) ---
Here's another way to fix the problem, call falloc() before grabbing the
NET_LOCK().
Comments?
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.144
diff -u -p -r1.144 uipc_syscalls.c
--- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
+++ kern/uipc_syscalls.c21 Jan 2017 00:38:18 -
@@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
{
struct filedesc *fdp = p->p_fd;
struct file *fp, *headfp;
- struct mbuf *nam;
+ struct mbuf *nam = NULL;
socklen_t namelen;
int error, s, tmpfd;
struct socket *head, *so;
@@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
return (error);
headfp = fp;
+ head = headfp->f_data;
+
+ fdplock(fdp);
+ error = falloc(p, &fp, &tmpfd);
+ if (error == 0 && (flags & SOCK_CLOEXEC))
+ fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
+ fdpunlock(fdp);
+ if (error != 0) {
+ /*
+* Probably ran out of file descriptors. Wakeup
+* so some other process might have a chance at it.
+*/
+ wakeup_one(&head->so_timeo);
+ FRELE(headfp, p);
+ return (error);
+ }
+
redo:
NET_LOCK(s);
- head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
error = EINVAL;
- goto bad;
+ goto out;
}
if ((head->so_state & SS_NBIO) && head->so_qlen == 0) {
if (head->so_state & SS_CANTRCVMORE)
error = ECONNABORTED;
else
error = EWOULDBLOCK;
- goto bad;
+ goto out;
}
while (head->so_qlen == 0 && head->so_error == 0) {
if (head->so_state & SS_CANTRCVMORE) {
@@ -298,34 +314,19 @@ redo:
}
error = tsleep(&head->so_timeo, PSOCK | PCATCH,
"netcon", 0);
- if (error) {
- goto bad;
- }
+ if (error)
+ goto out;
}
if (head->so_error) {
error = head->so_error;
head->so_error = 0;
- goto bad;
+ goto out;
}
/* Figure out whether the new socket should be non-blocking. */
nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
- fdplock(fdp);
- error = falloc(p, &fp, &tmpfd);
- if (error == 0 && (flags & SOCK_CLOEXEC))
- fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
- fdpunlock(fdp);
- if (error != 0) {
- /*
-* Probably ran out of file descriptors. Wakeup
-* so some other process might have a chance at it.
-*/
- wakeup_one(&head->so_timeo);
- goto bad;
- }
-
nam = m_get(M_WAIT, MT_SONAME);
/*
@@ -360,24 +361,20 @@ redo:
error = soaccept(so, nam);
if (!error && name != NULL)
error = copyaddrout(p, nam, name, namelen, anamelen);
-
- if (
Re: NET_LOCK() ordering issue
On Thu, Jan 19, 2017 at 07:14:59PM +1000, Martin Pieuchot wrote:
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 uipc_syscalls.c
> --- kern/uipc_syscalls.c 29 Dec 2016 12:12:43 - 1.144
> +++ kern/uipc_syscalls.c 19 Jan 2017 09:03:56 -
> @@ -278,6 +278,7 @@ doaccept(struct proc *p, int sock, struc
>
> headfp = fp;
> redo:
> + fdplock(fdp);
> NET_LOCK(s);
> head = headfp->f_data;
> if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
...
There we have
error = rwsleep(&head->so_timeo, &netlock, PSOCK | PCATCH,
"netcon", 0);
You release the netlock during sleep but still hold the fdplock.
This leads to a deadlock of userland during
/usr/src/regress/lib/libpthread/socket/1.
277134 socket1 native0x83accfd8 thrsleep
236121 socket1 native0xff00023eab56 netcon
220469 socket1 native0xff0004d53a98 fdlock
ddb{0}> trace /p 0t236121
sleep_finish() at sleep_finish+0xb1
rwsleep() at rwsleep+0x8e
doaccept() at doaccept+0x37b
syscall() at syscall+0x27b
--- syscall (number 30) ---
ddb{0}> trace /p 0t220469
sleep_finish() at sleep_finish+0xb1
rw_enter() at rw_enter+0x1cb
sys_socket() at sys_socket+0xbf
syscall() at syscall+0x27b
--- syscall (number 97) ---
> @@ -312,11 +313,9 @@ redo:
> nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
> : (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
>
> - fdplock(fdp);
> error = falloc(p, &fp, &tmpfd);
> if (error == 0 && (flags & SOCK_CLOEXEC))
> fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
> - fdpunlock(fdp);
> if (error != 0) {
> /*
>* Probably ran out of file descriptors. Wakeup
> @@ -336,7 +335,6 @@ redo:
> if (head->so_qlen == 0) {
> NET_UNLOCK(s);
> m_freem(nam);
> - fdplock(fdp);
> fdremove(fdp, tmpfd);
> closef(fp, p);
> fdpunlock(fdp);
> @@ -365,7 +363,6 @@ redo:
> /* if an error occurred, free the file descriptor */
> NET_UNLOCK(s);
> m_freem(nam);
> - fdplock(fdp);
> fdremove(fdp, tmpfd);
> closef(fp, p);
> fdpunlock(fdp);
> @@ -377,6 +374,7 @@ redo:
> m_freem(nam);
> bad:
> NET_UNLOCK(s);
> + fdpunlock(fdp);
> out:
> FRELE(headfp, p);
> return (error);
Re: NET_LOCK() ordering issue
On Thu, Jan 19, 2017 at 07:14:59PM +1000, Martin Pieuchot wrote:
> Turns out that the NET_LOCK() related hang reported by many is a
> deadlock. One way to prevent it is to ensure that fdplock() is
> always taken before the NET_LOCK() when both have to been hold.
>
> This generates the diff below.
>
> Comments, ok?
unp_externalize() does mostly file system operations, so it does
not need the netlock. But there is one call to unp_discard(). Do
we consider unix domain sockets part of the network stack? As all
functions are running in syscall context with kernel lock, it does
not really matter. There is nothing that has to be protected in
unp_discard().
> if (pr->pr_domain->dom_externalize &&
> mtod(cm, struct cmsghdr *)->cmsg_type ==
> - SCM_RIGHTS)
> -error = (*pr->pr_domain->dom_externalize)(cm,
> -controllen, flags);
> + SCM_RIGHTS) {
> + NET_UNLOCK(s);
> + error = (*pr->pr_domain->dom_externalize)(
> + cm, controllen, flags);
> + NET_LOCK(s);
> + }
The body of the if block is 4 spaces too much left. That was wrong
before, but now the multiline condition and the multiline block
should not be on the same level. This way it would still fit
in 80 columns.
error =
(*pr->pr_domain->dom_externalize)
(cm, controllen, flags);
OK bluhm@
NET_LOCK() ordering issue
Turns out that the NET_LOCK() related hang reported by many is a
deadlock. One way to prevent it is to ensure that fdplock() is
always taken before the NET_LOCK() when both have to been hold.
This generates the diff below.
Comments, ok?
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.171
diff -u -p -r1.171 uipc_socket.c
--- kern/uipc_socket.c 29 Dec 2016 12:12:43 - 1.171
+++ kern/uipc_socket.c 19 Jan 2017 09:14:22 -
@@ -800,9 +800,12 @@ dontblock:
if (controlp) {
if (pr->pr_domain->dom_externalize &&
mtod(cm, struct cmsghdr *)->cmsg_type ==
- SCM_RIGHTS)
- error = (*pr->pr_domain->dom_externalize)(cm,
- controllen, flags);
+ SCM_RIGHTS) {
+ NET_UNLOCK(s);
+ error = (*pr->pr_domain->dom_externalize)(
+ cm, controllen, flags);
+ NET_LOCK(s);
+ }
*controlp = cm;
} else {
/*
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.144
diff -u -p -r1.144 uipc_syscalls.c
--- kern/uipc_syscalls.c29 Dec 2016 12:12:43 - 1.144
+++ kern/uipc_syscalls.c19 Jan 2017 09:03:56 -
@@ -278,6 +278,7 @@ doaccept(struct proc *p, int sock, struc
headfp = fp;
redo:
+ fdplock(fdp);
NET_LOCK(s);
head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
@@ -312,11 +313,9 @@ redo:
nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
- fdplock(fdp);
error = falloc(p, &fp, &tmpfd);
if (error == 0 && (flags & SOCK_CLOEXEC))
fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
- fdpunlock(fdp);
if (error != 0) {
/*
* Probably ran out of file descriptors. Wakeup
@@ -336,7 +335,6 @@ redo:
if (head->so_qlen == 0) {
NET_UNLOCK(s);
m_freem(nam);
- fdplock(fdp);
fdremove(fdp, tmpfd);
closef(fp, p);
fdpunlock(fdp);
@@ -365,7 +363,6 @@ redo:
/* if an error occurred, free the file descriptor */
NET_UNLOCK(s);
m_freem(nam);
- fdplock(fdp);
fdremove(fdp, tmpfd);
closef(fp, p);
fdpunlock(fdp);
@@ -377,6 +374,7 @@ redo:
m_freem(nam);
bad:
NET_UNLOCK(s);
+ fdpunlock(fdp);
out:
FRELE(headfp, p);
return (error);
