Download raw body.
unlock tcp drop sysctl
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 */
> }
>
unlock tcp drop sysctl