On Wed, Aug 02, 2017 at 11:03:13AM +0200, Martin Pieuchot wrote:
> Diff below moves the socket lock "above" sosetopt(), sogetopt() and
> sosplice(). While this adds a lot of lock/unlock dances in NFS, they
> will be merge in a later diff.
>
> sosetopt() modifies a socket fields so it needs the lock. sogetopt()
> do not always need it, but it makes the code simpler to always grab
> it and the code section is really small anyway.
>
> ok?
OK bluhm@
> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.198
> diff -u -p -r1.198 uipc_socket.c
> --- kern/uipc_socket.c 27 Jul 2017 12:05:36 -0000 1.198
> +++ kern/uipc_socket.c 2 Aug 2017 08:21:45 -0000
> @@ -1073,7 +1073,9 @@ sosplice(struct socket *so, int fd, off_
> struct file *fp;
> struct socket *sosp;
> struct sosplice *sp;
> - int s, error = 0;
> + int error = 0;
> +
> + soassertlocked(so);
>
> if (sosplice_taskq == NULL)
> sosplice_taskq = taskq_create("sosplice", 1, IPL_SOFTNET, 0);
> @@ -1097,17 +1099,14 @@ sosplice(struct socket *so, int fd, off_
>
> /* If no fd is given, unsplice by removing existing link. */
> if (fd < 0) {
> - s = solock(so);
> /* Lock receive buffer. */
> if ((error = sblock(so, &so->so_rcv,
> (so->so_state & SS_NBIO) ? M_NOWAIT : M_WAITOK)) != 0) {
> - sounlock(s);
> return (error);
> }
> if (so->so_sp->ssp_socket)
> sounsplice(so, so->so_sp->ssp_socket, 1);
> sbunlock(&so->so_rcv);
> - sounlock(s);
> return (0);
> }
>
> @@ -1129,17 +1128,14 @@ sosplice(struct socket *so, int fd, off_
> pool_put(&sosplice_pool, sp);
> }
>
> - s = solock(so);
> /* Lock both receive and send buffer. */
> if ((error = sblock(so, &so->so_rcv,
> (so->so_state & SS_NBIO) ? M_NOWAIT : M_WAITOK)) != 0) {
> - sounlock(s);
> FRELE(fp, curproc);
> return (error);
> }
> if ((error = sblock(so, &sosp->so_snd, M_WAITOK)) != 0) {
> sbunlock(&so->so_rcv);
> - sounlock(s);
> FRELE(fp, curproc);
> return (error);
> }
> @@ -1185,7 +1181,6 @@ sosplice(struct socket *so, int fd, off_
> release:
> sbunlock(&sosp->so_snd);
> sbunlock(&so->so_rcv);
> - sounlock(s);
> FRELE(fp, curproc);
> return (error);
> }
> @@ -1565,15 +1560,15 @@ sowwakeup(struct socket *so)
> int
> sosetopt(struct socket *so, int level, int optname, struct mbuf *m0)
> {
> - int s, error = 0;
> + int error = 0;
> struct mbuf *m = m0;
>
> + soassertlocked(so);
> +
> if (level != SOL_SOCKET) {
> if (so->so_proto && so->so_proto->pr_ctloutput) {
> - s = solock(so);
> error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> level, optname, m0);
> - sounlock(s);
> return (error);
> }
> error = ENOPROTOOPT;
> @@ -1647,14 +1642,11 @@ sosetopt(struct socket *so, int level, i
> error = EINVAL;
> goto bad;
> }
> - s = solock(so);
> if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
> sbreserve(so, &so->so_snd, cnt)) {
> - sounlock(s);
> error = ENOBUFS;
> goto bad;
> }
> - sounlock(s);
> so->so_snd.sb_wat = cnt;
> break;
>
> @@ -1663,14 +1655,11 @@ sosetopt(struct socket *so, int level, i
> error = EINVAL;
> goto bad;
> }
> - s = solock(so);
> if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
> sbreserve(so, &so->so_rcv, cnt)) {
> - sounlock(s);
> error = ENOBUFS;
> goto bad;
> }
> - sounlock(s);
> so->so_rcv.sb_wat = cnt;
> break;
>
> @@ -1724,10 +1713,8 @@ sosetopt(struct socket *so, int level, i
> struct domain *dom = so->so_proto->pr_domain;
>
> level = dom->dom_protosw->pr_protocol;
> - s = solock(so);
> error = (*so->so_proto->pr_ctloutput)
> (PRCO_SETOPT, so, level, optname, m0);
> - sounlock(s);
> return (error);
> }
> error = ENOPROTOOPT;
> @@ -1756,10 +1743,8 @@ sosetopt(struct socket *so, int level, i
> break;
> }
> if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) {
> - s = solock(so);
> (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> level, optname, m0);
> - sounlock(s);
> m = NULL; /* freed by protocol */
> }
> }
> @@ -1772,18 +1757,18 @@ bad:
> int
> sogetopt(struct socket *so, int level, int optname, struct mbuf **mp)
> {
> - int s, error = 0;
> + int error = 0;
> struct mbuf *m;
>
> + soassertlocked(so);
> +
> if (level != SOL_SOCKET) {
> if (so->so_proto && so->so_proto->pr_ctloutput) {
> m = m_get(M_WAIT, MT_SOOPTS);
> m->m_len = 0;
>
> - s = solock(so);
> error = (*so->so_proto->pr_ctloutput)(PRCO_GETOPT, so,
> level, optname, m);
> - sounlock(s);
> if (error) {
> m_free(m);
> return (error);
> @@ -1869,10 +1854,8 @@ sogetopt(struct socket *so, int level, i
> struct domain *dom = so->so_proto->pr_domain;
>
> level = dom->dom_protosw->pr_protocol;
> - s = solock(so);
> error = (*so->so_proto->pr_ctloutput)
> (PRCO_GETOPT, so, level, optname, m);
> - sounlock(s);
> if (error) {
> (void)m_free(m);
> return (error);
> @@ -1886,13 +1869,10 @@ sogetopt(struct socket *so, int level, i
> case SO_SPLICE:
> {
> off_t len;
> - int s;
>
> - s = solock(so);
> m->m_len = sizeof(off_t);
> len = so->so_sp ? so->so_sp->ssp_len : 0;
> memcpy(mtod(m, off_t *), &len, sizeof(off_t));
> - sounlock(s);
> break;
> }
> #endif /* SOCKET_SPLICE */
> Index: kern/uipc_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.156
> diff -u -p -r1.156 uipc_syscalls.c
> --- kern/uipc_syscalls.c 24 Jul 2017 15:07:39 -0000 1.156
> +++ kern/uipc_syscalls.c 2 Aug 2017 08:17:41 -0000
> @@ -940,7 +940,8 @@ sys_setsockopt(struct proc *p, void *v,
> } */ *uap = v;
> struct file *fp;
> struct mbuf *m = NULL;
> - int error;
> + struct socket *so;
> + int s, error;
>
>
> if ((error = getsock(p, SCARG(uap, s), &fp)) != 0)
> @@ -972,7 +973,10 @@ sys_setsockopt(struct proc *p, void *v,
> }
> m->m_len = SCARG(uap, valsize);
> }
> - error = sosetopt(fp->f_data, SCARG(uap, level), SCARG(uap, name), m);
> + so = fp->f_data;
> + s = solock(so);
> + error = sosetopt(so, SCARG(uap, level), SCARG(uap, name), m);
> + sounlock(s);
> m = NULL;
> bad:
> m_freem(m);
> @@ -993,7 +997,8 @@ sys_getsockopt(struct proc *p, void *v,
> struct file *fp;
> struct mbuf *m = NULL;
> socklen_t valsize;
> - int error;
> + struct socket *so;
> + int s, error;
>
> if ((error = getsock(p, SCARG(uap, s), &fp)) != 0)
> return (error);
> @@ -1007,9 +1012,11 @@ sys_getsockopt(struct proc *p, void *v,
> goto out;
> } else
> valsize = 0;
> - if ((error = sogetopt(fp->f_data, SCARG(uap, level),
> - SCARG(uap, name), &m)) == 0 && SCARG(uap, val) && valsize &&
> - m != NULL) {
> + so = fp->f_data;
> + s = solock(so);
> + error = sogetopt(so, SCARG(uap, level), SCARG(uap, name), &m);
> + sounlock(s);
> + if (error == 0 && SCARG(uap, val) && valsize && m != NULL) {
> if (valsize > m->m_len)
> valsize = m->m_len;
> error = copyout(mtod(m, caddr_t), SCARG(uap, val), valsize);
> Index: nfs/krpc_subr.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/krpc_subr.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 krpc_subr.c
> --- nfs/krpc_subr.c 24 Aug 2015 14:00:29 -0000 1.29
> +++ nfs/krpc_subr.c 2 Aug 2017 08:17:41 -0000
> @@ -211,7 +211,7 @@ krpc_call(struct sockaddr_in *sa, u_int
> struct rpc_call *call;
> struct rpc_reply *reply;
> struct uio auio;
> - int error, rcvflg, timo, secs, len;
> + int s, error, rcvflg, timo, secs, len;
> static u_int32_t xid = 0;
> char addr[INET_ADDRSTRLEN];
> int *ip;
> @@ -239,7 +239,10 @@ krpc_call(struct sockaddr_in *sa, u_int
> tv.tv_usec = 0;
> memcpy(mtod(m, struct timeval *), &tv, sizeof tv);
> m->m_len = sizeof(tv);
> - if ((error = sosetopt(so, SOL_SOCKET, SO_RCVTIMEO, m)))
> + s = solock(so);
> + error = sosetopt(so, SOL_SOCKET, SO_RCVTIMEO, m);
> + sounlock(s);
> + if (error)
> goto out;
>
> /*
> @@ -251,7 +254,10 @@ krpc_call(struct sockaddr_in *sa, u_int
> on = mtod(m, int32_t *);
> m->m_len = sizeof(*on);
> *on = 1;
> - if ((error = sosetopt(so, SOL_SOCKET, SO_BROADCAST, m)))
> + s = solock(so);
> + error = sosetopt(so, SOL_SOCKET, SO_BROADCAST, m);
> + sounlock(s);
> + if (error)
> goto out;
> }
>
> @@ -264,7 +270,9 @@ krpc_call(struct sockaddr_in *sa, u_int
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> + s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + sounlock(s);
> if (error)
> goto out;
>
> @@ -286,7 +294,9 @@ krpc_call(struct sockaddr_in *sa, u_int
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> + s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + sounlock(s);
> if (error)
> goto out;
>
> Index: nfs/nfs_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
> retrieving revision 1.120
> diff -u -p -r1.120 nfs_socket.c
> --- nfs/nfs_socket.c 24 Jul 2017 15:07:39 -0000 1.120
> +++ nfs/nfs_socket.c 2 Aug 2017 08:17:41 -0000
> @@ -262,7 +262,9 @@ nfs_connect(struct nfsmount *nmp, struct
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> + s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + sounlock(s);
> if (error)
> goto bad;
>
> @@ -282,7 +284,9 @@ nfs_connect(struct nfsmount *nmp, struct
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> + s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + sounlock(s);
> if (error)
> goto bad;
> }
> @@ -331,6 +335,7 @@ nfs_connect(struct nfsmount *nmp, struct
> * Always set receive timeout to detect server crash and reconnect.
> * Otherwise, we can get stuck in soreceive forever.
> */
> + s = solock(so);
> so->so_rcv.sb_timeo = (5 * hz);
> if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
> so->so_snd.sb_timeo = (5 * hz);
> @@ -364,7 +369,6 @@ nfs_connect(struct nfsmount *nmp, struct
> rcvreserve = (nmp->nm_rsize + NFS_MAXPKTHDR +
> sizeof (u_int32_t)) * 2;
> }
> - s = solock(so);
> error = soreserve(so, sndreserve, rcvreserve);
> sounlock(s);
> if (error)
> Index: nfs/nfs_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/nfs_syscalls.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 nfs_syscalls.c
> --- nfs/nfs_syscalls.c 27 Jun 2017 12:02:43 -0000 1.109
> +++ nfs/nfs_syscalls.c 2 Aug 2017 08:17:41 -0000
> @@ -249,8 +249,8 @@ nfssvc_addsock(struct file *fp, struct m
> siz = NFS_MAXPACKET;
> s = solock(so);
> error = soreserve(so, siz, siz);
> - sounlock(s);
> if (error) {
> + sounlock(s);
> m_freem(mynam);
> return (error);
> }
> @@ -277,11 +277,11 @@ nfssvc_addsock(struct file *fp, struct m
> so->so_rcv.sb_timeo = 0;
> so->so_snd.sb_flags &= ~SB_NOINTR;
> so->so_snd.sb_timeo = 0;
> + sounlock(s);
> if (tslp)
> slp = tslp;
> else {
> - slp = malloc(sizeof(*slp), M_NFSSVC,
> - M_WAITOK|M_ZERO);
> + slp = malloc(sizeof(*slp), M_NFSSVC, M_WAITOK|M_ZERO);
> TAILQ_INSERT_TAIL(&nfssvc_sockhead, slp, ns_chain);
> }
> slp->ns_so = so;
