Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
make tcp_mssdflt atomic
To:
tech@openbsd.org
Date:
Wed, 25 Dec 2024 16:34:26 +0100

Download raw body.

Thread
  • Alexander Bluhm:

    make tcp_mssdflt atomic

Hi,

To further unlock TCP sysctl, we need atomic access to tcp_mssdflt.
pf(4) is reading the value multiple times.  Better read it once and
pass mssdflt down the call stack.

In pf_calc_mss() was a potential integer underflow.  Use the signed
variant imax(9) and imin(9) like I have fixed it in TCP stack.

mvs: I know that you dislike adding the [a] to the header file.
Note that tcp_mssdflt has no comment in its tcp_subr.c definition.
I consider the locking strategy part of the API so it makes sense
to add [a] in tcp_var.h.  The current code is inconsistent anyway,
but for TCP I think the block with comments in header file is the
better place.

ok?

bluhm

Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
diff -u -p -r1.1206 pf.c
--- net/pf.c	8 Nov 2024 13:22:09 -0000	1.1206
+++ net/pf.c	25 Dec 2024 15:13:02 -0000
@@ -212,7 +212,7 @@ int			 pf_icmp_state_lookup(struct pf_pd
 int			 pf_test_state_icmp(struct pf_pdesc *,
 			    struct pf_state **, u_short *);
 u_int16_t		 pf_calc_mss(struct pf_addr *, sa_family_t, int,
-			    u_int16_t);
+			    uint16_t, uint16_t);
 static __inline int	 pf_set_rt_ifp(struct pf_state *, struct pf_addr *,
 			    sa_family_t, struct pf_src_node **);
 struct pf_divert	*pf_get_divert(struct mbuf *);
@@ -3926,17 +3926,18 @@ pf_get_wscale(struct pf_pdesc *pd)
 }
 
 u_int16_t
-pf_get_mss(struct pf_pdesc *pd)
+pf_get_mss(struct pf_pdesc *pd, uint16_t mssdflt)
 {
 	int		 olen;
 	u_int8_t	 opts[MAX_TCPOPTLEN], *opt;
-	u_int16_t	 mss = tcp_mssdflt;
+	u_int16_t	 mss;
 
 	olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr);
 	if (olen < TCPOLEN_MAXSEG || !pf_pull_hdr(pd->m,
 	    pd->off + sizeof(struct tcphdr), opts, olen, NULL, pd->af))
 		return (0);
 
+	mss = mssdflt;
 	opt = opts;
 	while ((opt = pf_find_tcpopt(opt, opts, olen,
 		    TCPOPT_MAXSEG, TCPOLEN_MAXSEG)) != NULL) {
@@ -3949,7 +3950,8 @@ pf_get_mss(struct pf_pdesc *pd)
 }
 
 u_int16_t
-pf_calc_mss(struct pf_addr *addr, sa_family_t af, int rtableid, u_int16_t offer)
+pf_calc_mss(struct pf_addr *addr, sa_family_t af, int rtableid, uint16_t offer,
+    uint16_t mssdflt)
 {
 	struct ifnet		*ifp;
 	struct sockaddr_in	*dst;
@@ -3958,8 +3960,7 @@ pf_calc_mss(struct pf_addr *addr, sa_fam
 #endif /* INET6 */
 	struct rtentry		*rt = NULL;
 	struct sockaddr_storage	 ss;
-	int			 hlen;
-	u_int16_t		 mss = tcp_mssdflt;
+	int			 hlen, mss;
 
 	memset(&ss, 0, sizeof(ss));
 
@@ -3984,14 +3985,15 @@ pf_calc_mss(struct pf_addr *addr, sa_fam
 #endif /* INET6 */
 	}
 
+	mss = mssdflt;
 	if (rt != NULL && (ifp = if_get(rt->rt_ifidx)) != NULL) {
 		mss = ifp->if_mtu - hlen - sizeof(struct tcphdr);
-		mss = max(tcp_mssdflt, mss);
+		mss = imax(mss, mssdflt);
 		if_put(ifp);
 	}
 	rtfree(rt);
-	mss = min(mss, offer);
-	mss = max(mss, 64);		/* sanity - at least max opt space */
+	mss = imin(mss, offer);
+	mss = imax(mss, 64);		/* sanity - at least max opt space */
 	return (mss);
 }
 
@@ -4597,7 +4599,6 @@ pf_create_state(struct pf_pdesc *pd, str
 {
 	struct pf_state		*st = NULL;
 	struct tcphdr		*th = &pd->hdr.tcp;
-	u_int16_t		 mss = tcp_mssdflt;
 	u_short			 reason;
 	u_int			 i;
 
@@ -4764,15 +4765,17 @@ pf_create_state(struct pf_pdesc *pd, str
 	}
 	if (pd->proto == IPPROTO_TCP && (th->th_flags & (TH_SYN|TH_ACK)) ==
 	    TH_SYN && r->keep_state == PF_STATE_SYNPROXY && pd->dir == PF_IN) {
-		int rtid = pd->rdomain;
-		if (act->rtableid >= 0)
-			rtid = act->rtableid;
+		int		rtid;
+		uint16_t	mss, mssdflt;
+
+		rtid = (act->rtableid >= 0) ? act->rtableid : pd->rdomain;
 		pf_set_protostate(st, PF_PEER_SRC, PF_TCPS_PROXY_SRC);
 		st->src.seqhi = arc4random();
 		/* Find mss option */
-		mss = pf_get_mss(pd);
-		mss = pf_calc_mss(pd->src, pd->af, rtid, mss);
-		mss = pf_calc_mss(pd->dst, pd->af, rtid, mss);
+		mssdflt = atomic_load_int(&tcp_mssdflt);
+		mss = pf_get_mss(pd, mssdflt);
+		mss = pf_calc_mss(pd->src, pd->af, rtid, mss, mssdflt);
+		mss = pf_calc_mss(pd->dst, pd->af, rtid, mss, mssdflt);
 		st->src.mss = mss;
 		pf_send_tcp(r, pd->af, pd->dst, pd->src, th->th_dport,
 		    th->th_sport, st->src.seqhi, ntohl(th->th_seq) + 1,
Index: net/pf_syncookies.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_syncookies.c,v
diff -u -p -r1.7 pf_syncookies.c
--- net/pf_syncookies.c	10 Sep 2018 15:54:28 -0000	1.7
+++ net/pf_syncookies.c	25 Dec 2024 15:13:02 -0000
@@ -199,10 +199,11 @@ pf_synflood_check(struct pf_pdesc *pd)
 void
 pf_syncookie_send(struct pf_pdesc *pd)
 {
-	uint16_t	mss;
+	uint16_t	mss, mssdflt;
 	uint32_t	iss;
 
-	mss = max(tcp_mssdflt, pf_get_mss(pd));
+	mssdflt = atomic_load_int(&tcp_mssdflt);
+	mss = max(pf_get_mss(pd, mssdflt), mssdflt);
 	iss = pf_syncookie_generate(pd, mss);
 	pf_send_tcp(NULL, pd->af, pd->dst, pd->src, *pd->dport, *pd->sport,
 	    iss, ntohl(pd->hdr.tcp.th_seq) + 1, TH_SYN|TH_ACK, 0, mss,
Index: net/pfvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
diff -u -p -r1.541 pfvar.h
--- net/pfvar.h	12 Nov 2024 04:14:51 -0000	1.541
+++ net/pfvar.h	25 Dec 2024 15:13:02 -0000
@@ -1858,7 +1858,7 @@ void			 pf_mbuf_unlink_inpcb(struct mbuf
 u_int8_t*		 pf_find_tcpopt(u_int8_t *, u_int8_t *, size_t,
 			    u_int8_t, u_int8_t);
 u_int8_t		 pf_get_wscale(struct pf_pdesc *);
-u_int16_t		 pf_get_mss(struct pf_pdesc *);
+u_int16_t		 pf_get_mss(struct pf_pdesc *, uint16_t);
 struct mbuf *		 pf_build_tcp(const struct pf_rule *, sa_family_t,
 			    const struct pf_addr *, const struct pf_addr *,
 			    u_int16_t, u_int16_t, u_int32_t, u_int32_t,
Index: netinet/tcp_subr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
diff -u -p -r1.201 tcp_subr.c
--- netinet/tcp_subr.c	17 Apr 2024 20:48:51 -0000	1.201
+++ netinet/tcp_subr.c	25 Dec 2024 15:13:02 -0000
@@ -437,7 +437,7 @@ tcp_newtcpcb(struct inpcb *inp, int wait
 	if (tp == NULL)
 		return (NULL);
 	TAILQ_INIT(&tp->t_segq);
-	tp->t_maxseg = tcp_mssdflt;
+	tp->t_maxseg = atomic_load_int(&tcp_mssdflt);
 	tp->t_maxopd = 0;
 
 	for (i = 0; i < TCPT_NTIMERS; i++)
Index: netinet/tcp_var.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_var.h,v
diff -u -p -r1.178 tcp_var.h
--- netinet/tcp_var.h	13 May 2024 01:15:53 -0000	1.178
+++ netinet/tcp_var.h	25 Dec 2024 15:13:18 -0000
@@ -679,7 +679,7 @@ extern	struct pool tcpcb_pool;
 extern	struct inpcbtable tcbtable, tcb6table;	/* queue of active tcpcb's */
 extern	int tcp_do_rfc1323;	/* enabled/disabled? */
 extern	int tcptv_keep_init;	/* [N] time to keep alive initial SYN packet */
-extern	int tcp_mssdflt;	/* default maximum segment size */
+extern	int tcp_mssdflt;	/* [a] default maximum segment size */
 extern	int tcp_rst_ppslim;	/* maximum outgoing RST packet per second */
 extern	int tcp_ack_on_push;	/* ACK immediately on PUSH */
 extern	int tcp_do_sack;	/* SACK enabled/disabled */