From: Alexander Bluhm Subject: Re: Push solock() down to sobind() To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Fri, 19 Jan 2024 15:17:30 +0100 On Tue, Jan 09, 2024 at 03:39:33PM +0100, Alexander Bluhm wrote: > On Tue, Jan 09, 2024 at 04:17:27PM +0300, Vitaliy Makkoveev wrote: > > All existing sobind() calls are only wrapped by solock(). So push it > > down and convert to solock_shared() to make identical to sys_bind(). > > solock_shared() makes senses because the most of these sockets are > > UDP sockets. > > Changing to solock_shared() for NFS is risky. > > I would prefer to do the solock_shared() conversion step by step > with proper review of the sub system. Then pushing the lock down > to sobind() will be mechanical. I had a look at all the sub systems. Most have their own locking. NFS uses kernel lock so everything is serialized. OK bluhm@ > > Index: sys/kern/uipc_socket.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > > retrieving revision 1.312 > > diff -u -p -r1.312 uipc_socket.c > > --- sys/kern/uipc_socket.c 19 Dec 2023 21:34:22 -0000 1.312 > > +++ sys/kern/uipc_socket.c 9 Jan 2024 13:08:15 -0000 > > @@ -219,8 +219,13 @@ socreate(int dom, struct socket **aso, i > > int > > sobind(struct socket *so, struct mbuf *nam, struct proc *p) > > { > > - soassertlocked(so); > > - return pru_bind(so, nam, p); > > + int error; > > + > > + solock_shared(so); > > + error = pru_bind(so, nam, p); > > + sounlock_shared(so); > > + > > + return (error); > > } > > > > int > > Index: sys/kern/uipc_syscalls.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v > > retrieving revision 1.216 > > diff -u -p -r1.216 uipc_syscalls.c > > --- sys/kern/uipc_syscalls.c 3 Jan 2024 11:07:04 -0000 1.216 > > +++ sys/kern/uipc_syscalls.c 9 Jan 2024 13:08:15 -0000 > > @@ -185,9 +185,7 @@ sys_bind(struct proc *p, void *v, regist > > if (KTRPOINT(p, KTR_STRUCT)) > > ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen)); > > #endif > > - solock_shared(so); > > error = sobind(so, nam, p); > > - sounlock_shared(so); > > m_freem(nam); > > out: > > FRELE(fp, p); > > Index: sys/net/bfd.c > > =================================================================== > > RCS file: /cvs/src/sys/net/bfd.c,v > > retrieving revision 1.80 > > diff -u -p -r1.80 bfd.c > > --- sys/net/bfd.c 3 Aug 2023 09:49:08 -0000 1.80 > > +++ sys/net/bfd.c 9 Jan 2024 13:08:15 -0000 > > @@ -477,9 +477,7 @@ bfd_listener(struct bfd_config *bfd, uns > > break; > > } > > > > - solock(so); > > error = sobind(so, m, p); > > - sounlock(so); > > if (error) { > > printf("%s: sobind error %d\n", > > __func__, error); > > @@ -578,9 +576,7 @@ bfd_sender(struct bfd_config *bfd, unsig > > break; > > } > > > > - solock(so); > > error = sobind(so, m, p); > > - sounlock(so); > > if (error) { > > printf("%s: sobind error %d\n", > > __func__, error); > > Index: sys/net/if_pflow.c > > =================================================================== > > RCS file: /cvs/src/sys/net/if_pflow.c,v > > retrieving revision 1.109 > > diff -u -p -r1.109 if_pflow.c > > --- sys/net/if_pflow.c 23 Dec 2023 10:52:54 -0000 1.109 > > +++ sys/net/if_pflow.c 9 Jan 2024 13:08:15 -0000 > > @@ -467,9 +467,7 @@ pflow_set(struct pflow_softc *sc, struct > > memcpy(sa, sc->sc_flowsrc, > > sc->sc_flowsrc->sa_len); > > > > - solock(so); > > error = sobind(so, m, p); > > - sounlock(so); > > m_freem(m); > > if (error) { > > soclose(so, MSG_DONTWAIT); > > Index: sys/net/if_vxlan.c > > =================================================================== > > RCS file: /cvs/src/sys/net/if_vxlan.c,v > > retrieving revision 1.99 > > diff -u -p -r1.99 if_vxlan.c > > --- sys/net/if_vxlan.c 23 Dec 2023 10:52:54 -0000 1.99 > > +++ sys/net/if_vxlan.c 9 Jan 2024 13:08:15 -0000 > > @@ -972,9 +972,7 @@ vxlan_tep_add_addr(struct vxlan_softc *s > > unhandled_af(vt->vt_af); > > } > > > > - solock(so); > > error = sobind(so, &m, curproc); > > - sounlock(so); > > if (error != 0) > > goto close; > > > > Index: sys/net/if_wg.c > > =================================================================== > > RCS file: /cvs/src/sys/net/if_wg.c,v > > retrieving revision 1.35 > > diff -u -p -r1.35 if_wg.c > > --- sys/net/if_wg.c 1 Jan 2024 18:47:02 -0000 1.35 > > +++ sys/net/if_wg.c 9 Jan 2024 13:08:15 -0000 > > @@ -734,12 +734,11 @@ wg_socket_open(struct socket **so, int a > > sounlock(*so); > > > > if ((ret = sosetopt(*so, SOL_SOCKET, SO_RTABLE, &mrtable)) == 0) { > > - solock(*so); > > if ((ret = sobind(*so, &mhostnam, curproc)) == 0) { > > + /* No lock required. Immutable for this socket. */ > > *port = sotoinpcb(*so)->inp_lport; > > *rtable = sotoinpcb(*so)->inp_rtableid; > > } > > - sounlock(*so); > > } > > > > if (ret != 0) > > Index: sys/nfs/krpc_subr.c > > =================================================================== > > RCS file: /cvs/src/sys/nfs/krpc_subr.c,v > > retrieving revision 1.38 > > diff -u -p -r1.38 krpc_subr.c > > --- sys/nfs/krpc_subr.c 3 Aug 2023 09:49:09 -0000 1.38 > > +++ sys/nfs/krpc_subr.c 9 Jan 2024 13:08:15 -0000 > > @@ -280,9 +280,7 @@ krpc_call(struct sockaddr_in *sa, u_int > > sin->sin_family = AF_INET; > > sin->sin_addr.s_addr = INADDR_ANY; > > sin->sin_port = htons(0); > > - solock(so); > > error = sobind(so, m, &proc0); > > - sounlock(so); > > m_freem(m); > > if (error) { > > printf("bind failed\n"); > > Index: sys/nfs/nfs_socket.c > > =================================================================== > > RCS file: /cvs/src/sys/nfs/nfs_socket.c,v > > retrieving revision 1.144 > > diff -u -p -r1.144 nfs_socket.c > > --- sys/nfs/nfs_socket.c 3 Aug 2023 09:49:09 -0000 1.144 > > +++ sys/nfs/nfs_socket.c 9 Jan 2024 13:08:15 -0000 > > @@ -281,9 +281,7 @@ nfs_connect(struct nfsmount *nmp, struct > > sin->sin_family = AF_INET; > > sin->sin_addr.s_addr = INADDR_ANY; > > sin->sin_port = htons(0); > > - solock(so); > > error = sobind(so, nam, &proc0); > > - sounlock(so); > > if (error) > > goto bad; > >