From: Alexander Bluhm Subject: Re: Use `sb_mtx' to protect `sb_timeo_nsecs' To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Mon, 5 Feb 2024 21:04:43 +0100 On Mon, Feb 05, 2024 at 12:31:06PM +0300, Vitaliy Makkoveev wrote: > Start moving 'sockbuf' structure data under `sb_mtx' mutex(9). In most > places solock() is still held because other 'sockbuf' members require > it, but in so{g,s}etopt() paths solock() is avoided. > > This diff doesn't conflict with "Use `sb_mtx' instead of `inp_mtx'..." > diff. OK bluhm@ > Index: sys/kern/uipc_socket.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > retrieving revision 1.316 > diff -u -p -r1.316 uipc_socket.c > --- sys/kern/uipc_socket.c 3 Feb 2024 22:50:08 -0000 1.316 > +++ sys/kern/uipc_socket.c 5 Feb 2024 09:13:10 -0000 > @@ -1224,7 +1224,9 @@ sorflush(struct socket *so) > m = sb->sb_mb; > memset(&sb->sb_startzero, 0, > (caddr_t)&sb->sb_endzero - (caddr_t)&sb->sb_startzero); > + mtx_enter(&sb->sb_mtx); > sb->sb_timeo_nsecs = INFSLP; > + mtx_leave(&sb->sb_mtx); > sbunlock(so, sb); > if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose) > (*pr->pr_domain->dom_dispose)(m); > @@ -1907,9 +1909,9 @@ sosetopt(struct socket *so, int level, i > if (nsecs == 0) > nsecs = INFSLP; > > - solock(so); > + mtx_enter(&sb->sb_mtx); > sb->sb_timeo_nsecs = nsecs; > - sounlock(so); > + mtx_leave(&sb->sb_mtx); > break; > } > > @@ -2047,9 +2049,9 @@ sogetopt(struct socket *so, int level, i > struct timeval tv; > uint64_t nsecs; > > - solock_shared(so); > + mtx_enter(&sb->sb_mtx); > nsecs = sb->sb_timeo_nsecs; > - sounlock_shared(so); > + mtx_leave(&sb->sb_mtx); > > m->m_len = sizeof(struct timeval); > memset(&tv, 0, sizeof(tv)); > Index: sys/kern/uipc_socket2.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket2.c,v > retrieving revision 1.141 > diff -u -p -r1.141 uipc_socket2.c > --- sys/kern/uipc_socket2.c 3 Feb 2024 22:50:08 -0000 1.141 > +++ sys/kern/uipc_socket2.c 5 Feb 2024 09:13:10 -0000 > @@ -216,10 +216,14 @@ sonewconn(struct socket *head, int conns > goto fail; > so->so_snd.sb_wat = head->so_snd.sb_wat; > so->so_snd.sb_lowat = head->so_snd.sb_lowat; > + mtx_enter(&head->so_snd.sb_mtx); > so->so_snd.sb_timeo_nsecs = head->so_snd.sb_timeo_nsecs; > + mtx_leave(&head->so_snd.sb_mtx); > so->so_rcv.sb_wat = head->so_rcv.sb_wat; > so->so_rcv.sb_lowat = head->so_rcv.sb_lowat; > + mtx_enter(&head->so_rcv.sb_mtx); > so->so_rcv.sb_timeo_nsecs = head->so_rcv.sb_timeo_nsecs; > + mtx_leave(&head->so_rcv.sb_mtx); > > sigio_copy(&so->so_sigio, &head->so_sigio); > > @@ -506,15 +510,17 @@ sosleep_nsec(struct socket *so, void *id > int > sbwait(struct socket *so, struct sockbuf *sb) > { > + uint64_t timeo_nsecs; > int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH; > > soassertlocked(so); > > mtx_enter(&sb->sb_mtx); > + timeo_nsecs = sb->sb_timeo_nsecs; > sb->sb_flags |= SB_WAIT; > mtx_leave(&sb->sb_mtx); > > - return sosleep_nsec(so, &sb->sb_cc, prio, "netio", sb->sb_timeo_nsecs); > + return sosleep_nsec(so, &sb->sb_cc, prio, "netio", timeo_nsecs); > } > > int > 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 5 Feb 2024 09:13:10 -0000 > @@ -295,7 +295,6 @@ nfs_connect(struct nfsmount *nmp, struct > goto bad; > } > > - solock(so); > /* > * Protocols that do not require connections may be optionally left > * unconnected for servers that reply from a port other than NFS_PORT. > @@ -303,9 +302,10 @@ nfs_connect(struct nfsmount *nmp, struct > if (nmp->nm_flag & NFSMNT_NOCONN) { > if (nmp->nm_soflags & PR_CONNREQUIRED) { > error = ENOTCONN; > - goto bad_locked; > + goto bad; > } > } else { > + solock(so); > error = soconnect(so, nmp->nm_nam); > if (error) > goto bad_locked; > @@ -330,17 +330,21 @@ nfs_connect(struct nfsmount *nmp, struct > so->so_error = 0; > goto bad_locked; > } > + sounlock(so); > } > /* > * Always set receive timeout to detect server crash and reconnect. > * Otherwise, we can get stuck in soreceive forever. > */ > + mtx_enter(&so->so_rcv.sb_mtx); > so->so_rcv.sb_timeo_nsecs = SEC_TO_NSEC(5); > + mtx_leave(&so->so_rcv.sb_mtx); > + mtx_enter(&so->so_snd.sb_mtx); > if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT)) > so->so_snd.sb_timeo_nsecs = SEC_TO_NSEC(5); > else > so->so_snd.sb_timeo_nsecs = INFSLP; > - sounlock(so); > + mtx_leave(&so->so_snd.sb_mtx); > if (nmp->nm_sotype == SOCK_DGRAM) { > sndreserve = nmp->nm_wsize + NFS_MAXPKTHDR; > rcvreserve = (max(nmp->nm_rsize, nmp->nm_readdirsize) + > Index: sys/nfs/nfs_syscalls.c > =================================================================== > RCS file: /cvs/src/sys/nfs/nfs_syscalls.c,v > retrieving revision 1.120 > diff -u -p -r1.120 nfs_syscalls.c > --- sys/nfs/nfs_syscalls.c 12 Jan 2024 08:47:46 -0000 1.120 > +++ sys/nfs/nfs_syscalls.c 5 Feb 2024 09:13:10 -0000 > @@ -277,9 +277,13 @@ nfssvc_addsock(struct file *fp, struct m > } > solock(so); > so->so_rcv.sb_flags &= ~SB_NOINTR; > + mtx_enter(&so->so_rcv.sb_mtx); > so->so_rcv.sb_timeo_nsecs = INFSLP; > + mtx_leave(&so->so_rcv.sb_mtx); > so->so_snd.sb_flags &= ~SB_NOINTR; > + mtx_enter(&so->so_snd.sb_mtx); > so->so_snd.sb_timeo_nsecs = INFSLP; > + mtx_leave(&so->so_snd.sb_mtx); > sounlock(so); > if (tslp) > slp = tslp;