Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: TCP window shifts unsinged long
To:
Theo de Raadt <deraadt@openbsd.org>
Cc:
Mark Kettenis <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date:
Mon, 11 Aug 2025 16:43:27 +0200

Download raw body.

Thread
The source of the u_long types are in the socket buffer.

struct sockbuf {
        u_long  sb_cc;                  /* [m] actual chars in buffer */
        u_long  sb_datacc;              /* [m] data only chars in buffer */
        u_long  sb_hiwat;               /* [m] max actual char count */
        u_long  sb_wat;                 /* [m] default watermark */
        u_long  sb_mbcnt;               /* [m] chars of mbufs used */
        u_long  sb_mbmax;               /* [m] max chars of mbufs to use */
}

The were converted from u_short to u_long here:

commit 4924467d92d8f1e83e3d218800f0804c18ec2f95
Author: Mike Karels <karels>
Date:   Wed Jun 17 22:08:30 1987 -0700

    use longs for sockbuf counters, it fits!

    SCCS-vsn: 7.2

Are you really suggesting to convert them to u_int?  This would
need a review of all comparisons in all domains and protocols.  I
doubt that this is feasible.

All I want is getting some comparisons in TCP less complicated.
The easiest way to achieve this, is to convert the size calculations
to u_long.  Then buffer length, window size, and sizeof have the
same integer rank.  I am pretty sure that 32 bit are sufficent.
But in my opinion, long is the only type to which we can convert
the existing code.  Unless you want everthing in unsinged long long,
but that is overkill, would hurt 32 bit platforms, and access is
not MP atomic.

The longer I look into these conversions, the more complicated it
gets.  Below is a diff where I convert all window sizes to u_long.
Also I removed some more needless casts.  At least that makes it
consistent.  I you don't like it, I will drop the diff.

bluhm

Index: netinet/tcp.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp.h,v
diff -u -p -r1.24 tcp.h
--- netinet/tcp.h	19 May 2023 01:04:39 -0000	1.24
+++ netinet/tcp.h	11 Aug 2025 13:19:44 -0000
@@ -113,7 +113,7 @@ struct tcphdr {
  */
 #define	TCP_MSS		512
 
-#define	TCP_MAXWIN	65535	/* largest value for (unscaled) window */
+#define	TCP_MAXWIN	65535UL	/* largest value for (unscaled) window */
 
 #define	TCP_MAX_WINSHIFT	14	/* maximum window shift */
 
Index: netinet/tcp_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
diff -u -p -r1.458 tcp_input.c
--- netinet/tcp_input.c	5 Aug 2025 09:51:12 -0000	1.458
+++ netinet/tcp_input.c	11 Aug 2025 14:33:26 -0000
@@ -690,7 +690,7 @@ findpcb:
 
 	/* Unscale the window into a 32-bit value. */
 	if ((tiflags & TH_SYN) == 0)
-		tiwin = th->th_win << tp->snd_scale;
+		tiwin = (u_long)th->th_win << tp->snd_scale;
 	else
 		tiwin = th->th_win;
 
@@ -990,7 +990,7 @@ findpcb:
 #endif
 	    (!opti.ts_present || TSTMP_GEQ(opti.ts_val, tp->ts_recent)) &&
 	    th->th_seq == tp->rcv_nxt &&
-	    tiwin && tiwin == tp->snd_wnd &&
+	    tiwin > 0 && tiwin == tp->snd_wnd &&
 	    tp->snd_nxt == tp->snd_max) {
 
 		/*
@@ -1156,12 +1156,10 @@ findpcb:
 	 * but not less than advertised window.
 	 */
 	{
-		int win;
+		u_long win;
 
-		win = sbspace(&so->so_rcv);
-		if (win < 0)
-			win = 0;
-		tp->rcv_wnd = imax(win, (int)(tp->rcv_adv - tp->rcv_nxt));
+		win = lmax(sbspace(&so->so_rcv), 0);
+		tp->rcv_wnd = lmax(win, (int)(tp->rcv_adv - tp->rcv_nxt));
 	}
 
 	switch (tp->t_state) {
@@ -1551,7 +1549,7 @@ trimthenstep6:
 			(TF_RCVD_SCALE|TF_REQ_SCALE)) {
 			tp->snd_scale = tp->requested_s_scale;
 			tp->rcv_scale = tp->request_r_scale;
-			tiwin = th->th_win << tp->snd_scale;
+			tiwin = (u_long)th->th_win << tp->snd_scale;
 		}
 		tcp_flush_queue(tp);
 		tp->snd_wl1 = th->th_seq - 1;
@@ -1582,11 +1580,13 @@ trimthenstep6:
 		if (do_ecn && (tiflags & TH_ECE)) {
 			if ((tp->t_flags & TF_ECN_PERMIT) &&
 			    SEQ_GEQ(tp->snd_una, tp->snd_last)) {
-				u_int win;
+				u_long win;
 
-				win = min(tp->snd_wnd, tp->snd_cwnd) / tp->t_maxseg;
+				win = ulmin(tp->snd_wnd, tp->snd_cwnd) /
+				    tp->t_maxseg;
 				if (win > 1) {
-					tp->snd_ssthresh = win / 2 * tp->t_maxseg;
+					tp->snd_ssthresh = win / 2 *
+					    tp->t_maxseg;
 					tp->snd_cwnd = tp->snd_ssthresh;
 					tp->snd_last = tp->snd_max;
 					tp->t_flags |= TF_SEND_CWR;
@@ -3881,7 +3881,7 @@ syn_cache_add(struct sockaddr *src, stru
     struct tcp_opt_info *oi, tcp_seq *issp, uint64_t now, int do_ecn)
 {
 	struct tcpcb tb, *tp;
-	long win;
+	u_long win;
 	struct syn_cache *sc;
 	struct syn_cache_head *scp;
 	struct mbuf *ipopts;
@@ -3900,7 +3900,7 @@ syn_cache_add(struct sockaddr *src, stru
 	/*
 	 * Initialize some local state.
 	 */
-	win = sbspace(&so->so_rcv);
+	win = lmax(sbspace(&so->so_rcv), 0);
 	if (win > TCP_MAXWIN)
 		win = TCP_MAXWIN;
 
Index: netinet/tcp_output.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_output.c,v
diff -u -p -r1.156 tcp_output.c
--- netinet/tcp_output.c	8 Jul 2025 00:47:41 -0000	1.156
+++ netinet/tcp_output.c	11 Aug 2025 14:21:53 -0000
@@ -190,7 +190,8 @@ int
 tcp_output(struct tcpcb *tp)
 {
 	struct socket *so = tp->t_inpcb->inp_socket;
-	long len, win, rcv_hiwat, txmaxseg;
+	u_long win;
+	long len, rcv_hiwat, txmaxseg;
 	int off, flags, error;
 	struct mbuf *m;
 	struct tcphdr *th;
@@ -374,7 +375,7 @@ again:
 		flags &= ~TH_FIN;
 
 	mtx_enter(&so->so_rcv.sb_mtx);
-	win = sbspace_locked(&so->so_rcv);
+	win = lmax(sbspace_locked(&so->so_rcv), 0);
 	rcv_hiwat = (long) so->so_rcv.sb_hiwat;
 	mtx_leave(&so->so_rcv.sb_mtx);
 
@@ -418,7 +419,7 @@ again:
 		 * taking into account that we are limited by
 		 * TCP_MAXWIN << tp->rcv_scale.
 		 */
-		long adv = lmin(win, (long)TCP_MAXWIN << tp->rcv_scale) -
+		long adv = ulmin(win, TCP_MAXWIN << tp->rcv_scale) -
 			(tp->rcv_adv - tp->rcv_nxt);
 
 		if (adv >= (long) (2 * tp->t_maxseg))
@@ -857,15 +858,15 @@ send:
 	 * Calculate receive window.  Don't shrink window,
 	 * but avoid silly window syndrome.
 	 */
-	if (win < (rcv_hiwat / 4) && win < (long)tp->t_maxseg)
+	if (win < (rcv_hiwat / 4) && win < tp->t_maxseg)
 		win = 0;
-	if (win > (long)TCP_MAXWIN << tp->rcv_scale)
-		win = (long)TCP_MAXWIN << tp->rcv_scale;
-	if (win < (long)(int32_t)(tp->rcv_adv - tp->rcv_nxt))
-		win = (long)(int32_t)(tp->rcv_adv - tp->rcv_nxt);
+	if (win > TCP_MAXWIN << tp->rcv_scale)
+		win = TCP_MAXWIN << tp->rcv_scale;
+	if (win < (int32_t)(tp->rcv_adv - tp->rcv_nxt))
+		win = (int32_t)(tp->rcv_adv - tp->rcv_nxt);
 	if (flags & TH_RST)
 		win = 0;
-	th->th_win = htons((u_int16_t) (win>>tp->rcv_scale));
+	th->th_win = htons(win >> tp->rcv_scale);
 	if (th->th_win == 0)
 		tp->t_sndzerowin++;
 	if (SEQ_GT(tp->snd_up, tp->snd_nxt)) {
@@ -1164,7 +1165,7 @@ out:
 	 * then remember the size of the advertised window.
 	 * Any pending ACK has now been sent.
 	 */
-	if (win > 0 && SEQ_GT(tp->rcv_nxt+win, tp->rcv_adv))
+	if (win > 0 && SEQ_GT(tp->rcv_nxt + win, tp->rcv_adv))
 		tp->rcv_adv = tp->rcv_nxt + win;
 	tp->last_ack_sent = tp->rcv_nxt;
 	tp->t_sndacktime = now;
Index: netinet/tcp_subr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
diff -u -p -r1.216 tcp_subr.c
--- netinet/tcp_subr.c	18 Jul 2025 08:39:14 -0000	1.216
+++ netinet/tcp_subr.c	11 Aug 2025 14:35:11 -0000
@@ -294,7 +294,7 @@ tcp_respond(struct tcpcb *tp, caddr_t te
     tcp_seq ack, tcp_seq seq, int flags, u_int rtableid, uint64_t now)
 {
 	int tlen;
-	int win = 0;
+	u_long win = 0;
 	struct mbuf *m = NULL;
 	struct tcphdr *th;
 	struct ip *ip;
@@ -305,7 +305,7 @@ tcp_respond(struct tcpcb *tp, caddr_t te
 
 	if (tp) {
 		struct socket *so = tp->t_inpcb->inp_socket;
-		win = sbspace(&so->so_rcv);
+		win = lmax(sbspace(&so->so_rcv), 0);
 		/*
 		 * If this is called with an unconnected
 		 * socket/tp/pcb (tp->pf is 0), we lose.
@@ -364,7 +364,7 @@ tcp_respond(struct tcpcb *tp, caddr_t te
 		win >>= tp->rcv_scale;
 	if (win > TCP_MAXWIN)
 		win = TCP_MAXWIN;
-	th->th_win = htons((u_int16_t)win);
+	th->th_win = htons(win);
 	th->th_urp = 0;
 
 	if (tp && (tp->t_flags & (TF_REQ_TSTMP|TF_NOOPT)) == TF_REQ_TSTMP &&
Index: netinet/tcp_var.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_var.h,v
diff -u -p -r1.195 tcp_var.h
--- netinet/tcp_var.h	18 Jun 2025 16:15:46 -0000	1.195
+++ netinet/tcp_var.h	11 Aug 2025 14:08:40 -0000
@@ -248,7 +248,7 @@ struct syn_cache {
 	struct refcnt sc_refcnt;		/* ref count list and timer */
 	struct timeout sc_timer;		/* rexmt timer */
 	struct route sc_route;			/* [s] cached route */
-	long sc_win;				/* [I] advertised window */
+	u_long sc_win;				/* [I] advertised window */
 	struct syn_cache_head *sc_buckethead;	/* [S] our bucket index */
 	struct syn_cache_set *sc_set;		/* [S] our syn cache set */
 	u_int64_t sc_timestamp;		/* [s] timestamp from SYN */