On 27/05/17(Sat) 11:34, Sebastian Benoit wrote:
> (benno_pflow_try3_2_locked_arg.diff)
>
> Introduce a 'locked' argument to sobind(), socreate() and soclose() to
> indicate if its called with a lock held.
> In pflow in the ioctl path, these functions can now be used without
> giving up the netlock first.
I don't see a reason why we should do this.
> diff --git sys/kern/sys_socket.c sys/kern/sys_socket.c
> index 0a225cb95ec..4c6d9bcf516 100644
> --- sys/kern/sys_socket.c
> +++ sys/kern/sys_socket.c
> @@ -200,7 +200,7 @@ soo_close(struct file *fp, struct proc *p)
> int error = 0;
>
> if (fp->f_data)
> - error = soclose(fp->f_data);
> + error = soclose(fp->f_data, 0);
> fp->f_data = 0;
> return (error);
> }
> diff --git sys/kern/uipc_socket.c sys/kern/uipc_socket.c
> index 29323e8a41f..d2973ccf053 100644
> --- sys/kern/uipc_socket.c
> +++ sys/kern/uipc_socket.c
> @@ -108,7 +108,7 @@ soinit(void)
> * switching out to the protocol specific routines.
> */
> int
> -socreate(int dom, struct socket **aso, int type, int proto)
> +socreate(int dom, struct socket **aso, int type, int proto, int locked)
> {
> struct proc *p = curproc; /* XXX */
> struct protosw *prp;
> @@ -136,27 +136,32 @@ socreate(int dom, struct socket **aso, int type, int
> proto)
> so->so_cpid = p->p_p->ps_pid;
> so->so_proto = prp;
>
> - s = solock(so);
> + if (!locked)
> + s = solock(so);
> error = (*prp->pr_attach)(so, proto);
> if (error) {
> so->so_state |= SS_NOFDREF;
> sofree(so);
> - sounlock(s);
> + if (!locked)
> + sounlock(s);
> return (error);
> }
> - sounlock(s);
> + if (!locked)
> + sounlock(s);
> *aso = so;
> return (0);
> }
>
> int
> -sobind(struct socket *so, struct mbuf *nam, struct proc *p)
> +sobind(struct socket *so, struct mbuf *nam, struct proc *p, int locked)
> {
> int s, error;
>
> - s = solock(so);
> + if (!locked)
> + s = solock(so);
> error = (*so->so_proto->pr_usrreq)(so, PRU_BIND, NULL, nam, NULL, p);
> - sounlock(s);
> + if (!locked)
> + sounlock(s);
> return (error);
> }
>
> @@ -227,12 +232,13 @@ sofree(struct socket *so)
> * Free socket when disconnect complete.
> */
> int
> -soclose(struct socket *so)
> +soclose(struct socket *so, int locked)
> {
> struct socket *so2;
> int s, error = 0;
>
> - s = solock(so);
> + if (!locked)
> + s = solock(so);
> if (so->so_options & SO_ACCEPTCONN) {
> while ((so2 = TAILQ_FIRST(&so->so_q0)) != NULL) {
> (void) soqremque(so2, 0);
> @@ -273,10 +279,12 @@ drop:
> }
> discard:
> if (so->so_state & SS_NOFDREF)
> - panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
> + panic("soclose NOFDREF: so %p, so_type %d locked %d", so,
> + so->so_type, locked);
> so->so_state |= SS_NOFDREF;
> sofree(so);
> - sounlock(s);
> + if (!locked)
> + sounlock(s);
> return (error);
> }
>
> diff --git sys/kern/uipc_syscalls.c sys/kern/uipc_syscalls.c
> index acf671a65f3..587da137dff 100644
> --- sys/kern/uipc_syscalls.c
> +++ sys/kern/uipc_syscalls.c
> @@ -103,7 +103,8 @@ sys_socket(struct proc *p, void *v, register_t *retval)
> fp->f_type = DTYPE_SOCKET;
> fp->f_ops = &socketops;
> error = socreate(SCARG(uap, domain), &so,
> - type & ~(SOCK_CLOEXEC | SOCK_NONBLOCK | SOCK_DNS), SCARG(uap,
> protocol));
> + type & ~(SOCK_CLOEXEC | SOCK_NONBLOCK | SOCK_DNS),
> + SCARG(uap, protocol), 0);
> if (error) {
> fdplock(fdp);
> fdremove(fdp, fd);
> @@ -201,7 +202,7 @@ sys_bind(struct proc *p, void *v, register_t *retval)
> if (KTRPOINT(p, KTR_STRUCT))
> ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
> #endif
> - error = sobind(so, nam, p);
> + error = sobind(so, nam, p, 0);
> m_freem(nam);
> out:
> FRELE(fp, p);
> @@ -450,10 +451,12 @@ sys_socketpair(struct proc *p, void *v, register_t
> *retval)
> nonblock = SCARG(uap, type) & SOCK_NONBLOCK;
> fflag = FREAD | FWRITE | (nonblock ? FNONBLOCK : 0);
>
> - error = socreate(SCARG(uap, domain), &so1, type, SCARG(uap, protocol));
> + error = socreate(SCARG(uap, domain), &so1, type,
> + SCARG(uap, protocol), 0);
> if (error)
> return (error);
> - error = socreate(SCARG(uap, domain), &so2, type, SCARG(uap, protocol));
> + error = socreate(SCARG(uap, domain), &so2, type,
> + SCARG(uap, protocol), 0);
> if (error)
> goto free1;
>
> @@ -508,10 +511,10 @@ free3:
> fdpunlock(fdp);
> free2:
> if (so2 != NULL)
> - (void)soclose(so2);
> + (void)soclose(so2, 0);
> free1:
> if (so1 != NULL)
> - (void)soclose(so1);
> + (void)soclose(so1, 0);
> return (error);
> }
>
> diff --git sys/miscfs/fifofs/fifo_vnops.c sys/miscfs/fifofs/fifo_vnops.c
> index 6ffecdcd5fe..a7cd911318c 100644
> --- sys/miscfs/fifofs/fifo_vnops.c
> +++ sys/miscfs/fifofs/fifo_vnops.c
> @@ -129,22 +129,22 @@ fifo_open(void *v)
> if ((fip = vp->v_fifoinfo) == NULL) {
> fip = malloc(sizeof(*fip), M_VNODE, M_WAITOK);
> vp->v_fifoinfo = fip;
> - if ((error = socreate(AF_LOCAL, &rso, SOCK_STREAM, 0)) != 0) {
> + if ((error = socreate(AF_LOCAL, &rso, SOCK_STREAM, 0, 0)) != 0)
> {
> free(fip, M_VNODE, sizeof *fip);
> vp->v_fifoinfo = NULL;
> return (error);
> }
> fip->fi_readsock = rso;
> - if ((error = socreate(AF_LOCAL, &wso, SOCK_STREAM, 0)) != 0) {
> - (void)soclose(rso);
> + if ((error = socreate(AF_LOCAL, &wso, SOCK_STREAM, 0, 0)) != 0)
> {
> + (void)soclose(rso, 0);
> free(fip, M_VNODE, sizeof *fip);
> vp->v_fifoinfo = NULL;
> return (error);
> }
> fip->fi_writesock = wso;
> if ((error = soconnect2(wso, rso)) != 0) {
> - (void)soclose(wso);
> - (void)soclose(rso);
> + (void)soclose(wso, 0);
> + (void)soclose(rso, 0);
> free(fip, M_VNODE, sizeof *fip);
> vp->v_fifoinfo = NULL;
> return (error);
> @@ -371,8 +371,8 @@ fifo_close(void *v)
> }
> }
> if (fip->fi_readers == 0 && fip->fi_writers == 0) {
> - error1 = soclose(fip->fi_readsock);
> - error2 = soclose(fip->fi_writesock);
> + error1 = soclose(fip->fi_readsock, 0);
> + error2 = soclose(fip->fi_writesock, 0);
> free(fip, M_VNODE, sizeof *fip);
> vp->v_fifoinfo = NULL;
> }
> @@ -389,8 +389,8 @@ fifo_reclaim(void *v)
> if (fip == NULL)
> return (0);
>
> - soclose(fip->fi_readsock);
> - soclose(fip->fi_writesock);
> + soclose(fip->fi_readsock, 0);
> + soclose(fip->fi_writesock, 0);
> free(fip, M_VNODE, sizeof *fip);
> vp->v_fifoinfo = NULL;
>
> diff --git sys/net/bfd.c sys/net/bfd.c
> index 26f487555de..9ad81167167 100644
> --- sys/net/bfd.c
> +++ sys/net/bfd.c
> @@ -254,14 +254,14 @@ bfd_clear_task(void *arg)
> if (bfd->bc_so) {
> /* remove upcall before calling soclose or it will be called */
> bfd->bc_so->so_upcall = NULL;
> - soclose(bfd->bc_so);
> + soclose(bfd->bc_so, 0);
> }
> if (bfd->bc_soecho) {
> bfd->bc_soecho->so_upcall = NULL;
> - soclose(bfd->bc_soecho);
> + soclose(bfd->bc_soecho, 0);
> }
> if (bfd->bc_sosend)
> - soclose(bfd->bc_sosend);
> + soclose(bfd->bc_sosend, 0);
>
> rtfree(bfd->bc_rt);
> bfd->bc_rt = NULL;
> @@ -442,7 +442,7 @@ bfd_listener(struct bfd_config *bfd, unsigned int port)
> if (src->sa_family != dst->sa_family || src->sa_len != dst->sa_len)
> return (NULL);
>
> - error = socreate(dst->sa_family, &so, SOCK_DGRAM, 0);
> + error = socreate(dst->sa_family, &so, SOCK_DGRAM, 0, 0);
> if (error) {
> printf("%s: socreate error %d\n",
> __func__, error);
> @@ -477,7 +477,7 @@ bfd_listener(struct bfd_config *bfd, unsigned int port)
> break;
> }
>
> - error = sobind(so, m, p);
> + error = sobind(so, m, p, 0);
> if (error) {
> printf("%s: sobind error %d\n",
> __func__, error);
> @@ -490,7 +490,7 @@ bfd_listener(struct bfd_config *bfd, unsigned int port)
>
> close:
> m_free(m);
> - soclose(so);
> + soclose(so, 0);
>
> return (NULL);
> }
> @@ -516,7 +516,7 @@ bfd_sender(struct bfd_config *bfd, unsigned int port)
> if (src->sa_family != dst->sa_family || src->sa_len != dst->sa_len)
> return (NULL);
>
> - error = socreate(dst->sa_family, &so, SOCK_DGRAM, 0);
> + error = socreate(dst->sa_family, &so, SOCK_DGRAM, 0, 0);
>
> if (error)
> return (NULL);
> @@ -571,7 +571,7 @@ bfd_sender(struct bfd_config *bfd, unsigned int port)
> break;
> }
>
> - error = sobind(so, m, p);
> + error = sobind(so, m, p, 0);
> if (error) {
> printf("%s: sobind error %d\n",
> __func__, error);
> @@ -605,7 +605,7 @@ bfd_sender(struct bfd_config *bfd, unsigned int port)
>
> close:
> m_free(m);
> - soclose(so);
> + soclose(so, 0);
>
> return (NULL);
> }
> diff --git sys/net/if_pflow.c sys/net/if_pflow.c
> index 8cfffa1e4e7..68495eb885d 100644
> --- sys/net/if_pflow.c
> +++ sys/net/if_pflow.c
> @@ -295,7 +295,7 @@ pflow_clone_destroy(struct ifnet *ifp)
> pflow_flush(sc);
> m_freem(sc->send_nam);
> if (sc->so != NULL) {
> - error = soclose(sc->so);
> + error = soclose(sc->so, 1);
> sc->so = NULL;
> }
> if (sc->sc_flowdst != NULL)
> @@ -336,7 +336,7 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
> struct proc *p = curproc;
> struct socket *so;
> struct sockaddr *sa;
> - int s, error = 0;
> + int error = 0;
>
> if (pflowr->addrmask & PFLOW_MASK_VERSION) {
> switch(pflowr->version) {
> @@ -356,7 +356,7 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
> free(sc->sc_flowdst, M_DEVBUF, sc->sc_flowdst->sa_len);
> sc->sc_flowdst = NULL;
> if (sc->so != NULL) {
> - soclose(sc->so);
> + soclose(sc->so, 1);
> sc->so = NULL;
> }
> }
> @@ -402,7 +402,7 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
> free(sc->sc_flowsrc, M_DEVBUF, sc->sc_flowsrc->sa_len);
> sc->sc_flowsrc = NULL;
> if (sc->so != NULL) {
> - soclose(sc->so);
> + soclose(sc->so, 1);
> sc->so = NULL;
> }
> switch(pflowr->flowsrc.ss_family) {
> @@ -432,11 +432,9 @@ pflow_set(struct pflow_softc *sc, struct pflowreq
> *pflowr)
> }
>
> if (sc->so == NULL) {
> - rw_exit_write(&netlock);
> - s = splnet();
> if (pflowvalidsockaddr(sc->sc_flowdst, 0)) {
> error = socreate(sc->sc_flowdst->sa_family,
> - &so, SOCK_DGRAM, 0);
> + &so, SOCK_DGRAM, 0, 1);
> if (error)
> return (error);
> if (pflowvalidsockaddr(sc->sc_flowsrc, 1)) {
> @@ -448,24 +446,18 @@ pflow_set(struct pflow_softc *sc, struct pflowreq
> *pflowr)
> memcpy(sa, sc->sc_flowsrc,
> sc->sc_flowsrc->sa_len);
>
> - error = sobind(so, m, p);
> + error = sobind(so, m, p, 1);
> m_freem(m);
> if (error) {
> - soclose(so);
> + soclose(so, 1);
> return (error);
> }
> }
> sc->so = so;
> }
> - splx(s);
> - rw_enter_write(&netlock);
> } else if (!pflowvalidsockaddr(sc->sc_flowdst, 0)) {
> - rw_exit_write(&netlock);
> - s = splnet();
> - soclose(sc->so);
> + soclose(sc->so, 1);
> sc->so = NULL;
> - splx(s);
> - rw_enter_write(&netlock);
> }
>
> /* error check is above */
> diff --git sys/nfs/krpc_subr.c sys/nfs/krpc_subr.c
> index eddad29015a..60f8b2d7619 100644
> --- sys/nfs/krpc_subr.c
> +++ sys/nfs/krpc_subr.c
> @@ -231,7 +231,7 @@ krpc_call(struct sockaddr_in *sa, u_int prog, u_int vers,
> u_int func,
> /*
> * Create socket and set its receive timeout.
> */
> - if ((error = socreate(AF_INET, &so, SOCK_DGRAM, 0)))
> + if ((error = socreate(AF_INET, &so, SOCK_DGRAM, 0, 0)))
> goto out;
>
> m = m_get(M_WAIT, MT_SOOPTS);
> @@ -275,7 +275,7 @@ krpc_call(struct sockaddr_in *sa, u_int prog, u_int vers,
> u_int func,
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = INADDR_ANY;
> sin->sin_port = htons(0);
> - error = sobind(so, m, &proc0);
> + error = sobind(so, m, &proc0, 0);
> m_freem(m);
> if (error) {
> printf("bind failed\n");
> @@ -453,7 +453,7 @@ krpc_call(struct sockaddr_in *sa, u_int prog, u_int vers,
> u_int func,
> m_freem(nam);
> m_freem(mhead);
> m_freem(from);
> - soclose(so);
> + soclose(so, 0);
> return error;
> }
>
> diff --git sys/nfs/nfs_boot.c sys/nfs/nfs_boot.c
> index f0bf6359b2f..1bbddf12fc6 100644
> --- sys/nfs/nfs_boot.c
> +++ sys/nfs/nfs_boot.c
> @@ -157,7 +157,7 @@ nfs_boot_init(struct nfs_diskless *nd, struct proc *procp)
> * Get the old interface flags and or IFF_UP into them; if
> * IFF_UP set blindly, interface selection can be clobbered.
> */
> - if ((error = socreate(AF_INET, &so, SOCK_DGRAM, 0)) != 0)
> + if ((error = socreate(AF_INET, &so, SOCK_DGRAM, 0, 0)) != 0)
> panic("nfs_boot: socreate, error=%d", error);
> NET_LOCK(s);
> error = ifioctl(so, SIOCGIFFLAGS, (caddr_t)&ireq, procp);
> @@ -196,7 +196,7 @@ nfs_boot_init(struct nfs_diskless *nd, struct proc *procp)
> if (error)
> panic("nfs_boot: set if addr, error=%d", error);
>
> - soclose(so);
> + soclose(so, 0);
>
> TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> if (ifa->ifa_addr->sa_family == AF_INET)
> diff --git sys/nfs/nfs_socket.c sys/nfs/nfs_socket.c
> index 619951ba1d5..115ba0b8532 100644
> --- sys/nfs/nfs_socket.c
> +++ sys/nfs/nfs_socket.c
> @@ -243,7 +243,7 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
> nmp->nm_so = NULL;
> saddr = mtod(nmp->nm_nam, struct sockaddr *);
> error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
> - nmp->nm_soproto);
> + nmp->nm_soproto, 0);
> if (error)
> goto bad;
> so = nmp->nm_so;
> @@ -273,7 +273,7 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = INADDR_ANY;
> sin->sin_port = htons(0);
> - error = sobind(so, m, &proc0);
> + error = sobind(so, m, &proc0, 0);
> m_freem(m);
> if (error)
> goto bad;
> @@ -426,7 +426,7 @@ nfs_disconnect(struct nfsmount *nmp)
> so = nmp->nm_so;
> nmp->nm_so = NULL;
> soshutdown(so, SHUT_RDWR);
> - soclose(so);
> + soclose(so, 0);
> }
> }
>
> diff --git sys/sys/socketvar.h sys/sys/socketvar.h
> index 32967536f53..d028fc7e869 100644
> --- sys/sys/socketvar.h
> +++ sys/sys/socketvar.h
> @@ -281,13 +281,13 @@ int sb_lock(struct sockbuf *sb);
> void soinit(void);
> int soabort(struct socket *so);
> int soaccept(struct socket *so, struct mbuf *nam);
> -int sobind(struct socket *so, struct mbuf *nam, struct proc *p);
> +int sobind(struct socket *so, struct mbuf *nam, struct proc *p, int locked);
> void socantrcvmore(struct socket *so);
> void socantsendmore(struct socket *so);
> -int soclose(struct socket *so);
> +int soclose(struct socket *so, int locked);
> int soconnect(struct socket *so, struct mbuf *nam);
> int soconnect2(struct socket *so1, struct socket *so2);
> -int socreate(int dom, struct socket **aso, int type, int proto);
> +int socreate(int dom, struct socket **aso, int type, int proto, int locked);
> int sodisconnect(struct socket *so);
> void sofree(struct socket *so);
> int sogetopt(struct socket *so, int level, int optname,
>
