From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: tcp debug mutex
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 11 Apr 2024 00:26:01 +0300

> 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.

> 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 18:53:38 -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,20 +100,32 @@
> #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
>  */
> void
> -tcp_trace(short act, short ostate, struct tcpcb *tp, struct tcpcb *otp,
> +tcp_trace_locked(short act, short ostate, struct tcpcb *tp, struct tcpcb *otp,
>     caddr_t headers, int req, int len)
> {
> #ifdef TCPDEBUG
> +	struct tcphdr *th;
> 	tcp_seq seq, ack;
> 	int flags;
> #endif
> @@ -120,7 +133,6 @@ tcp_trace(short act, short ostate, struc
> 	struct tcp_debug *td = &tcp_debug[tcp_debx++];
> 	struct tcpiphdr *ti = (struct tcpiphdr *)headers;
> 	struct tcpipv6hdr *ti6 = (struct tcpipv6hdr *)headers;
> -	struct tcphdr *th;
> 
> 	if (tcp_debx == TCP_NDEBUG)
> 		tcp_debx = 0;
> @@ -153,13 +165,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;
> @@ -225,4 +241,13 @@ tcp_trace(short act, short ostate, struc
> 	printf("\tsnd_(wl1,wl2,wnd) (%x,%x,%lx)\n",
> 	    tp->snd_wl1, tp->snd_wl2, tp->snd_wnd);
> #endif /* TCPDEBUG */
> +}
> +
> +void
> +tcp_trace(short act, short ostate, struct tcpcb *tp, struct tcpcb *otp,
> +    caddr_t headers, int req, int len)
> +{
> +	mtx_enter(&tcp_debug_mtx);
> +	tcp_trace_locked(act, ostate, tp, otp, headers, req, len);
> +	mtx_leave(&tcp_debug_mtx);
> }
>