On Thu, Jul 13, 2023 at 02:22:17AM +0300, Vitaliy Makkoveev wrote: > This is a part of my standalone sblock() work. I need this movement > because buffers related SO_SND* and SO_RCV* socket options modification > should be protected with sblock(). However, standalone sblock() has > different lock orders with solock() for receive and send buffers. At > least sblock() for `so_snd' buffer will always be taken before solock() > in the sosend() path. > > The switch() block was split by two. SO_DONTROUTE, SO_SPLICE, SO_SND* > and SO_RCV* cases do not require to call (*pr_ctloutput)(), so they were > moved to the first switch() block solock() was pushed into each case > individually. For SO_SND* and SO_RCV* cases solock() will be replaced by > sblock() in the future. SO_RTABLE case calls (*pr_ctloutput)(), but do > this in the special way, so it was placed to the first switch() block > too. > > The second switch() block contains the cases which require to call > (*pr_ctloutput)(). solock() is taken around this block together with the > (*pr_ctloutput)() call to keep atomicy.
I did not see where (*pr_ctloutput)() is actually required. In this else level == SOL_SOCKET and all pr_ctloutput functions I could find do nothing for case SOL_SOCKET. So I do not see the cases where calling (*pr_ctloutput)() is required and where not. What did I miss? I run regress with this and a witness kernel. No fallout. These witness warnings are alway there when I run regress. http://bluhm.genua.de/regress/results/2023-07-21T13%3A08%3A59Z/bsdcons-ot29.txt [-- MARK -- Fri Jul 21 16:55:00 2023] witness: lock order reversal: 1st 0xfffffd88688efdc8 vmmaplk (&map->lock) 2nd 0xfffffd870a1b35f0 inode (&ip->i_lock) lock order "&ip->i_lock"(rrwlock) -> "&map->lock"(rwlock) first seen at: #0 rw_enter_read+0x50 #1 uvmfault_lookup+0x8a #2 uvm_fault_check+0x36 #3 uvm_fault+0xfb #4 kpageflttrap+0x14d #5 kerntrap+0x95 #6 alltraps_kern_meltdown+0x7b #7 copyout+0x57 #8 ffs_read+0x1f6 #9 VOP_READ+0x45 #10 vn_rdwr+0xa5 #11 vmcmd_map_readvn+0xb5 #12 exec_process_vmcmds+0x98 #13 sys_execve+0x7ad #14 start_init+0x26f #15 proc_trampoline+0x1c lock order "&map->lock"(rwlock) -> "&ip->i_lock"(rrwlock) first seen at: #0 rw_enter+0x71 #1 rrw_enter+0x57 #2 VOP_LOCK+0x5f #3 vn_lock+0xbd #4 vn_rdwr+0x83 #5 vndstrategy+0x2c7 #6 physio+0x237 #7 spec_read+0xac #8 VOP_READ+0x45 #9 vn_read+0xaa #10 dofilereadv+0x14a #11 sys_read+0x55 #12 syscall+0x3d4 #13 Xsyscall+0x128 [-- MARK -- Fri Jul 21 17:05:00 2023] witness: lock order reversal: 1st 0xfffffd885cd0b8e0 vmmaplk (&map->lock) 2nd 0xffff8000246587c0 nfsnode (&np->n_lock) lock order data w2 -> w1 missing lock order "&map->lock"(rwlock) -> "&np->n_lock"(rrwlock) first seen at: #0 rw_enter+0x71 #1 rrw_enter+0x57 #2 VOP_LOCK+0x5f #3 vn_lock+0xbd #4 vn_rdwr+0x83 #5 vndstrategy+0x2c7 #6 physio+0x237 #7 spec_write+0x9a #8 VOP_WRITE+0x45 #9 vn_write+0x105 #10 dofilewritev+0x151 #11 sys_pwrite+0x60 #12 syscall+0x3d4 #13 Xsyscall+0x128 > sys_setsockopt() is not the only sosetopt() caller. For such places > the solock() could be just dropped around sosetopt() call. Please note, > solock() protects only socket consistency so this doesn't brings any > atomicy loss. > > I want to receive feedback, polish the diff if required, and then I'll > ask to test the final version with bulk builds and the snaps. > > Index: sys/kern/uipc_socket.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > retrieving revision 1.305 > diff -u -p -r1.305 uipc_socket.c > --- sys/kern/uipc_socket.c 4 Jul 2023 22:28:24 -0000 1.305 > +++ sys/kern/uipc_socket.c 12 Jul 2023 23:08:02 -0000 > @@ -1789,57 +1789,23 @@ sosetopt(struct socket *so, int level, i > { > int error = 0; > > - soassertlocked(so); > - > if (level != SOL_SOCKET) { > if (so->so_proto->pr_ctloutput) { > + solock(so); > error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so, > level, optname, m); > + sounlock(so); > return (error); > } > error = ENOPROTOOPT; > } else { > switch (optname) { > - case SO_BINDANY: > - if ((error = suser(curproc)) != 0) /* XXX */ > - return (error); > - break; > - } > - > - switch (optname) { > - > - case SO_LINGER: > - if (m == NULL || m->m_len != sizeof (struct linger) || > - mtod(m, struct linger *)->l_linger < 0 || > - mtod(m, struct linger *)->l_linger > SHRT_MAX) > - return (EINVAL); > - so->so_linger = mtod(m, struct linger *)->l_linger; > - /* FALLTHROUGH */ > - > - case SO_BINDANY: > - case SO_DEBUG: > - case SO_KEEPALIVE: > - case SO_USELOOPBACK: > - case SO_BROADCAST: > - case SO_REUSEADDR: > - case SO_REUSEPORT: > - case SO_OOBINLINE: > - case SO_TIMESTAMP: > - case SO_ZEROIZE: > - if (m == NULL || m->m_len < sizeof (int)) > - return (EINVAL); > - if (*mtod(m, int *)) > - so->so_options |= optname; > - else > - so->so_options &= ~optname; > - break; > - > case SO_DONTROUTE: > if (m == NULL || m->m_len < sizeof (int)) > return (EINVAL); > if (*mtod(m, int *)) > - error = EOPNOTSUPP; > - break; > + return (EOPNOTSUPP); > + return (0); > > case SO_SNDBUF: > case SO_RCVBUF: > @@ -1853,23 +1819,32 @@ sosetopt(struct socket *so, int level, i > cnt = *mtod(m, int *); > if ((long)cnt <= 0) > cnt = 1; > - switch (optname) { > > + solock(so); > + switch (optname) { > case SO_SNDBUF: > - if (so->so_snd.sb_state & SS_CANTSENDMORE) > - return (EINVAL); > + if (so->so_snd.sb_state & SS_CANTSENDMORE) { > + error = EINVAL; > + break; > + } > if (sbcheckreserve(cnt, so->so_snd.sb_wat) || > - sbreserve(so, &so->so_snd, cnt)) > - return (ENOBUFS); > + sbreserve(so, &so->so_snd, cnt)) { > + error = ENOBUFS; > + break; > + } > so->so_snd.sb_wat = cnt; > break; > > case SO_RCVBUF: > - if (so->so_rcv.sb_state & SS_CANTRCVMORE) > - return (EINVAL); > + if (so->so_rcv.sb_state & SS_CANTRCVMORE) { > + error = EINVAL; > + break; > + } > if (sbcheckreserve(cnt, so->so_rcv.sb_wat) || > - sbreserve(so, &so->so_rcv, cnt)) > - return (ENOBUFS); > + sbreserve(so, &so->so_rcv, cnt)) { > + error = ENOBUFS; > + break; > + } > so->so_rcv.sb_wat = cnt; > break; > > @@ -1884,7 +1859,8 @@ sosetopt(struct socket *so, int level, i > so->so_rcv.sb_hiwat : cnt; > break; > } > - break; > + sounlock(so); > + return (error); > } > > case SO_SNDTIMEO: > @@ -1903,8 +1879,9 @@ sosetopt(struct socket *so, int level, i > return (EDOM); > if (nsecs == 0) > nsecs = INFSLP; > - switch (optname) { > > + solock(so); > + switch (optname) { > case SO_SNDTIMEO: > so->so_snd.sb_timeo_nsecs = nsecs; > break; > @@ -1912,7 +1889,8 @@ sosetopt(struct socket *so, int level, i > so->so_rcv.sb_timeo_nsecs = nsecs; > break; > } > - break; > + sounlock(so); > + return (0); > } > > case SO_RTABLE: > @@ -1923,19 +1901,21 @@ sosetopt(struct socket *so, int level, i > so->so_proto->pr_domain; > > level = dom->dom_protosw->pr_protocol; > + solock(so); > error = (*so->so_proto->pr_ctloutput) > (PRCO_SETOPT, so, level, optname, m); > + sounlock(so); > return (error); > } > - error = ENOPROTOOPT; > - break; > + return (ENOPROTOOPT); > > #ifdef SOCKET_SPLICE > case SO_SPLICE: > + solock(so); > if (m == NULL) { > error = sosplice(so, -1, 0, NULL); > } else if (m->m_len < sizeof(int)) { > - return (EINVAL); > + error = EINVAL; > } else if (m->m_len < sizeof(struct splice)) { > error = sosplice(so, *mtod(m, int *), 0, NULL); > } else { > @@ -1944,17 +1924,59 @@ sosetopt(struct socket *so, int level, i > mtod(m, struct splice *)->sp_max, > &mtod(m, struct splice *)->sp_idle); > } > - break; > + sounlock(so); > + return (error); > #endif /* SOCKET_SPLICE */ > + } > + > + switch (optname) { > + case SO_BINDANY: > + if ((error = suser(curproc)) != 0) /* XXX */ > + return (error); > + break; > + } > + > + solock(so); > + switch (optname) { > + case SO_LINGER: > + if (m == NULL || m->m_len != sizeof (struct linger) || > + mtod(m, struct linger *)->l_linger < 0 || > + mtod(m, struct linger *)->l_linger > SHRT_MAX) { > + error = EINVAL; > + break; > + } > + so->so_linger = mtod(m, struct linger *)->l_linger; > + /* FALLTHROUGH */ > + > + case SO_BINDANY: > + case SO_DEBUG: > + case SO_KEEPALIVE: > + case SO_USELOOPBACK: > + case SO_BROADCAST: > + case SO_REUSEADDR: > + case SO_REUSEPORT: > + case SO_OOBINLINE: > + case SO_TIMESTAMP: > + case SO_ZEROIZE: > + if (m == NULL || m->m_len < sizeof (int)) { > + error = EINVAL; > + break; > + } > + if (*mtod(m, int *)) > + so->so_options |= optname; > + else > + so->so_options &= ~optname; > + > + if (so->so_proto->pr_ctloutput) > + (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, > + so, level, optname, m); > + break; > > default: > error = ENOPROTOOPT; > break; > } > - if (error == 0 && so->so_proto->pr_ctloutput) { > - (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so, > - level, optname, m); > - } > + sounlock(so); > } > > return (error); > Index: sys/kern/uipc_syscalls.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v > retrieving revision 1.212 > diff -u -p -r1.212 uipc_syscalls.c > --- sys/kern/uipc_syscalls.c 10 Feb 2023 14:34:17 -0000 1.212 > +++ sys/kern/uipc_syscalls.c 12 Jul 2023 23:08:02 -0000 > @@ -1232,9 +1232,7 @@ sys_setsockopt(struct proc *p, void *v, > m->m_len = SCARG(uap, valsize); > } > so = fp->f_data; > - solock(so); > error = sosetopt(so, SCARG(uap, level), SCARG(uap, name), m); > - sounlock(so); > bad: > m_freem(m); > FRELE(fp, p); > Index: sys/net/bfd.c > =================================================================== > RCS file: /cvs/src/sys/net/bfd.c,v > retrieving revision 1.79 > diff -u -p -r1.79 bfd.c > --- sys/net/bfd.c 12 Jul 2023 16:10:45 -0000 1.79 > +++ sys/net/bfd.c 12 Jul 2023 23:08:02 -0000 > @@ -452,9 +452,7 @@ bfd_listener(struct bfd_config *bfd, uns > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = MAXTTL; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_MINTTL, mopt); > - sounlock(so); > m_freem(mopt); > if (error) { > printf("%s: sosetopt error %d\n", > @@ -531,9 +529,7 @@ bfd_sender(struct bfd_config *bfd, unsig > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = IP_PORTRANGE_HIGH; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt); > - sounlock(so); > m_freem(mopt); > if (error) { > printf("%s: sosetopt error %d\n", > @@ -545,9 +541,7 @@ bfd_sender(struct bfd_config *bfd, unsig > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = MAXTTL; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_TTL, mopt); > - sounlock(so); > m_freem(mopt); > if (error) { > printf("%s: sosetopt error %d\n", > @@ -559,9 +553,7 @@ bfd_sender(struct bfd_config *bfd, unsig > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = IPTOS_PREC_INTERNETCONTROL; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_TOS, mopt); > - sounlock(so); > m_freem(mopt); > if (error) { > printf("%s: sosetopt error %d\n", > Index: sys/net/if_vxlan.c > =================================================================== > RCS file: /cvs/src/sys/net/if_vxlan.c,v > retrieving revision 1.92 > diff -u -p -r1.92 if_vxlan.c > --- sys/net/if_vxlan.c 13 Apr 2023 02:19:05 -0000 1.92 > +++ sys/net/if_vxlan.c 12 Jul 2023 23:08:02 -0000 > @@ -934,9 +934,9 @@ vxlan_tep_add_addr(struct vxlan_softc *s > goto free; > > solock(so); > - > sotoinpcb(so)->inp_upcall = vxlan_input; > sotoinpcb(so)->inp_upcall_arg = vt; > + sounlock(so); > > m_inithdr(&m); > m.m_len = sizeof(vt->vt_rdomain); > @@ -973,12 +973,12 @@ 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; > > - sounlock(so); > - > rw_assert_wrlock(&vxlan_lock); > TAILQ_INSERT_TAIL(&vxlan_teps, vt, vt_entry); > > @@ -987,7 +987,6 @@ vxlan_tep_add_addr(struct vxlan_softc *s > return (0); > > close: > - sounlock(so); > soclose(so, MSG_DONTWAIT); > free: > free(vt, M_DEVBUF, sizeof(*vt)); > Index: sys/net/if_wg.c > =================================================================== > RCS file: /cvs/src/sys/net/if_wg.c,v > retrieving revision 1.28 > diff -u -p -r1.28 if_wg.c > --- sys/net/if_wg.c 1 Jun 2023 18:57:53 -0000 1.28 > +++ sys/net/if_wg.c 12 Jul 2023 23:08:02 -0000 > @@ -720,14 +720,16 @@ wg_socket_open(struct socket **so, int a > solock(*so); > sotoinpcb(*so)->inp_upcall = wg_input; > sotoinpcb(*so)->inp_upcall_arg = upcall_arg; > + sounlock(*so); > > if ((ret = sosetopt(*so, SOL_SOCKET, SO_RTABLE, &mrtable)) == 0) { > + solock(*so); > if ((ret = sobind(*so, &mhostnam, curproc)) == 0) { > *port = sotoinpcb(*so)->inp_lport; > *rtable = sotoinpcb(*so)->inp_rtableid; > } > + sounlock(*so); > } > - sounlock(*so); > > if (ret != 0) > wg_socket_close(so); > Index: sys/nfs/krpc_subr.c > =================================================================== > RCS file: /cvs/src/sys/nfs/krpc_subr.c,v > retrieving revision 1.37 > diff -u -p -r1.37 krpc_subr.c > --- sys/nfs/krpc_subr.c 6 Jun 2022 14:45:41 -0000 1.37 > +++ sys/nfs/krpc_subr.c 12 Jul 2023 23:08:02 -0000 > @@ -239,9 +239,7 @@ 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); > - solock(so); > error = sosetopt(so, SOL_SOCKET, SO_RCVTIMEO, m); > - sounlock(so); > m_freem(m); > if (error) > goto out; > @@ -255,9 +253,7 @@ krpc_call(struct sockaddr_in *sa, u_int > on = mtod(m, int32_t *); > m->m_len = sizeof(*on); > *on = 1; > - solock(so); > error = sosetopt(so, SOL_SOCKET, SO_BROADCAST, m); > - sounlock(so); > m_freem(m); > if (error) > goto out; > @@ -272,9 +268,7 @@ krpc_call(struct sockaddr_in *sa, u_int > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = IP_PORTRANGE_LOW; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt); > - sounlock(so); > m_freem(mopt); > if (error) > goto out; > @@ -299,9 +293,7 @@ krpc_call(struct sockaddr_in *sa, u_int > mopt->m_len = sizeof(int); > ip = mtod(mopt, int *); > *ip = IP_PORTRANGE_DEFAULT; > - solock(so); > error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt); > - sounlock(so); > m_freem(mopt); > if (error) > goto out; > Index: sys/nfs/nfs_socket.c > =================================================================== > RCS file: /cvs/src/sys/nfs/nfs_socket.c,v > retrieving revision 1.143 > diff -u -p -r1.143 nfs_socket.c > --- sys/nfs/nfs_socket.c 13 Aug 2022 21:01:46 -0000 1.143 > +++ sys/nfs/nfs_socket.c 12 Jul 2023 23:08:02 -0000 > @@ -258,7 +258,6 @@ nfs_connect(struct nfsmount *nmp, struct > MGET(nam, M_WAIT, MT_SONAME); > > so = nmp->nm_so; > - solock(so); > nmp->nm_soflags = so->so_proto->pr_flags; > > /* > @@ -282,7 +281,9 @@ 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; > > @@ -294,6 +295,7 @@ 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. > @@ -301,12 +303,12 @@ nfs_connect(struct nfsmount *nmp, struct > if (nmp->nm_flag & NFSMNT_NOCONN) { > if (nmp->nm_soflags & PR_CONNREQUIRED) { > error = ENOTCONN; > - goto bad; > + goto bad_locked; > } > } else { > error = soconnect(so, nmp->nm_nam); > if (error) > - goto bad; > + goto bad_locked; > > /* > * Wait for the connection to complete. Cribbed from the > @@ -320,13 +322,13 @@ nfs_connect(struct nfsmount *nmp, struct > so->so_error == 0 && rep && > (error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){ > so->so_state &= ~SS_ISCONNECTING; > - goto bad; > + goto bad_locked; > } > } > if (so->so_error) { > error = so->so_error; > so->so_error = 0; > - goto bad; > + goto bad_locked; > } > } > /* > @@ -338,6 +340,7 @@ nfs_connect(struct nfsmount *nmp, struct > so->so_snd.sb_timeo_nsecs = SEC_TO_NSEC(5); > else > so->so_snd.sb_timeo_nsecs = INFSLP; > + sounlock(so); > if (nmp->nm_sotype == SOCK_DGRAM) { > sndreserve = nmp->nm_wsize + NFS_MAXPKTHDR; > rcvreserve = (max(nmp->nm_rsize, nmp->nm_readdirsize) + > @@ -360,9 +363,10 @@ nfs_connect(struct nfsmount *nmp, struct > } else { > panic("%s: nm_sotype %d", __func__, nmp->nm_sotype); > } > + solock(so); > error = soreserve(so, sndreserve, rcvreserve); > if (error) > - goto bad; > + goto bad_locked; > so->so_rcv.sb_flags |= SB_NOINTR; > so->so_snd.sb_flags |= SB_NOINTR; > sounlock(so); > @@ -377,8 +381,9 @@ nfs_connect(struct nfsmount *nmp, struct > nmp->nm_timeouts = 0; > return (0); > > -bad: > +bad_locked: > sounlock(so); > +bad: > > m_freem(mopt); > m_freem(nam); > Index: sys/nfs/nfs_syscalls.c > =================================================================== > RCS file: /cvs/src/sys/nfs/nfs_syscalls.c,v > retrieving revision 1.118 > diff -u -p -r1.118 nfs_syscalls.c > --- sys/nfs/nfs_syscalls.c 6 Jun 2022 14:45:41 -0000 1.118 > +++ sys/nfs/nfs_syscalls.c 12 Jul 2023 23:08:02 -0000 > @@ -249,8 +249,8 @@ nfssvc_addsock(struct file *fp, struct m > siz = NFS_MAXPACKET; > solock(so); > error = soreserve(so, siz, siz); > + sounlock(so); > if (error) { > - sounlock(so); > m_freem(mynam); > return (error); > } > @@ -275,6 +275,7 @@ nfssvc_addsock(struct file *fp, struct m > sosetopt(so, IPPROTO_TCP, TCP_NODELAY, m); > m_freem(m); > } > + solock(so); > so->so_rcv.sb_flags &= ~SB_NOINTR; > so->so_rcv.sb_timeo_nsecs = INFSLP; > so->so_snd.sb_flags &= ~SB_NOINTR;
