Download raw body.
IP6_EXTHDR_GET pass mbuf pointer
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
IP6_EXTHDR_GET pass mbuf pointer