Download raw body.
make tcp_mssdflt atomic
> On 25 Dec 2024, at 18:34, Alexander Bluhm <bluhm@openbsd.org> 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 */
>
make tcp_mssdflt atomic