From: Claudio Jeker Subject: Re: IP6_EXTHDR_GET pass mbuf pointer To: Alexander Bluhm Cc: tech@openbsd.org Date: Mon, 26 May 2025 15:41:11 +0200 On Sun, May 25, 2025 at 11:59:23AM +0900, Alexander Bluhm wrote: > On Thu, May 22, 2025 at 05:23:48PM +0200, Claudio Jeker wrote: > > On Fri, May 23, 2025 at 12:07:46AM +0900, Alexander Bluhm wrote: > > > On Thu, May 22, 2025 at 04:25:57PM +0200, Claudio Jeker wrote: > > > > Would it make sense to actually kill this macro and make it a proper > > > > function? This thing is a monster and it probably makes things slower if > > > > anything. > > > > > > I was considering this. Problem is that typ is passed. Then the > > > return pointer is casted accordingly. Maybe we get can get away > > > with void pointer and the compiler doing its job. > > > > (val) = (typ)(mtod((*(mp)), caddr_t) + (off)); \ > > (val) = (typ)NULL; \ > > > > Urgh, these casts are just horrible. I think using a void * as return > > works equally well. > > > > > static inline void *ip6_exthdr_get(struct mbuf **mp, int off, int len); > > > th = ip6_exthdr_get(mp, iphlen, sizeof(*th)); > > > > Please use a size_t len argument. Also I'm not sure if you gain anything > > from static inline. Would be an interesting experiment. > > Here is the ip6_exthdr_get() function diff. > > I make it static inline in netinet/ip6.h, mainly because we have > no proper C file for it. There is no ip6.c. And it has to be in > netinet as it is used by TCP and UDP in kernel without IPv6. Then > the namespace ip6_ would be inconsistent. Keeping it in in6.h > avoids all these naming problems. And the inline function is not > very long. > > I moved the panic("m_pulldown malfunction") as kassert into > m_pulldown(). Then it is only implemented once. The code is > complicated enough that checking makes some sense. > > Regarding the type of len, I don't know. A lot of sizes in the > network stack are passed around as signed int. At least that avoids > type confusion when comparing them against each other. len is > passed to m_pulldown() which also takes an int. The callers of > ip6_exthdr_get() pass int or size_t to off or len inconsistently. > I think it is best to keep everthing int as it is. > > I have also wrapped an ugly a long line in the diff's context. > > ok? Looks good to me. OK claudio@ > bluhm > > Index: kern/uipc_mbuf2.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf2.c,v > diff -u -p -r1.47 uipc_mbuf2.c > --- kern/uipc_mbuf2.c 29 Aug 2024 16:42:30 -0000 1.47 > +++ kern/uipc_mbuf2.c 24 May 2025 14:02:05 -0000 > @@ -213,6 +213,7 @@ m_pulldown(struct mbuf *m, int off, int > off = 0; > > ok: > + KASSERT(n->m_len >= off + len); > if (offp) > *offp = off; > return (n); > Index: netinet/ip6.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip6.h,v > diff -u -p -r1.22 ip6.h > --- netinet/ip6.h 24 May 2025 12:27:23 -0000 1.22 > +++ netinet/ip6.h 24 May 2025 14:21:57 -0000 > @@ -267,28 +267,27 @@ struct ip6_frag { > > #ifdef _KERNEL > /* > - * IP6_EXTHDR_GET ensures that intermediate protocol header (from "off" to > - * "len") is located in single mbuf, on contiguous memory region. > + * ip6_exthdr_get() ensures that intermediate protocol header (from "off" > + * to "len") is located in single mbuf, on contiguous memory region. > * The pointer to the region will be returned to pointer variable "val", > * with type "typ". > */ > -#define IP6_EXTHDR_GET(val, typ, mp, off, len) \ > -do { \ > - struct mbuf *t; \ > - int tmp; \ > - if ((*(mp))->m_len >= (off) + (len)) \ > - (val) = (typ)(mtod((*(mp)), caddr_t) + (off)); \ > - else { \ > - t = m_pulldown((*(mp)), (off), (len), &tmp); \ > - if (t) { \ > - if (t->m_len < tmp + (len)) \ > - panic("m_pulldown malfunction"); \ > - (val) = (typ)(mtod(t, caddr_t) + tmp); \ > - } else { \ > - (val) = (typ)NULL; \ > - (*(mp)) = NULL; \ > - } \ > - } \ > -} while (/* CONSTCOND */ 0) > + > +static inline void * > +ip6_exthdr_get(struct mbuf **mp, int off, int len) > +{ > + struct mbuf *t; > + int toff; > + > + if ((*mp)->m_len >= off + len) > + return (mtod(*mp, caddr_t) + off); > + > + t = m_pulldown(*mp, off, len, &toff); > + if (t == NULL) { > + *mp = NULL; > + return (NULL); > + } > + return (mtod(t, caddr_t) + toff); > +} > #endif /* _KERNEL */ > #endif /* _NETINET_IP6_H_ */ > Index: netinet/tcp_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v > diff -u -p -r1.448 tcp_input.c > --- netinet/tcp_input.c 24 May 2025 12:27:23 -0000 1.448 > +++ netinet/tcp_input.c 24 May 2025 14:08:47 -0000 > @@ -448,8 +448,8 @@ tcp_input_solocked(struct mbuf **mp, int > * Get IP and TCP header together in first mbuf. > * Note: IP leaves IP header in first mbuf. > */ > - IP6_EXTHDR_GET(th, struct tcphdr *, mp, iphlen, sizeof(*th)); > - if (!th) { > + th = ip6_exthdr_get(mp, iphlen, sizeof(*th)); > + if (th == NULL) { > tcpstat_inc(tcps_rcvshort); > return IPPROTO_DONE; > } > @@ -532,8 +532,8 @@ tcp_input_solocked(struct mbuf **mp, int > } > tlen -= off; > if (off > sizeof(struct tcphdr)) { > - IP6_EXTHDR_GET(th, struct tcphdr *, mp, iphlen, off); > - if (!th) { > + th = ip6_exthdr_get(mp, iphlen, off); > + if (th == NULL) { > tcpstat_inc(tcps_rcvshort); > return IPPROTO_DONE; > } > Index: netinet/udp_usrreq.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/udp_usrreq.c,v > diff -u -p -r1.338 udp_usrreq.c > --- netinet/udp_usrreq.c 24 May 2025 12:27:23 -0000 1.338 > +++ netinet/udp_usrreq.c 24 May 2025 14:09:18 -0000 > @@ -213,8 +213,8 @@ udp_input(struct mbuf **mp, int *offp, i > > udpstat_inc(udps_ipackets); > > - IP6_EXTHDR_GET(uh, struct udphdr *, mp, iphlen, sizeof(struct udphdr)); > - if (!uh) { > + uh = ip6_exthdr_get(mp, iphlen, sizeof(struct udphdr)); > + if (uh == NULL) { > udpstat_inc(udps_hdrops); > return IPPROTO_DONE; > } > Index: netinet6/dest6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/dest6.c,v > diff -u -p -r1.22 dest6.c > --- netinet6/dest6.c 24 May 2025 12:27:23 -0000 1.22 > +++ netinet6/dest6.c 24 May 2025 14:10:09 -0000 > @@ -57,12 +57,12 @@ dest6_input(struct mbuf **mp, int *offp, > u_int8_t *opt; > > /* validation of the length of the header */ > - IP6_EXTHDR_GET(dstopts, struct ip6_dest *, mp, off, sizeof(*dstopts)); > + dstopts = ip6_exthdr_get(mp, off, sizeof(*dstopts)); > if (dstopts == NULL) > return IPPROTO_DONE; > dstoptlen = (dstopts->ip6d_len + 1) << 3; > > - IP6_EXTHDR_GET(dstopts, struct ip6_dest *, mp, off, dstoptlen); > + dstopts = ip6_exthdr_get(mp, off, dstoptlen); > if (dstopts == NULL) > return IPPROTO_DONE; > off += dstoptlen; > Index: netinet6/frag6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/frag6.c,v > diff -u -p -r1.91 frag6.c > --- netinet6/frag6.c 24 May 2025 12:27:23 -0000 1.91 > +++ netinet6/frag6.c 24 May 2025 14:15:34 -0000 > @@ -125,7 +125,7 @@ frag6_input(struct mbuf **mp, int *offp, > u_int8_t ecn, ecn0; > > ip6 = mtod(*mp, struct ip6_hdr *); > - IP6_EXTHDR_GET(ip6f, struct ip6_frag *, mp, offset, sizeof(*ip6f)); > + ip6f = ip6_exthdr_get(mp, offset, sizeof(*ip6f)); > if (ip6f == NULL) > return IPPROTO_DONE; > > @@ -451,8 +451,7 @@ frag6_input(struct mbuf **mp, int *offp, > int prvnxt = ip6_get_prevhdr(*mp, offset); > uint8_t *prvnxtp; > > - IP6_EXTHDR_GET(prvnxtp, uint8_t *, mp, prvnxt, > - sizeof(*prvnxtp)); > + prvnxtp = ip6_exthdr_get(mp, prvnxt, sizeof(*prvnxtp)); > if (prvnxtp == NULL) > goto dropfrag; > *prvnxtp = nxt; > Index: netinet6/icmp6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/icmp6.c,v > diff -u -p -r1.265 icmp6.c > --- netinet6/icmp6.c 24 May 2025 12:27:23 -0000 1.265 > +++ netinet6/icmp6.c 24 May 2025 14:14:28 -0000 > @@ -281,8 +281,7 @@ icmp6_do_error(struct mbuf *m, int type, > if (off >= 0 && nxt == IPPROTO_ICMPV6) { > struct icmp6_hdr *icp; > > - IP6_EXTHDR_GET(icp, struct icmp6_hdr *, &m, off, > - sizeof(*icp)); > + icp = ip6_exthdr_get(&m, off, sizeof(*icp)); > if (icp == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (NULL); > @@ -407,7 +406,7 @@ icmp6_input(struct mbuf **mp, int *offp, > /* > * calculate the checksum > */ > - IP6_EXTHDR_GET(icmp6, struct icmp6_hdr *, mp, off, sizeof(*icmp6)); > + icmp6 = ip6_exthdr_get(mp, off, sizeof(*icmp6)); > if (icmp6 == NULL) { > icmp6stat_inc(icp6s_tooshort); > return IPPROTO_DONE; > @@ -588,8 +587,7 @@ icmp6_input(struct mbuf **mp, int *offp, > n->m_next = n0; > } else { > deliverecho: > - IP6_EXTHDR_GET(nicmp6, struct icmp6_hdr *, &n, off, > - sizeof(*nicmp6)); > + nicmp6 = ip6_exthdr_get(&n, off, sizeof(*nicmp6)); > noff = off; > } > if (n) { > @@ -762,8 +760,8 @@ icmp6_notify_error(struct mbuf *m, int o > icmp6stat_inc(icp6s_tooshort); > goto freeit; > } > - IP6_EXTHDR_GET(icmp6, struct icmp6_hdr *, &m, off, > - sizeof(*icmp6) + sizeof(struct ip6_hdr)); > + icmp6 = ip6_exthdr_get(&m, off, > + sizeof(*icmp6) + sizeof(struct ip6_hdr)); > if (icmp6 == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > @@ -791,8 +789,7 @@ icmp6_notify_error(struct mbuf *m, int o > case IPPROTO_HOPOPTS: > case IPPROTO_DSTOPTS: > case IPPROTO_AH: > - IP6_EXTHDR_GET(eh, struct ip6_ext *, &m, > - eoff, sizeof(*eh)); > + eh = ip6_exthdr_get(&m, eoff, sizeof(*eh)); > if (eh == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > @@ -813,8 +810,7 @@ icmp6_notify_error(struct mbuf *m, int o > * information that depends on the final > * destination (e.g. path MTU). > */ > - IP6_EXTHDR_GET(rth, struct ip6_rthdr *, &m, > - eoff, sizeof(*rth)); > + rth = ip6_exthdr_get(&m, eoff, sizeof(*rth)); > if (rth == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > @@ -832,24 +828,23 @@ icmp6_notify_error(struct mbuf *m, int o > rth->ip6r_type == IPV6_RTHDR_TYPE_0) { > int hops; > > - IP6_EXTHDR_GET(rth0, > - struct ip6_rthdr0 *, &m, > - eoff, rthlen); > + rth0 = ip6_exthdr_get(&m, eoff, rthlen); > if (rth0 == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > } > /* just ignore a bogus header */ > if ((rth0->ip6r0_len % 2) == 0 && > - (hops = rth0->ip6r0_len/2)) > - finaldst = (struct in6_addr *)(rth0 + 1) + (hops - 1); > + (hops = rth0->ip6r0_len/2)) { > + finaldst = (struct in6_addr *) > + (rth0 + 1) + (hops - 1); > + } > } > eoff += rthlen; > nxt = rth->ip6r_nxt; > break; > case IPPROTO_FRAGMENT: > - IP6_EXTHDR_GET(fh, struct ip6_frag *, &m, > - eoff, sizeof(*fh)); > + fh = ip6_exthdr_get(&m, eoff, sizeof(*fh)); > if (fh == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > @@ -879,8 +874,8 @@ icmp6_notify_error(struct mbuf *m, int o > } > } > notify: > - IP6_EXTHDR_GET(icmp6, struct icmp6_hdr *, &m, off, > - sizeof(*icmp6) + sizeof(struct ip6_hdr)); > + icmp6 = ip6_exthdr_get(&m, off, > + sizeof(*icmp6) + sizeof(struct ip6_hdr)); > if (icmp6 == NULL) { > icmp6stat_inc(icp6s_tooshort); > return (-1); > @@ -1204,7 +1199,7 @@ icmp6_redirect_input(struct mbuf *m, int > if (!(ifp->if_xflags & IFXF_AUTOCONF6)) > goto freeit; > > - IP6_EXTHDR_GET(nd_rd, struct nd_redirect *, &m, off, icmp6len); > + nd_rd = ip6_exthdr_get(&m, off, icmp6len); > if (nd_rd == NULL) { > icmp6stat_inc(icp6s_tooshort); > if_put(ifp); > Index: netinet6/ip6_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v > diff -u -p -r1.271 ip6_input.c > --- netinet6/ip6_input.c 24 May 2025 12:27:23 -0000 1.271 > +++ netinet6/ip6_input.c 24 May 2025 14:19:44 -0000 > @@ -685,8 +685,8 @@ ip6_hbhchcheck(struct mbuf **mp, int *of > (caddr_t)&ip6->ip6_plen - (caddr_t)ip6); > goto bad; > } > - IP6_EXTHDR_GET(hbh, struct ip6_hbh *, mp, > - sizeof(struct ip6_hdr), sizeof(struct ip6_hbh)); > + hbh = ip6_exthdr_get(mp, sizeof(struct ip6_hdr), > + sizeof(struct ip6_hbh)); > if (hbh == NULL) { > ip6stat_inc(ip6s_tooshort); > goto bad; > @@ -814,15 +814,14 @@ ip6_hopopts_input(struct mbuf **mp, int > struct ip6_hbh *hbh; > > /* validation of the length of the header */ > - IP6_EXTHDR_GET(hbh, struct ip6_hbh *, mp, > - sizeof(struct ip6_hdr), sizeof(struct ip6_hbh)); > + hbh = ip6_exthdr_get(mp, sizeof(struct ip6_hdr), > + sizeof(struct ip6_hbh)); > if (hbh == NULL) { > ip6stat_inc(ip6s_tooshort); > return -1; > } > hbhlen = (hbh->ip6h_len + 1) << 3; > - IP6_EXTHDR_GET(hbh, struct ip6_hbh *, mp, sizeof(struct ip6_hdr), > - hbhlen); > + hbh = ip6_exthdr_get(mp, sizeof(struct ip6_hdr), hbhlen); > if (hbh == NULL) { > ip6stat_inc(ip6s_tooshort); > return -1; > Index: netinet6/mld6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/mld6.c,v > diff -u -p -r1.66 mld6.c > --- netinet6/mld6.c 24 May 2025 12:27:23 -0000 1.66 > +++ netinet6/mld6.c 24 May 2025 14:15:51 -0000 > @@ -179,7 +179,7 @@ mld6_input(struct mbuf *m, int off) > /* XXX: These are necessary for KAME's link-local hack */ > struct in6_addr all_nodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT; > > - IP6_EXTHDR_GET(mldh, struct mld_hdr *, &m, off, sizeof(*mldh)); > + mldh = ip6_exthdr_get(&m, off, sizeof(*mldh)); > if (mldh == NULL) { > icmp6stat_inc(icp6s_tooshort); > return; > Index: netinet6/nd6_nbr.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_nbr.c,v > diff -u -p -r1.159 nd6_nbr.c > --- netinet6/nd6_nbr.c 24 May 2025 12:27:23 -0000 1.159 > +++ netinet6/nd6_nbr.c 24 May 2025 14:16:32 -0000 > @@ -117,7 +117,7 @@ nd6_ns_input(struct mbuf *m, int off, in > if (ifp == NULL) > goto freeit; > > - IP6_EXTHDR_GET(nd_ns, struct nd_neighbor_solicit *, &m, off, icmp6len); > + nd_ns = ip6_exthdr_get(&m, off, icmp6len); > if (nd_ns == NULL) { > icmp6stat_inc(icp6s_tooshort); > if_put(ifp); > @@ -540,7 +540,7 @@ nd6_na_input(struct mbuf *m, int off, in > if (ip6->ip6_hlim != 255) > goto bad; > > - IP6_EXTHDR_GET(nd_na, struct nd_neighbor_advert *, &m, off, icmp6len); > + nd_na = ip6_exthdr_get(&m, off, icmp6len); > if (nd_na == NULL) { > icmp6stat_inc(icp6s_tooshort); > if_put(ifp); > Index: netinet6/nd6_rtr.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_rtr.c,v > diff -u -p -r1.174 nd6_rtr.c > --- netinet6/nd6_rtr.c 24 May 2025 12:27:24 -0000 1.174 > +++ netinet6/nd6_rtr.c 24 May 2025 14:17:58 -0000 > @@ -92,8 +92,7 @@ nd6_rtr_cache(struct mbuf *m, int off, i > if (IN6_IS_ADDR_UNSPECIFIED(&saddr6)) > goto freeit; > > - IP6_EXTHDR_GET(nd_rs, struct nd_router_solicit *, &m, off, > - icmp6len); > + nd_rs = ip6_exthdr_get(&m, off, icmp6len); > if (nd_rs == NULL) { > icmp6stat_inc(icp6s_tooshort); > return; > @@ -109,8 +108,7 @@ nd6_rtr_cache(struct mbuf *m, int off, i > if (!IN6_IS_ADDR_LINKLOCAL(&saddr6)) > goto bad; > > - IP6_EXTHDR_GET(nd_ra, struct nd_router_advert *, &m, off, > - icmp6len); > + nd_ra = ip6_exthdr_get(&m, off, icmp6len); > if (nd_ra == NULL) { > icmp6stat_inc(icp6s_tooshort); > return; > Index: netinet6/raw_ip6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/raw_ip6.c,v > diff -u -p -r1.191 raw_ip6.c > --- netinet6/raw_ip6.c 24 May 2025 12:27:24 -0000 1.191 > +++ netinet6/raw_ip6.c 24 May 2025 14:20:04 -0000 > @@ -147,8 +147,7 @@ rip6_input(struct mbuf **mp, int *offp, > if (proto == IPPROTO_ICMPV6) { > struct icmp6_hdr *icmp6; > > - IP6_EXTHDR_GET(icmp6, struct icmp6_hdr *, mp, *offp, > - sizeof(*icmp6)); > + icmp6 = ip6_exthdr_get(mp, *offp, sizeof(*icmp6)); > if (icmp6 == NULL) > return IPPROTO_DONE; > type = icmp6->icmp6_type; > Index: netinet6/route6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/route6.c,v > diff -u -p -r1.24 route6.c > --- netinet6/route6.c 24 May 2025 12:27:24 -0000 1.24 > +++ netinet6/route6.c 24 May 2025 14:20:27 -0000 > @@ -59,7 +59,7 @@ route6_input(struct mbuf **mp, int *offp > int off = *offp, rhlen; > > ip6 = mtod(*mp, struct ip6_hdr *); > - IP6_EXTHDR_GET(rh, struct ip6_rthdr *, mp, off, sizeof(*rh)); > + rh = ip6_exthdr_get(mp, off, sizeof(*rh)); > if (rh == NULL) { > ip6stat_inc(ip6s_tooshort); > return IPPROTO_DONE; > -- :wq Claudio