From: Alexander Bluhm Subject: make tcp_mssdflt atomic To: tech@openbsd.org Date: Wed, 25 Dec 2024 16:34:26 +0100 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 */