From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: tcp timer assert ip version
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: OpenBSD Tech <tech@openbsd.org>
Date: Sat, 27 Jan 2024 22:27:20 +0300

> On 22 Jan 2024, at 00:31, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> 
> Hi,
> 
> I took me a while to figure out, why the inp->inp_faddr access in
> tcp_timer_rexmt() is actually IPv4 only.
> 
> Better add a comment and kassert.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: netinet/tcp_timer.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.c,v
> diff -u -p -r1.74 tcp_timer.c
> --- netinet/tcp_timer.c	11 Jan 2024 13:49:49 -0000	1.74
> +++ netinet/tcp_timer.c	21 Jan 2024 14:59:16 -0000
> @@ -198,30 +198,35 @@ void
> tcp_timer_rexmt(void *arg)
> {
> 	struct tcpcb *otp = NULL, *tp = arg;
> +	struct inpcb *inp;
> 	uint32_t rto;
> 	short ostate;
> 
> 	NET_LOCK();
> +	inp = tp->t_inpcb;
> +
> 	/* Ignore canceled timeouts or timeouts that have been rescheduled. */
> 	if (!ISSET((tp)->t_flags, TF_TMR_REXMT) ||
> 	    timeout_pending(&tp->t_timer[TCPT_REXMT]))
> 		goto out;
> 	CLR((tp)->t_flags, TF_TMR_REXMT);
> 
> -	if ((tp->t_flags & TF_PMTUD_PEND) && tp->t_inpcb &&
> +	if ((tp->t_flags & TF_PMTUD_PEND) && inp &&
> 	    SEQ_GEQ(tp->t_pmtud_th_seq, tp->snd_una) &&
> 	    SEQ_LT(tp->t_pmtud_th_seq, (int)(tp->snd_una + tp->t_maxseg))) {
> 		struct sockaddr_in sin;
> 		struct icmp icmp;
> 
> +		/* TF_PMTUD_PEND is set in tcp_ctlinput() which is IPv4 only */
> +		KASSERT(!ISSET(inp->inp_flags, INP_IPV6));
> 		tp->t_flags &= ~TF_PMTUD_PEND;
> 
> 		/* XXX create fake icmp message with relevant entries */
> 		icmp.icmp_nextmtu = tp->t_pmtud_nextmtu;
> 		icmp.icmp_ip.ip_len = tp->t_pmtud_ip_len;
> 		icmp.icmp_ip.ip_hl = tp->t_pmtud_ip_hl;
> -		icmp.icmp_ip.ip_dst = tp->t_inpcb->inp_faddr;
> -		icmp_mtudisc(&icmp, tp->t_inpcb->inp_rtableid);
> +		icmp.icmp_ip.ip_dst = inp->inp_faddr;
> +		icmp_mtudisc(&icmp, inp->inp_rtableid);
> 
> 		/*
> 		 * Notify all connections to the same peer about
> @@ -230,9 +235,9 @@ tcp_timer_rexmt(void *arg)
> 		bzero(&sin, sizeof(sin));
> 		sin.sin_len = sizeof(sin);
> 		sin.sin_family = AF_INET;
> -		sin.sin_addr = tp->t_inpcb->inp_faddr;
> -		in_pcbnotifyall(&tcbtable, sintosa(&sin),
> -		    tp->t_inpcb->inp_rtableid, EMSGSIZE, tcp_mtudisc);
> +		sin.sin_addr = inp->inp_faddr;
> +		in_pcbnotifyall(&tcbtable, sintosa(&sin), inp->inp_rtableid,
> +		    EMSGSIZE, tcp_mtudisc);
> 		goto out;
> 	}
> 
> @@ -244,7 +249,7 @@ tcp_timer_rexmt(void *arg)
> 		    tp->t_softerror : ETIMEDOUT);
> 		goto out;
> 	}
> -	if (tp->t_inpcb->inp_socket->so_options & SO_DEBUG) {
> +	if (inp->inp_socket->so_options & SO_DEBUG) {
> 		otp = tp;
> 		ostate = tp->t_state;
> 	}
> @@ -265,10 +270,9 @@ tcp_timer_rexmt(void *arg)
> 	 * lots more sophisticated searching to find the right
> 	 * value here...
> 	 */
> -	if (ip_mtudisc && tp->t_inpcb &&
> +	if (ip_mtudisc && inp &&
> 	    TCPS_HAVEESTABLISHED(tp->t_state) &&
> 	    tp->t_rxtshift > TCP_MAXRXTSHIFT / 6) {
> -		struct inpcb *inp = tp->t_inpcb;
> 		struct rtentry *rt = NULL;
> 
> 		/* No data to send means path mtu is not a problem */
> @@ -319,7 +323,7 @@ tcp_timer_rexmt(void *arg)
> 	 * retransmit times until then.
> 	 */
> 	if (tp->t_rxtshift > TCP_MAXRXTSHIFT / 4) {
> -		in_losing(tp->t_inpcb);
> +		in_losing(inp);
> 		tp->t_rttvar += (tp->t_srtt >> TCP_RTT_SHIFT);
> 		tp->t_srtt = 0;
> 	}
>