From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: remove TCP timeout reaper
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 7 Jun 2025 01:13:10 +0300

On Fri, Jun 06, 2025 at 09:20:26PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> I think the TCP timeout reaper is no longer necessary.  Idea was
> to prevent that timeouts are using TCP sockets that were already
> closed.
> 
> But now tcp_close() runs with socket lock and tcp_timer_enter()
> checks that intotcpcb(inp) is not NULL while holding the socket
> lock.  So timeout cannot run if TCP has been closed.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: netinet/in_pcb.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v
> diff -u -p -r1.169 in_pcb.h
> --- netinet/in_pcb.h	3 Jun 2025 16:51:26 -0000	1.169
> +++ netinet/in_pcb.h	6 Jun 2025 19:05:08 -0000
> @@ -138,7 +138,7 @@ struct inpcb {
>  	u_int16_t inp_fport;		/* [t] foreign port */
>  	u_int16_t inp_lport;		/* [t] local port */
>  	struct	  socket *inp_socket;	/* [I] back pointer to socket */
> -	caddr_t	  inp_ppcb;		/* pointer to per-protocol pcb */
> +	caddr_t	  inp_ppcb;		/* [s] pointer to per-protocol pcb */
>  	struct    route inp_route;	/* [s] cached route */
>  	struct    refcnt inp_refcnt;	/* refcount PCB, delay memory free */
>  	int	  inp_flags;		/* generic IP/datagram flags */
> Index: netinet/tcp_subr.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
> diff -u -p -r1.211 tcp_subr.c
> --- netinet/tcp_subr.c	3 Jun 2025 16:51:26 -0000	1.211
> +++ netinet/tcp_subr.c	6 Jun 2025 19:05:08 -0000
> @@ -440,8 +440,6 @@ tcp_newtcpcb(struct inpcb *inp, int wait
>  	tp->t_inpcb = inp;
>  	for (i = 0; i < TCPT_NTIMERS; i++)
>  		TCP_TIMER_INIT(tp, i);
> -	timeout_set_flags(&tp->t_timer_reaper, tcp_timer_reaper, tp,
> -	    KCLOCK_NONE, TIMEOUT_PROC | TIMEOUT_MPSAFE);
>  
>  	tp->sack_enable = atomic_load_int(&tcp_do_sack);
>  	tp->t_flags = atomic_load_int(&tcp_do_rfc1323) ?
> @@ -528,9 +526,8 @@ tcp_close(struct tcpcb *tp)
>  	}
>  
>  	m_free(tp->t_template);
> -	/* Free tcpcb after all pending timers have been run. */
> -	timeout_add(&tp->t_timer_reaper, 0);
>  	inp->inp_ppcb = NULL;
> +	pool_put(&tcpcb_pool, tp);
>  	soisdisconnected(so);
>  	in_pcbdetach(inp);
>  	tcpstat_inc(tcps_closed);
> Index: netinet/tcp_timer.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.c,v
> diff -u -p -r1.84 tcp_timer.c
> --- netinet/tcp_timer.c	3 Jun 2025 16:51:26 -0000	1.84
> +++ netinet/tcp_timer.c	6 Jun 2025 19:05:08 -0000
> @@ -541,19 +541,3 @@ tcp_timer_2msl(void *arg)
>   out:
>  	tcp_timer_leave(inp, so);
>  }
> -
> -void
> -tcp_timer_reaper(void *arg)
> -{
> -	struct tcpcb *tp = arg;
> -
> -	/*
> -	 * This timer is necessary to delay the pool_put() after all timers
> -	 * have finished, even if they were sleeping to grab the net lock.
> -	 * Putting the pool_put() in a timer is sufficient as all timers run
> -	 * from the same timeout thread.  Note that neither softnet thread nor
> -	 * user process may access the tcpcb after arming the reaper timer.
> -	 * Freeing may run in parallel as it does not grab the net lock.
> -	 */
> -	pool_put(&tcpcb_pool, tp);
> -}
> Index: netinet/tcp_timer.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.h,v
> diff -u -p -r1.26 tcp_timer.h
> --- netinet/tcp_timer.h	16 Jan 2025 11:59:20 -0000	1.26
> +++ netinet/tcp_timer.h	6 Jun 2025 19:05:08 -0000
> @@ -164,8 +164,5 @@ extern int tcp_keepidle_sec;	/* [a] copy
>  extern int tcp_keepintvl_sec;	/* [a] copy of above in seconds for sysctl */
>  extern int tcp_ttl;		/* time to live for TCP segs */
>  extern const int tcp_backoff[];
> -
> -void	tcp_timer_reaper(void *);
> -
>  #endif /* _KERNEL */
>  #endif /* _NETINET_TCP_TIMER_H_ */
> Index: netinet/tcp_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_var.h,v
> diff -u -p -r1.191 tcp_var.h
> --- netinet/tcp_var.h	7 May 2025 14:10:19 -0000	1.191
> +++ netinet/tcp_var.h	6 Jun 2025 19:05:08 -0000
> @@ -70,7 +70,6 @@ struct tcpqent {
>  struct tcpcb {
>  	struct tcpqehead t_segq;		/* sequencing queue */
>  	struct timeout t_timer[TCPT_NTIMERS];	/* tcp timers */
> -	struct timeout t_timer_reaper;	/* reaper is special, no refcnt */
>  	short	t_state;		/* state of this connection */
>  	short	t_rxtshift;		/* log(2) of rexmt exp. backoff */
>  	int	t_rxtcur;		/* current retransmit value */
>