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

On Wed, Apr 10, 2024 at 11:45:06PM +0200, Alexander Bluhm wrote:
> 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.
> 

I'm not against this version, but why not simple "goto unlock" or
"goto out"?

Anyway, feel free to commit this version if you like it more.

> 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);
>  }