On Tue, Jan 31, 2023 at 11:32:44PM +0100, Jan Klemkow wrote:
> On Tue, Jan 31, 2023 at 09:12:51PM +0100, Christian Weisgerber wrote:
> > > - I turned the KASSERTS to returns.
The old code only parsed the TCP|UDP header if the driver needed
it and M_(TCP|UDP)_CSUM_OUT was set. So we may have more calls to
m_getptr().
Should we check for the M_.*_CSUM_OUT flags in ether_extract_headers()?
Or would that be a micro optimization that does not change much?
> > > - Check if the mbuf is large enough for an ether header.
> if (m == NULL || m->m_len - hoff < sizeof(*ext->ip4))
Could you change this code to
if (m == NULL || m->m_len < sizeof(*ext->ip4) + hoff)
I don't want to think about what happens if there is an integer
underflow. Overflow cannot happen as hoff should be much smaller
than maximum size_t. Or is it just me who does not understand the
C conversion from negative int to size_t comparison.
> > > - additionally #ifdef'd INET6 around the ip6_hdr in the new struct
Why? Usually I prefer the same struct size also for non generic
kernels. If you compile special kernels, user land debugging tools
behave stange and counting bytes in ddb gets hard.
otherwise OK bluhm@
> Index: dev/pci/if_ix.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_ix.c,v
> retrieving revision 1.191
> diff -u -p -r1.191 if_ix.c
> --- dev/pci/if_ix.c 26 Jan 2023 07:32:39 -0000 1.191
> +++ dev/pci/if_ix.c 31 Jan 2023 21:05:40 -0000
> @@ -2477,25 +2477,16 @@ static inline int
> ixgbe_csum_offload(struct mbuf *mp, uint32_t *vlan_macip_lens,
> uint32_t *type_tucmd_mlhl, uint32_t *olinfo_status)
> {
> - struct ether_header *eh = mtod(mp, struct ether_header *);
> - struct mbuf *m;
> - int hoff;
> + struct ether_extracted ext;
> int offload = 0;
> uint32_t iphlen;
> - uint8_t ipproto;
>
> - *vlan_macip_lens |= (sizeof(*eh) << IXGBE_ADVTXD_MACLEN_SHIFT);
> + ether_extract_headers(mp, &ext);
>
> - switch (ntohs(eh->ether_type)) {
> - case ETHERTYPE_IP: {
> - struct ip *ip;
> + *vlan_macip_lens |= (sizeof(*ext.eh) << IXGBE_ADVTXD_MACLEN_SHIFT);
>
> - m = m_getptr(mp, sizeof(*eh), &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip));
> - ip = (struct ip *)(mtod(m, caddr_t) + hoff);
> -
> - iphlen = ip->ip_hl << 2;
> - ipproto = ip->ip_p;
> + if (ext.ip4) {
> + iphlen = ext.ip4->ip_hl << 2;
>
> if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
> *olinfo_status |= IXGBE_TXD_POPTS_IXSM << 8;
> @@ -2503,46 +2494,30 @@ ixgbe_csum_offload(struct mbuf *mp, uint
> }
>
> *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4;
> - break;
> - }
> -
> #ifdef INET6
> - case ETHERTYPE_IPV6: {
> - struct ip6_hdr *ip6;
> -
> - m = m_getptr(mp, sizeof(*eh), &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip6));
> - ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff);
> -
> - iphlen = sizeof(*ip6);
> - ipproto = ip6->ip6_nxt;
> + } else if (ext.ip6) {
> + iphlen = sizeof(*ext.ip6);
>
> *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6;
> - break;
> - }
> #endif
> -
> - default:
> + } else {
> return offload;
> }
>
> *vlan_macip_lens |= iphlen;
>
> - switch (ipproto) {
> - case IPPROTO_TCP:
> + if (ext.tcp) {
> *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_TCP;
> if (ISSET(mp->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) {
> *olinfo_status |= IXGBE_TXD_POPTS_TXSM << 8;
> offload = 1;
> }
> - break;
> - case IPPROTO_UDP:
> + } else if (ext.udp) {
> *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_UDP;
> if (ISSET(mp->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) {
> *olinfo_status |= IXGBE_TXD_POPTS_TXSM << 8;
> offload = 1;
> }
> - break;
> }
>
> return offload;
> Index: dev/pci/if_ixl.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_ixl.c,v
> retrieving revision 1.86
> diff -u -p -r1.86 if_ixl.c
> --- dev/pci/if_ixl.c 26 Jan 2023 07:32:39 -0000 1.86
> +++ dev/pci/if_ixl.c 31 Jan 2023 21:05:40 -0000
> @@ -2784,10 +2784,8 @@ ixl_load_mbuf(bus_dma_tag_t dmat, bus_dm
> static uint64_t
> ixl_tx_setup_offload(struct mbuf *m0)
> {
> - struct mbuf *m;
> - int hoff;
> + struct ether_extracted ext;
> uint64_t hlen;
> - uint8_t ipproto;
> uint64_t offload = 0;
>
> if (ISSET(m0->m_flags, M_VLANTAG)) {
> @@ -2800,39 +2798,21 @@ ixl_tx_setup_offload(struct mbuf *m0)
> M_IPV4_CSUM_OUT|M_TCP_CSUM_OUT|M_UDP_CSUM_OUT))
> return (offload);
>
> - switch (ntohs(mtod(m0, struct ether_header *)->ether_type)) {
> - case ETHERTYPE_IP: {
> - struct ip *ip;
> -
> - m = m_getptr(m0, ETHER_HDR_LEN, &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip));
> - ip = (struct ip *)(mtod(m, caddr_t) + hoff);
> + ether_extract_headers(m0, &ext);
>
> + if (ext.ip4) {
> offload |= ISSET(m0->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT) ?
> IXL_TX_DESC_CMD_IIPT_IPV4_CSUM :
> IXL_TX_DESC_CMD_IIPT_IPV4;
>
> - hlen = ip->ip_hl << 2;
> - ipproto = ip->ip_p;
> - break;
> - }
> -
> + hlen = ext.ip4->ip_hl << 2;
> #ifdef INET6
> - case ETHERTYPE_IPV6: {
> - struct ip6_hdr *ip6;
> -
> - m = m_getptr(m0, ETHER_HDR_LEN, &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip6));
> - ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff);
> -
> + } else if (ext.ip6) {
> offload |= IXL_TX_DESC_CMD_IIPT_IPV6;
>
> - hlen = sizeof(*ip6);
> - ipproto = ip6->ip6_nxt;
> - break;
> - }
> + hlen = sizeof(*ext.ip6);
> #endif
> - default:
> + } else {
> panic("CSUM_OUT set for non-IP packet");
> /* NOTREACHED */
> }
> @@ -2840,30 +2820,12 @@ ixl_tx_setup_offload(struct mbuf *m0)
> offload |= (ETHER_HDR_LEN >> 1) << IXL_TX_DESC_MACLEN_SHIFT;
> offload |= (hlen >> 2) << IXL_TX_DESC_IPLEN_SHIFT;
>
> - switch (ipproto) {
> - case IPPROTO_TCP: {
> - struct tcphdr *th;
> -
> - if (!ISSET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT))
> - break;
> -
> - m = m_getptr(m, hoff + hlen, &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*th));
> - th = (struct tcphdr *)(mtod(m, caddr_t) + hoff);
> -
> + if (ext.tcp && ISSET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) {
> offload |= IXL_TX_DESC_CMD_L4T_EOFT_TCP;
> - offload |= (uint64_t)th->th_off << IXL_TX_DESC_L4LEN_SHIFT;
> - break;
> - }
> -
> - case IPPROTO_UDP:
> - if (!ISSET(m0->m_pkthdr.csum_flags, M_UDP_CSUM_OUT))
> - break;
> -
> + offload |= (uint64_t)ext.tcp->th_off << IXL_TX_DESC_L4LEN_SHIFT;
> + } else if (ext.udp && ISSET(m0->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) {
> offload |= IXL_TX_DESC_CMD_L4T_EOFT_UDP;
> - offload |= (sizeof(struct udphdr) >> 2) <<
> - IXL_TX_DESC_L4LEN_SHIFT;
> - break;
> + offload |= (sizeof(*ext.udp) >> 2) << IXL_TX_DESC_L4LEN_SHIFT;
> }
>
> return (offload);
> Index: net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.286
> diff -u -p -r1.286 if_ethersubr.c
> --- net/if_ethersubr.c 26 Jan 2023 07:32:40 -0000 1.286
> +++ net/if_ethersubr.c 31 Jan 2023 21:24:04 -0000
> @@ -98,6 +98,10 @@ didn't get a copy, you may request one f
> #include <netinet/in.h>
> #include <netinet/if_ether.h>
> #include <netinet/ip_ipsp.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +#include <netinet/tcp.h>
> +#include <netinet/udp.h>
>
> #if NBPFILTER > 0
> #include <net/bpf.h>
> @@ -1033,4 +1037,67 @@ ether_e64_to_addr(struct ether_addr *ea,
> ea->ether_addr_octet[--i] = e64;
> e64 >>= 8;
> } while (i > 0);
> +}
> +
> +/* Parse different TCP/IP protocol headers for a quick view inside an mbuf.
> */
> +void
> +ether_extract_headers(struct mbuf *mp, struct ether_extracted *ext)
> +{
> + struct mbuf *m;
> + uint64_t hlen;
> + int hoff;
> + uint8_t ipproto;
> +
> + /* Return NULL if header was not recognized. */
> + memset(ext, 0, sizeof(*ext));
> +
> + if (mp->m_len < sizeof(*ext->eh))
> + return;
> +
> + ext->eh = mtod(mp, struct ether_header *);
> + switch (ntohs(ext->eh->ether_type)) {
> + case ETHERTYPE_IP:
> + m = m_getptr(mp, sizeof(*ext->eh), &hoff);
> + if (m == NULL || m->m_len - hoff < sizeof(*ext->ip4))
> + return;
> + ext->ip4 = (struct ip *)(mtod(m, caddr_t) + hoff);
> +
> + if (ISSET(ntohs(ext->ip4->ip_off), IP_MF|IP_OFFMASK))
> + return;
> +
> + hlen = ext->ip4->ip_hl << 2;
> + ipproto = ext->ip4->ip_p;
> +
> + break;
> +#ifdef INET6
> + case ETHERTYPE_IPV6:
> + m = m_getptr(mp, sizeof(*ext->eh), &hoff);
> + if (m == NULL || m->m_len - hoff < sizeof(*ext->ip6))
> + return;
> + ext->ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff);
> +
> + hlen = sizeof(*ext->ip6);
> + ipproto = ext->ip6->ip6_nxt;
> +
> + break;
> +#endif
> + default:
> + return;
> + }
> +
> + switch (ipproto) {
> + case IPPROTO_TCP:
> + m = m_getptr(m, hoff + hlen, &hoff);
> + if (m == NULL || m->m_len - hoff < sizeof(*ext->tcp))
> + return;
> + ext->tcp = (struct tcphdr *)(mtod(m, caddr_t) + hoff);
> + break;
> +
> + case IPPROTO_UDP:
> + m = m_getptr(m, hoff + hlen, &hoff);
> + if (m == NULL || m->m_len - hoff < sizeof(*ext->udp))
> + return;
> + ext->udp = (struct udphdr *)(mtod(m, caddr_t) + hoff);
> + break;
> + }
> }
> Index: netinet/if_ether.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/if_ether.h,v
> retrieving revision 1.86
> diff -u -p -r1.86 if_ether.h
> --- netinet/if_ether.h 26 Jan 2023 07:32:40 -0000 1.86
> +++ netinet/if_ether.h 31 Jan 2023 21:05:40 -0000
> @@ -297,6 +297,18 @@ const struct ether_brport *
> uint64_t ether_addr_to_e64(const struct ether_addr *);
> void ether_e64_to_addr(struct ether_addr *, uint64_t);
>
> +struct ether_extracted {
> + struct ether_header *eh;
> + struct ip *ip4;
> +#ifdef INET6
> + struct ip6_hdr *ip6;
> +#endif
> + struct tcphdr *tcp;
> + struct udphdr *udp;
> +};
> +
> +void ether_extract_headers(struct mbuf *, struct ether_extracted *);
> +
> /*
> * Ethernet multicast address structure. There is one of these for each
> * multicast address or range of multicast addresses that we are supposed
