Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
keep socket lock in sonewconn
To:
tech@openbsd.org
Date:
Fri, 24 Jan 2025 10:02:09 +0100

Download raw body.

Thread
Hi,

For TCP input unlocking we need a consistent lock of a newly created
socket.  Instead of releasing the lock in sonewconn() and grabbing
it again later, I think it is better that sonewconn() returns a
locked socket.

Currently I only changed syn_cache_get() which calls in_pcbsounlock_rele()
at the end.  Following diffs will push the unlock into tcp_input().

ok?

bluhm

Index: kern/uipc_socket2.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket2.c,v
diff -u -p -r1.168 uipc_socket2.c
--- kern/uipc_socket2.c	22 Jan 2025 15:05:49 -0000	1.168
+++ kern/uipc_socket2.c	23 Jan 2025 23:07:02 -0000
@@ -237,8 +237,6 @@ sonewconn(struct socket *head, int conns
 		wakeup(&head->so_timeo);
 	}
 
-	sounlock_nonet(so);
-
 	return (so);
 
 fail:
Index: kern/uipc_usrreq.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_usrreq.c,v
diff -u -p -r1.213 uipc_usrreq.c
--- kern/uipc_usrreq.c	20 Jan 2025 16:34:48 -0000	1.213
+++ kern/uipc_usrreq.c	23 Jan 2025 22:51:36 -0000
@@ -890,18 +890,17 @@ unp_connect(struct socket *so, struct mb
 
 		if ((so2->so_options & SO_ACCEPTCONN) == 0 ||
 		    (so3 = sonewconn(so2, 0, M_WAIT)) == NULL) {
+			sounlock(so2);
 			error = ECONNREFUSED;
-		}
-
-		sounlock(so2);
-
-		if (error != 0)
 			goto put;
+		}
 
 		/*
 		 * Since `so2' is protected by vnode(9) lock, `so3'
 		 * can't be PRU_ABORT'ed here.
 		 */
+		sounlock(so2);
+		sounlock(so3);
 		solock_pair(so, so3);
 
 		unp2 = sotounpcb(so2);
Index: netinet/tcp_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
diff -u -p -r1.423 tcp_input.c
--- netinet/tcp_input.c	16 Jan 2025 11:59:20 -0000	1.423
+++ netinet/tcp_input.c	23 Jan 2025 23:06:45 -0000
@@ -3535,13 +3535,15 @@ syn_cache_get(struct sockaddr *src, stru
 	struct inpcb *inp, *oldinp;
 	struct tcpcb *tp = NULL;
 	struct mbuf *am;
-	struct socket *oso;
+	struct socket *oldso;
 	u_int rtableid;
 
 	NET_ASSERT_LOCKED();
 
+	inp = sotoinpcb(so);
+
 	mtx_enter(&syn_cache_mtx);
-	sc = syn_cache_lookup(src, dst, &scp, sotoinpcb(so)->inp_rtableid);
+	sc = syn_cache_lookup(src, dst, &scp, inp->inp_rtableid);
 	if (sc == NULL) {
 		mtx_leave(&syn_cache_mtx);
 		return (NULL);
@@ -3571,12 +3573,13 @@ syn_cache_get(struct sockaddr *src, stru
 	 * connection when the SYN arrived.  If we can't create
 	 * the connection, abort it.
 	 */
-	oso = so;
+	oldso = so;
+	oldinp = inp;
 	so = sonewconn(so, SS_ISCONNECTED, M_DONTWAIT);
 	if (so == NULL)
 		goto resetandabort;
-
-	oldinp = sotoinpcb(oso);
+	soassertlocked(so);
+	soref(so);
 	inp = sotoinpcb(so);
 
 #ifdef IPSEC
@@ -3637,7 +3640,7 @@ syn_cache_get(struct sockaddr *src, stru
 	(void) m_free(am);
 
 	tp = intotcpcb(inp);
-	tp->t_flags = sototcpcb(oso)->t_flags & (TF_NOPUSH|TF_NODELAY);
+	tp->t_flags = sototcpcb(oldso)->t_flags & (TF_NOPUSH|TF_NODELAY);
 	if (sc->sc_request_r_scale != 15) {
 		tp->requested_s_scale = sc->sc_requested_s_scale;
 		tp->request_r_scale = sc->sc_request_r_scale;
@@ -3649,7 +3652,6 @@ syn_cache_get(struct sockaddr *src, stru
 	tp->t_template = tcp_template(tp);
 	if (tp->t_template == 0) {
 		tp = tcp_drop(tp, ENOBUFS);	/* destroys socket */
-		so = NULL;
 		goto abort;
 	}
 	tp->sack_enable = ISSET(sc->sc_fixflags, SCF_SACK_PERMIT);
@@ -3700,6 +3702,7 @@ syn_cache_get(struct sockaddr *src, stru
 		tp->rcv_adv = tp->rcv_nxt + sc->sc_win;
 	tp->last_ack_sent = tp->rcv_nxt;
 
+	in_pcbsounlock_rele(inp, so);
 	tcpstat_inc(tcps_sc_completed);
 	syn_cache_put(sc);
 	return (so);
@@ -3707,10 +3710,11 @@ syn_cache_get(struct sockaddr *src, stru
 resetandabort:
 	tcp_respond(NULL, mtod(m, caddr_t), th, (tcp_seq)0, th->th_ack, TH_RST,
 	    m->m_pkthdr.ph_rtableid, now);
-abort:
-	m_freem(m);
 	if (so != NULL)
 		soabort(so);
+abort:
+	m_freem(m);
+	in_pcbsounlock_rele(inp, so);
 	syn_cache_put(sc);
 	tcpstat_inc(tcps_sc_aborted);
 	return ((struct socket *)(-1));