From: Vitaliy Makkoveev Subject: Re: make tcp_mssdflt atomic To: Alexander Bluhm Cc: tech@openbsd.org Date: Thu, 26 Dec 2024 03:11:19 +0300 > On 25 Dec 2024, at 18:34, Alexander Bluhm wrote: > > 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? > ok mvs > 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 */ >