From: Vitaliy Makkoveev Subject: Re: unlock tcp drop sysctl To: Alexander Bluhm Cc: tech@openbsd.org Date: Thu, 9 Jan 2025 19:00:42 +0300 On Wed, Jan 08, 2025 at 06:30:35PM +0100, Alexander Bluhm wrote: > On Tue, Jan 07, 2025 at 12:47:30AM +0100, Alexander Bluhm wrote: > > Hi, > > > > Running TCP drop sysctl with shared net lock, allows to test socket > > locking directly from userland. > > > > mvs: I think you are right, moving sorele() from in_pcbsolock() to > > in_pcbsounlock() is a good idea. > > > > tcp_ident() uses shared net lock together with socket lock to call > > tcp_drop(). While there improve cosistency check of address family. > > > > ok? > > > > bluhm > > mvs@ suggested a while ago to add _ref and _rele to the in_pcb..solock > functions. In addition to locking they also do refcounting. > > As we protect tcp_drop() and tcp_close(), the inp->inp_socket in > in_pcbsounlock_rele() can be NULL now. > > ok? > ok mvs@ Would you like to split tcp_sysctl() with tcp_sysctl_locked() and start slightly moving some paths out of sysctl lock? > bluhm > > Index: netinet/in_pcb.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v > diff -u -p -r1.309 in_pcb.c > --- netinet/in_pcb.c 3 Jan 2025 12:56:15 -0000 1.309 > +++ netinet/in_pcb.c 8 Jan 2025 16:22:37 -0000 > @@ -623,7 +623,7 @@ in_pcbdetach(struct inpcb *inp) > } > > struct socket * > -in_pcbsolock(struct inpcb *inp) > +in_pcbsolock_ref(struct inpcb *inp) > { > struct socket *so; > > @@ -634,18 +634,18 @@ in_pcbsolock(struct inpcb *inp) > mtx_leave(&inp->inp_sofree_mtx); > if (so == NULL) > return NULL; > - > rw_enter_write(&so->so_lock); > - sorele(so); > - > return so; > } > > void > -in_pcbsounlock(struct inpcb *inp, struct socket *so) > +in_pcbsounlock_rele(struct inpcb *inp, struct socket *so) > { > - KASSERT(inp->inp_socket == so); > + if (so == NULL) > + return; > + KASSERT(inp->inp_socket == NULL || inp->inp_socket == so); > rw_exit_write(&so->so_lock); > + sorele(so); > } > > struct inpcb * > Index: netinet/in_pcb.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v > diff -u -p -r1.163 in_pcb.h > --- netinet/in_pcb.h 5 Jan 2025 12:10:39 -0000 1.163 > +++ netinet/in_pcb.h 8 Jan 2025 16:23:17 -0000 > @@ -311,8 +311,8 @@ int in_pcbaddrisavail(const struct inpc > int in_pcbconnect(struct inpcb *, struct mbuf *); > void in_pcbdetach(struct inpcb *); > struct socket * > - in_pcbsolock(struct inpcb *); > -void in_pcbsounlock(struct inpcb *, struct socket *); > + in_pcbsolock_ref(struct inpcb *); > +void in_pcbsounlock_rele(struct inpcb *, struct socket *); > struct inpcb * > in_pcbref(struct inpcb *); > void in_pcbunref(struct inpcb *); > Index: netinet/tcp_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v > diff -u -p -r1.420 tcp_input.c > --- netinet/tcp_input.c 5 Jan 2025 12:18:48 -0000 1.420 > +++ netinet/tcp_input.c 8 Jan 2025 16:24:01 -0000 > @@ -3410,7 +3410,7 @@ syn_cache_timer(void *arg) > mtx_leave(&syn_cache_mtx); > > NET_LOCK_SHARED(); > - so = in_pcbsolock(inp); > + so = in_pcbsolock_ref(inp); > if (so != NULL) { > now = tcp_now(); > #ifdef TCP_ECN > @@ -3418,8 +3418,8 @@ syn_cache_timer(void *arg) > #endif > (void) syn_cache_respond(sc, NULL, now, do_ecn); > tcpstat_inc(tcps_sc_retransmitted); > - in_pcbsounlock(inp, so); > } > + in_pcbsounlock_rele(inp, so); > NET_UNLOCK_SHARED(); > > in_pcbunref(inp); > Index: netinet/tcp_usrreq.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_usrreq.c,v > diff -u -p -r1.238 tcp_usrreq.c > --- netinet/tcp_usrreq.c 5 Jan 2025 12:23:38 -0000 1.238 > +++ netinet/tcp_usrreq.c 8 Jan 2025 16:25:17 -0000 > @@ -1122,15 +1122,13 @@ tcp_ident(void *oldp, size_t *oldlenp, v > int error = 0; > struct tcp_ident_mapping tir; > struct inpcb *inp; > - struct tcpcb *tp = NULL; > + struct socket *so = NULL; > struct sockaddr_in *fin, *lin; > #ifdef INET6 > struct sockaddr_in6 *fin6, *lin6; > struct in6_addr f6, l6; > #endif > > - NET_ASSERT_LOCKED(); > - > if (dodrop) { > if (oldp != NULL || *oldlenp != 0) > return (EINVAL); > @@ -1150,25 +1148,41 @@ tcp_ident(void *oldp, size_t *oldlenp, v > if ((error = copyin(oldp, &tir, sizeof (tir))) != 0 ) > return (error); > } > + > + NET_LOCK_SHARED(); > + > switch (tir.faddr.ss_family) { > #ifdef INET6 > case AF_INET6: > + if (tir.laddr.ss_family != AF_INET6) { > + NET_UNLOCK_SHARED(); > + return (EAFNOSUPPORT); > + } > fin6 = (struct sockaddr_in6 *)&tir.faddr; > error = in6_embedscope(&f6, fin6, NULL, NULL); > - if (error) > + if (error) { > + NET_UNLOCK_SHARED(); > return EINVAL; /*?*/ > + } > lin6 = (struct sockaddr_in6 *)&tir.laddr; > error = in6_embedscope(&l6, lin6, NULL, NULL); > - if (error) > + if (error) { > + NET_UNLOCK_SHARED(); > return EINVAL; /*?*/ > + } > break; > #endif > case AF_INET: > + if (tir.laddr.ss_family != AF_INET) { > + NET_UNLOCK_SHARED(); > + return (EAFNOSUPPORT); > + } > fin = (struct sockaddr_in *)&tir.faddr; > lin = (struct sockaddr_in *)&tir.laddr; > break; > default: > - return (EINVAL); > + NET_UNLOCK_SHARED(); > + return (EAFNOSUPPORT); > } > > switch (tir.faddr.ss_family) { > @@ -1187,11 +1201,20 @@ tcp_ident(void *oldp, size_t *oldlenp, v > } > > if (dodrop) { > - if (inp && (tp = intotcpcb(inp)) && > - ((inp->inp_socket->so_options & SO_ACCEPTCONN) == 0)) > + struct tcpcb *tp = NULL; > + > + if (inp != NULL) { > + so = in_pcbsolock_ref(inp); > + if (so != NULL) > + tp = intotcpcb(inp); > + } > + if (tp != NULL && !ISSET(so->so_options, SO_ACCEPTCONN)) > tp = tcp_drop(tp, ECONNABORTED); > else > error = ESRCH; > + > + in_pcbsounlock_rele(inp, so); > + NET_UNLOCK_SHARED(); > in_pcbunref(inp); > return (error); > } > @@ -1212,18 +1235,23 @@ tcp_ident(void *oldp, size_t *oldlenp, v > } > } > > - if (inp != NULL && (inp->inp_socket->so_state & SS_CONNECTOUT)) { > - tir.ruid = inp->inp_socket->so_ruid; > - tir.euid = inp->inp_socket->so_euid; > + if (inp != NULL) > + so = in_pcbsolock_ref(inp); > + > + if (so != NULL && ISSET(so->so_state, SS_CONNECTOUT)) { > + tir.ruid = so->so_ruid; > + tir.euid = so->so_euid; > } else { > tir.ruid = -1; > tir.euid = -1; > } > > - *oldlenp = sizeof (tir); > - error = copyout((void *)&tir, oldp, sizeof (tir)); > + in_pcbsounlock_rele(inp, so); > + NET_UNLOCK_SHARED(); > in_pcbunref(inp); > - return (error); > + > + *oldlenp = sizeof(tir); > + return copyout(&tir, oldp, sizeof(tir)); > } > > int > @@ -1428,16 +1456,10 @@ tcp_sysctl(int *name, u_int namelen, voi > return (error); > > case TCPCTL_IDENT: > - NET_LOCK(); > - error = tcp_ident(oldp, oldlenp, newp, newlen, 0); > - NET_UNLOCK(); > - return (error); > + return tcp_ident(oldp, oldlenp, newp, newlen, 0); > > case TCPCTL_DROP: > - NET_LOCK(); > - error = tcp_ident(oldp, oldlenp, newp, newlen, 1); > - NET_UNLOCK(); > - return (error); > + return tcp_ident(oldp, oldlenp, newp, newlen, 1); > > case TCPCTL_REASS_LIMIT: > NET_LOCK(); > @@ -1506,9 +1528,8 @@ tcp_sysctl(int *name, u_int namelen, voi > return (error); > > default: > - error = sysctl_bounded_arr(tcpctl_vars, nitems(tcpctl_vars), > + return sysctl_bounded_arr(tcpctl_vars, nitems(tcpctl_vars), > name, namelen, oldp, oldlenp, newp, newlen); > - return (error); > } > /* NOTREACHED */ > } >