From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: tcp debug mutex
To: Vitaliy Makkoveev <otto@bsdbox.dev>
Cc: tech@openbsd.org
Date: Wed, 10 Apr 2024 23:45:06 +0200

On Thu, Apr 11, 2024 at 12:26:01AM +0300, Vitaliy Makkoveev wrote:
> > On 10 Apr 2024, at 22:05, Alexander Bluhm <bluhm@openbsd.org> wrote:
> > 
> > Hi,
> > 
> > Protect the global variables in TCP debug code with global mutex.
> > Add a missing include and also fix the -Wunused-but-set-variable
> > warning.
> > 
> > ok?
> > 
> 
> Why do you need special tcp_trace_locked() function? Sorry if I
> missing something, but this is not obvious to me.

To avoid a mtx_leave() before each return.
But I can also do it that way.

ok?

bluhm

Index: netinet/tcp_debug.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_debug.c,v
diff -u -p -r1.31 tcp_debug.c
--- netinet/tcp_debug.c	11 Jan 2024 13:49:49 -0000	1.31
+++ netinet/tcp_debug.c	10 Apr 2024 21:38:24 -0000
@@ -79,6 +79,7 @@
 #include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/mbuf.h>
+#include <sys/mutex.h>
 #include <sys/socket.h>
 
 #include <net/route.h>
@@ -99,11 +100,22 @@
 #endif /* INET6 */
 
 #ifdef TCPDEBUG
+#include <sys/protosw.h>
+#endif
+
+/*
+ *  Locks used to protect struct members in this file:
+ *	D	TCP debug global mutex
+ */
+
+struct mutex tcp_debug_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
+
+#ifdef TCPDEBUG
 int	tcpconsdebug = 0;
 #endif
 
-struct	tcp_debug tcp_debug[TCP_NDEBUG];
-int	tcp_debx;
+struct	tcp_debug tcp_debug[TCP_NDEBUG];	/* [D] */
+int	tcp_debx;				/* [D] */
 
 /*
  * Tcp debug routines
@@ -113,14 +125,20 @@ tcp_trace(short act, short ostate, struc
     caddr_t headers, int req, int len)
 {
 #ifdef TCPDEBUG
+	struct tcphdr *th;
 	tcp_seq seq, ack;
 	int flags;
 #endif
 	int pf = PF_UNSPEC;
-	struct tcp_debug *td = &tcp_debug[tcp_debx++];
-	struct tcpiphdr *ti = (struct tcpiphdr *)headers;
-	struct tcpipv6hdr *ti6 = (struct tcpipv6hdr *)headers;
-	struct tcphdr *th;
+	struct tcp_debug *td;
+	struct tcpiphdr *ti;
+	struct tcpipv6hdr *ti6;
+
+	mtx_enter(&tcp_debug_mtx);
+
+	td = &tcp_debug[tcp_debx++];
+	ti = (struct tcpiphdr *)headers;
+	ti6 = (struct tcpipv6hdr *)headers;
 
 	if (tcp_debx == TCP_NDEBUG)
 		tcp_debx = 0;
@@ -153,13 +171,17 @@ tcp_trace(short act, short ostate, struc
 		switch (pf) {
 #ifdef INET6
 		case PF_INET6:
+#ifdef TCPDEBUG
 			th = &ti6->ti6_t;
+#endif
 			td->td_ti6 = *ti6;
 			td->td_ti6.ti6_plen = len;
 			break;
 #endif /* INET6 */
 		case PF_INET:
+#ifdef TCPDEBUG
 			th = &ti->ti_t;
+#endif
 			td->td_ti = *ti;
 			td->td_ti.ti_len = len;
 			break;
@@ -171,8 +193,10 @@ tcp_trace(short act, short ostate, struc
 
 	td->td_req = req;
 #ifdef TCPDEBUG
-	if (tcpconsdebug == 0)
+	if (tcpconsdebug == 0) {
+		mtx_leave(&tcp_debug_mtx);
 		return;
+	}
 	if (otp)
 		printf("%p %s:", otp, tcpstates[ostate]);
 	else
@@ -217,12 +241,16 @@ tcp_trace(short act, short ostate, struc
 		printf(" -> %s", tcpstates[tp->t_state]);
 	/* print out internal state of tp !?! */
 	printf("\n");
-	if (tp == NULL)
+	if (tp == NULL) {
+		mtx_leave(&tcp_debug_mtx);
 		return;
+	}
 	printf("\trcv_(nxt,wnd,up) (%x,%lx,%x) snd_(una,nxt,max) (%x,%x,%x)\n",
 	    tp->rcv_nxt, tp->rcv_wnd, tp->rcv_up, tp->snd_una, tp->snd_nxt,
 	    tp->snd_max);
 	printf("\tsnd_(wl1,wl2,wnd) (%x,%x,%lx)\n",
 	    tp->snd_wl1, tp->snd_wl2, tp->snd_wnd);
 #endif /* TCPDEBUG */
+
+	mtx_leave(&tcp_debug_mtx);
 }