From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: tcp syn cache dead flag
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 29 Apr 2025 17:59:32 +0300

On Tue, Apr 29, 2025 at 12:17:47AM +0200, Alexander Bluhm wrote:
> Hi,
> 
> The TCP SYN cache timer uses SCF_DEAD flag to detect closed listen
> socket.  Note that syn_cache_rm() is setting sc_inplisten to NULL
> in the same atomic section where SCF_DEAD is set.  Also syn_cache_timer()
> uses the SYN cache mutex to check sc_inplisten and SCF_DEAD together.
> So we can eliminate SCF_DEAD and rely on existing pointer to listen
> socket.
> 
> ok?
> 

ok mvs@

> bluhm
> 
> Index: netinet/tcp_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
> diff -u -p -r1.440 tcp_input.c
> --- netinet/tcp_input.c	23 Apr 2025 17:52:12 -0000	1.440
> +++ netinet/tcp_input.c	28 Apr 2025 21:28:45 -0000
> @@ -3188,8 +3188,6 @@ syn_cache_rm(struct syn_cache *sc)
>  {
>  	MUTEX_ASSERT_LOCKED(&syn_cache_mtx);
>  
> -	KASSERT(!ISSET(sc->sc_dynflags, SCF_DEAD));
> -	SET(sc->sc_dynflags, SCF_DEAD);
>  	TAILQ_REMOVE(&sc->sc_buckethead->sch_bucket, sc, sc_bucketq);
>  	in_pcbunref(sc->sc_inplisten);
>  	sc->sc_inplisten = NULL;
> @@ -3394,7 +3392,8 @@ syn_cache_timer(void *arg)
>  	int lastref, do_ecn = 0;
>  
>  	mtx_enter(&syn_cache_mtx);
> -	if (ISSET(sc->sc_dynflags, SCF_DEAD))
> +	inp = in_pcbref(sc->sc_inplisten);
> +	if (inp == NULL)
>  		goto freeit;
>  
>  	if (__predict_false(sc->sc_rxtshift == TCP_MAXRXTSHIFT)) {
> @@ -3418,9 +3417,6 @@ syn_cache_timer(void *arg)
>  	    TCPTV_REXMTMAX);
>  	if (timeout_add_msec(&sc->sc_timer, sc->sc_rxtcur))
>  		refcnt_take(&sc->sc_refcnt);
> -	inp = in_pcbref(sc->sc_inplisten);
> -	if (inp == NULL)
> -		goto freeit;
>  	mtx_leave(&syn_cache_mtx);
>  
>  	NET_LOCK_SHARED();
> @@ -3443,6 +3439,7 @@ syn_cache_timer(void *arg)
>   dropit:
>  	tcpstat_inc(tcps_sc_timed_out);
>  	syn_cache_rm(sc);
> +	in_pcbunref(inp);
>  	/* Decrement reference of the timer and free object after remove. */
>  	lastref = refcnt_rele(&sc->sc_refcnt);
>  	KASSERT(lastref == 0);
> Index: netinet/tcp_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_var.h,v
> diff -u -p -r1.189 tcp_var.h
> --- netinet/tcp_var.h	28 Apr 2025 21:12:35 -0000	1.189
> +++ netinet/tcp_var.h	28 Apr 2025 21:28:45 -0000
> @@ -265,7 +265,6 @@ struct syn_cache {
>  	u_int sc_rxtshift;		/* [S] for computing backoff */
>  	u_int sc_dynflags;		/* [S] flags accessed with mutex */
>  #define SCF_UNREACH	0x0001U		/* we've had an unreach error */
> -#define SCF_DEAD	0x0002U		/* this entry to be released */
>  
>  	u_short sc_fixflags;		/* [I] set during initialization */
>  #define SCF_TIMESTAMP	0x0010U		/* peer will do timestamps */
>