Re: Problem with checksum offloading on RPi3 (PF + Jails involved)

Index: sys/dev/usb/net/if_smsc.c
===================================================================
--- sys/dev/usb/net/if_smsc.c	(revision 367712)
+++ sys/dev/usb/net/if_smsc.c	(working copy)
@@ -94,6 +94,8 @@ __FBSDID("$FreeBSD$");
 
 #include <netinet/in.h>
 #include <netinet/ip.h>
+#include <netinet/tcp.h>
+#include <netinet/udp.h>
 
 #include "opt_platform.h"
 
@@ -198,6 +200,9 @@ static void	smsc_ifmedia_sts(struct ifnet *, struc
 
 static int smsc_chip_init(struct smsc_softc *sc);
 static int smsc_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data);
+static void smsc_rxfixup(struct ifnet *ifp, struct mbuf *m, int *pktlen);
+static int smsc_rxeof(struct usb_ether *ue, struct usb_page_cache *pc,
+    unsigned int offset, struct ifnet *ifp, int pktlen);
 
 static const struct usb_config smsc_config[SMSC_N_TRANSFER] = {
 	[SMSC_BULK_DT_WR] = {
@@ -795,11 +800,8 @@ static int smsc_sethwcsum(struct smsc_softc *sc)
 		return (err);
 	}
 
-	/* Enable/disable the Rx checksum */
-	if ((ifp->if_capabilities & ifp->if_capenable) & IFCAP_RXCSUM)
-		val |= SMSC_COE_CTRL_RX_EN;
-	else
-		val &= ~SMSC_COE_CTRL_RX_EN;
+	/* Always enable Rx checksum to simplify RX packet validation. */
+	val |= SMSC_COE_CTRL_RX_EN;
 
 	/* Enable/disable the Tx checksum (currently not supported) */
 	if ((ifp->if_capabilities & ifp->if_capenable) & IFCAP_TXCSUM)
@@ -925,6 +927,119 @@ smsc_init(struct usb_ether *ue)
 	smsc_start(ue);
 }
 
+static void
+smsc_rxfixup(struct ifnet *ifp, struct mbuf *m, int *pktlen)
+{
+	struct ether_header *eh;
+	struct ip *ip;
+	struct udphdr *uh;
+	int hlen, iplen, len;
+	uint16_t csum;
+
+	len = *pktlen;
+	if (len < sizeof(struct ether_header) + sizeof(struct ip))
+		return;
+	eh = mtod(m, struct ether_header *);
+	if (eh->ether_type != htons(ETHERTYPE_IP))
+		return;
+	ip = (struct ip *)(eh + 1);
+	if (ip->ip_v != IPVERSION)
+		return;
+
+	hlen = ip->ip_hl << 2;
+	len -= sizeof(struct ether_header);
+	if (hlen < sizeof(struct ip))
+		return;
+	if (ntohs(ip->ip_len) < hlen)
+		return;
+	iplen = ntohs(ip->ip_len);
+	if (iplen != len) {
+		if (iplen < sizeof(struct ip) + 26) {
+			/*
+			 * XXX
+			 * It seems H/W does not provide a reliable way to
+			 * know received ethernet frame length when sender
+			 * adds padding bytes to meet minimum ethernet frame
+			 * length.
+			 * Find actual ethernet frame size by checking total IP
+			 * datagram length.  This does not work for VLAN tagged
+			 * frames or non-IPv4 datagrams though.
+			 */
+			*pktlen -= (26 - iplen);
+
+			/*
+			 * The checksum appears to be simplistically calculated
+			 * over the udp/tcp header and data up to the end of the
+			 * eth frame.  Which means if the eth frame is padded
+			 * the csum calculation is incorrectly performed over
+			 * the padding bytes as well. Therefore to be safe we
+			 * ignore the H/W csum on frames less than or equal to
+			 * 64 bytes.
+			 */
+		}
+		/* IP datagram length does not match packet length. */
+		return;
+	}
+	if ((ifp->if_capenable & IFCAP_RXCSUM) == 0)
+		return;
+
+	if (ip->ip_off & htons(IP_MF | IP_OFFMASK))
+		return;	/* can't handle fragmented packet. */
+
+	switch (ip->ip_p) {
+	case IPPROTO_TCP:
+		if (len < (hlen + sizeof(struct tcphdr)))
+			return;
+		break;
+	case IPPROTO_UDP:
+		if (len < (hlen + sizeof(struct udphdr)))
+			return;
+		uh = (struct udphdr *)((caddr_t)ip + hlen);
+		if (uh->uh_sum == 0)
+			return;	/* no checksum. */
+		break;
+	default:
+		return;
+	}
+
+	if ((hlen - sizeof(struct ip)) > 0) {
+		/* XXX checksum fixup for IP options. */
+		return;
+	}
+	/* Extract H/W computed checksum. */
+	csum = be16dec(mtod(m, caddr_t) + *pktlen + ETHER_CRC_LEN);
+	m->m_pkthdr.csum_flags |= CSUM_DATA_VALID;
+	m->m_pkthdr.csum_data = csum;
+}
+
+static int
+smsc_rxeof(struct usb_ether *ue, struct usb_page_cache *pc, unsigned int offset,
+    struct ifnet *ifp, int pktlen)
+{
+	struct mbuf *m;
+
+	m = m_getcl(M_NOWAIT, MT_DATA, M_PKTHDR);
+	if (m == NULL) {
+		if_inc_counter(ifp, IFCOUNTER_IQDROPS, 1);
+		return (ENOMEM);
+	}
+	m->m_len = m->m_pkthdr.len = MCLBYTES;
+	m_adj(m, ETHER_ALIGN);
+
+	usbd_copy_out(pc, offset, mtod(m, caddr_t), pktlen);
+
+	if_inc_counter(ifp, IFCOUNTER_IPACKETS, 1);
+	m->m_pkthdr.rcvif = ifp;
+	m->m_pkthdr.len = m->m_len = pktlen;
+
+	pktlen -= (ETHER_CRC_LEN + SMSC_RX_CSUM_LEN);
+	smsc_rxfixup(ifp, m, &pktlen);
+	m->m_pkthdr.len = m->m_len = pktlen;
+
+	(void)mbufq_enqueue(&ue->ue_rxq, m);
+	return (0);
+}
+
 /**
  *	smsc_bulk_read_callback - Read callback used to process the USB URB
  *	@xfer: the USB transfer
@@ -942,7 +1057,6 @@ smsc_bulk_read_callback(struct usb_xfer *xfer, usb
 	struct smsc_softc *sc = usbd_xfer_softc(xfer);
 	struct usb_ether *ue = &sc->sc_ue;
 	struct ifnet *ifp = uether_getifp(ue);
-	struct mbuf *m;
 	struct usb_page_cache *pc;
 	uint32_t rxhdr;
 	int pktlen;
@@ -956,7 +1070,7 @@ smsc_bulk_read_callback(struct usb_xfer *xfer, usb
 	case USB_ST_TRANSFERRED:
 
 		/* There is always a zero length frame after bringing the IF up */
-		if (actlen < (sizeof(rxhdr) + ETHER_CRC_LEN))
+		if (actlen < (sizeof(rxhdr) + ETHER_MIN_LEN + SMSC_RX_CSUM_LEN))
 			goto tr_setup;
 
 		/* There maybe multiple packets in the USB frame, each will have a 
@@ -975,9 +1089,19 @@ smsc_bulk_read_callback(struct usb_xfer *xfer, usb
 				goto tr_setup;
 
 			usbd_copy_out(pc, off, &rxhdr, sizeof(rxhdr));
-			off += (sizeof(rxhdr) + ETHER_ALIGN);
+			off += sizeof(rxhdr);
 			rxhdr = le32toh(rxhdr);
-		
+
+			/*
+			 * XXX
+			 * It seems H/W does not have a feature to limit maximum
+			 * ethernet frame length allowed to receive path. The
+			 * SMSC_RX_STAT_FRM_LENGTH macro can return up to 2^14
+			 * bytes.  I think it's too dangerous to trust the H/W
+			 * reported RX size without further information from
+			 * vendor.  Drop all RX ethernet frames bigger than
+			 * (MCLBYTES - ETHER_ALIGN) bytes for safety.
+			 */
 			pktlen = (uint16_t)SMSC_RX_STAT_FRM_LENGTH(rxhdr);
 			
 			smsc_dbg_printf(sc, "rx : rxhdr 0x%08x : pktlen %d : actlen %d : "
@@ -989,88 +1113,19 @@ smsc_bulk_read_callback(struct usb_xfer *xfer, usb
 				if_inc_counter(ifp, IFCOUNTER_IERRORS, 1);
 				if (rxhdr & SMSC_RX_STAT_COLLISION)
 					if_inc_counter(ifp, IFCOUNTER_COLLISIONS, 1);
-			} else {
-				/* Check if the ethernet frame is too big or too small */
-				if ((pktlen < ETHER_HDR_LEN) || (pktlen > (actlen - off)))
+				if (pktlen > (MCLBYTES - ETHER_ALIGN)) {
+					/* Lost sync. */
 					goto tr_setup;
-			
-				/* Create a new mbuf to store the packet in */
-				m = uether_newbuf();
-				if (m == NULL) {
-					smsc_warn_printf(sc, "failed to create new mbuf\n");
-					if_inc_counter(ifp, IFCOUNTER_IQDROPS, 1);
-					goto tr_setup;
 				}
-				if (pktlen > m->m_len) {
-					smsc_dbg_printf(sc, "buffer too small %d vs %d bytes",
-					    pktlen, m->m_len);
-					if_inc_counter(ifp, IFCOUNTER_IQDROPS, 1);
-					m_freem(m);
+			} else {
+				if (pktlen < (ETHER_MIN_LEN + SMSC_RX_CSUM_LEN) ||
+				    pktlen > (MCLBYTES - ETHER_ALIGN) ||
+				    pktlen > (actlen - off)) {
+					/* Lost sync. */
+					if_inc_counter(ifp, IFCOUNTER_IERRORS, 1);
 					goto tr_setup;
 				}
-				usbd_copy_out(pc, off, mtod(m, uint8_t *), pktlen);
-
-				/* Check if RX TCP/UDP checksumming is being offloaded */
-				if ((ifp->if_capenable & IFCAP_RXCSUM) != 0) {
-					struct ether_header *eh;
-
-					eh = mtod(m, struct ether_header *);
-				
-					/* Remove the extra 2 bytes of the csum */
-					pktlen -= 2;
-
-					/* The checksum appears to be simplistically calculated
-					 * over the udp/tcp header and data up to the end of the
-					 * eth frame.  Which means if the eth frame is padded
-					 * the csum calculation is incorrectly performed over
-					 * the padding bytes as well. Therefore to be safe we
-					 * ignore the H/W csum on frames less than or equal to
-					 * 64 bytes.
-					 *
-					 * Ignore H/W csum for non-IPv4 packets.
-					 */
-					if ((be16toh(eh->ether_type) == ETHERTYPE_IP) &&
-					    (pktlen > ETHER_MIN_LEN)) {
-						struct ip *ip;
-
-						ip = (struct ip *)(eh + 1);
-						if ((ip->ip_v == IPVERSION) &&
-						    ((ip->ip_p == IPPROTO_TCP) ||
-						     (ip->ip_p == IPPROTO_UDP))) {
-							/* Indicate the UDP/TCP csum has been calculated */
-							m->m_pkthdr.csum_flags |= CSUM_DATA_VALID;
-
-							/* Copy the TCP/UDP checksum from the last 2 bytes
-							 * of the transfer and put in the csum_data field.
-							 */
-							usbd_copy_out(pc, (off + pktlen),
-							              &m->m_pkthdr.csum_data, 2);
-
-							/* The data is copied in network order, but the
-							 * csum algorithm in the kernel expects it to be
-							 * in host network order.
-							 */
-							m->m_pkthdr.csum_data = ntohs(m->m_pkthdr.csum_data);
-
-							smsc_dbg_printf(sc, "RX checksum offloaded (0x%04x)\n",
-							                m->m_pkthdr.csum_data);
-						}
-					}
-					
-					/* Need to adjust the offset as well or we'll be off
-					 * by 2 because the csum is removed from the packet
-					 * length.
-					 */
-					off += 2;
-				}
-			
-				/* Finally enqueue the mbuf on the receive queue */
-				/* Remove 4 trailing bytes */
-				if (pktlen < (4 + ETHER_HDR_LEN)) {
-					m_freem(m);
-					goto tr_setup;
-				}
-				uether_rxmbuf(ue, m, pktlen - 4);
+				smsc_rxeof(ue, pc, off, ifp, (int)pktlen);
 			}
 
 			/* Update the offset to move to the next potential packet */
@@ -1395,11 +1450,12 @@ smsc_chip_init(struct smsc_softc *sc)
 		goto init_failed;
 	}
 
-	/* Adjust the packet offset in the buffer (designed to try and align IP
-	 * header on 4 byte boundary)
+	/*
+	 * Set the RX packet offset in the buffer to 0(no padding) and
+	 * discard errored ethernet frames.
 	 */
 	reg_val &= ~SMSC_HW_CFG_RXDOFF;
-	reg_val |= (ETHER_ALIGN << 9) & SMSC_HW_CFG_RXDOFF;
+	reg_val |= SMSC_HW_CFG_DRP;
 
 	/* The following setings are used for 'turbo mode', a.k.a multiple frames
 	 * per Rx transaction (again info taken form Linux driver).
@@ -1494,7 +1550,6 @@ smsc_ioctl(struct ifnet *ifp, u_long cmd, caddr_t
 	struct ifreq *ifr;
 	int rc;
 	int mask;
-	int reinit;
 
 	if (cmd == SIOCSIFCAP) {
 		sc = uether_getsc(ue);
@@ -1503,25 +1558,14 @@ smsc_ioctl(struct ifnet *ifp, u_long cmd, caddr_t
 		SMSC_LOCK(sc);
 
 		rc = 0;
-		reinit = 0;
 
 		mask = ifr->ifr_reqcap ^ ifp->if_capenable;
 
 		/* Modify the RX CSUM enable bits */
 		if ((mask & IFCAP_RXCSUM) != 0 &&
-		    (ifp->if_capabilities & IFCAP_RXCSUM) != 0) {
+		    (ifp->if_capabilities & IFCAP_RXCSUM) != 0)
 			ifp->if_capenable ^= IFCAP_RXCSUM;
-			
-			if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
-				ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
-				reinit = 1;
-			}
-		}
-		
 		SMSC_UNLOCK(sc);
-		if (reinit)
-			uether_init(ue);
-
 	} else {
 		rc = uether_ioctl(ifp, cmd, data);
 	}
Index: sys/dev/usb/net/if_smscreg.h
===================================================================
--- sys/dev/usb/net/if_smscreg.h	(revision 367712)
+++ sys/dev/usb/net/if_smscreg.h	(working copy)
@@ -116,6 +116,8 @@
 #define SMSC_RX_STAT_DRIBBLING           (0x1UL << 2)
 #define SMSC_RX_STAT_CRC_ERROR           (0x1UL << 1)
 
+#define	SMSC_RX_CSUM_LEN		2
+
 /**
  * REGISTERS
  *