From owner-freebsd-arm@freebsd.org Mon Nov 16 01:19:15 2020 Return-Path: Delivered-To: freebsd-arm@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id C07CC46EBFC; Mon, 16 Nov 2020 01:19:15 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CZB766d8Cz3wDQ; Mon, 16 Nov 2020 01:19:14 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: by mail-pf1-x430.google.com with SMTP id v12so12186151pfm.13; Sun, 15 Nov 2020 17:19:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=WiEZNCemjnGzDHXIJU/a12aM+W1IXQEW2KOU1vAa0H0=; b=Sgrmaa9DpENwwpu/fIPxpzEzozT6bhBhD9UMLqUZKUdEYNJO5r3z9mO39VjbwXPgwS AIjqKUAdF/+T2Tk5HWsH1xyk3slbIptu9dzgo6p9I1+y51YAS48+UcxkGl9X+eFmYnzy OOz2eHwVr+XIPnixBhmbgAJp7I/CjOl33p3l/8Y3Y2u8qodrqEEYCwA/dqaqsNprcU+i fy5p2u0c9KHGTEqm4esGHljMmSnKfwXffxa7yvyld9u/jqT6BUeBDaeZ7FrcVLkFIsND FQuXpCknLwLmiewVUX7XGBxlcNkUwm3dzkzf6MndaWB6w32zR+kCvtRxtfOwNECvdMph GBAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=WiEZNCemjnGzDHXIJU/a12aM+W1IXQEW2KOU1vAa0H0=; b=CwUWZbUqznPjMkqQtYlyps/lI77TcAw86vsxdxz94CyN5sQU6Z8khdiDxSKrKbwv48 DhomICtvIeMwvsj7pAfsnRxKh9JHqZSBn0ndeOeEt0lenI1mY4c+5E0rY3hyHy1C6Uil 9zAUfca+GpyZOGJdPI7Hk+d/l28Dw7ipt/+TtpFTnTVJ2gQCBAcvuVszylXPNoAn3yPs xbfzaklmV3i+4BIF58XSEvUjK3SmNGQYcu3er7labE7bERhW+u5kpdQTYbgsomsE8tNd caf6z6y2hJMqMmQ8Nkpkjttz5Nxmleysw37i2CnfYogHEeMJLh0or58wJVbFqLgEoEMn Befg== X-Gm-Message-State: AOAM533CvAjN1Q49Low1gGNz31zbssoOFDSsDEflh+zBTTNPSW9VPmjl HFaxIK8qzIP8DFf9uHhs1CGT1NJXT9A= X-Google-Smtp-Source: ABdhPJxaP5JYGzQ40SFqDUFiDSglnIEmQSWXWRhKtOvhHbCWcvU9ONYZBN5aC5k6gsOq1ZzN+TUl7w== X-Received: by 2002:aa7:8281:0:b029:164:cc0f:2b3a with SMTP id s1-20020aa782810000b0290164cc0f2b3amr12218294pfm.30.1605489553631; Sun, 15 Nov 2020 17:19:13 -0800 (PST) Received: from localhost ([210.108.244.139]) by smtp.gmail.com with ESMTPSA id a11sm16417659pfn.125.2020.11.15.17.19.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 15 Nov 2020 17:19:12 -0800 (PST) Date: Mon, 16 Nov 2020 10:19:10 +0900 From: YongHyeon PYUN To: Carsten =?iso-8859-1?Q?B=E4cker?= Cc: Hans Petter Selasky , Kristof Provost , freebsd-arm@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: Problem with checksum offloading on RPi3 (PF + Jails involved) Message-ID: <20201116011910.GB1941@michelle> References: <748edc3d-4ef7-c4de-291f-7c0b460a6052@gmx.de> <5130ee46-5832-d4df-d774-c6bd32e10b30@gmx.de> <20201029213622.GM31099@funkthat.com> <55713894-A896-4F12-ABB9-93DFEB2F16B9@FreeBSD.org> <20201103045215.GA2524@michelle> <46d08198-530c-cb4b-efa8-4edaf89471c1@selasky.org> <4dfaa9a3-c085-8466-a6e4-19f988b5ed3d@selasky.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="liOOAslEiF7prFVr" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Queue-Id: 4CZB766d8Cz3wDQ X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=Sgrmaa9D; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of pyunyh@gmail.com designates 2607:f8b0:4864:20::430 as permitted sender) smtp.mailfrom=pyunyh@gmail.com X-Spamd-Result: default: False [-3.50 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36:c]; FREEMAIL_FROM(0.00)[gmail.com]; HAS_ATTACHMENT(0.00)[]; RCPT_COUNT_FIVE(0.00)[5]; RCVD_COUNT_THREE(0.00)[3]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; NEURAL_HAM_SHORT(-1.00)[-1.000]; FREEMAIL_TO(0.00)[gmx.de]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:+]; RBL_DBL_DONT_QUERY_IPS(0.00)[2607:f8b0:4864:20::430:from]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; MIME_GOOD(-0.10)[multipart/mixed,text/plain,text/x-diff]; SPAMHAUS_ZRD(0.00)[2607:f8b0:4864:20::430:from:127.0.2.255]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::430:from]; MID_RHS_NOT_FQDN(0.50)[]; RCVD_TLS_ALL(0.00)[]; MAILMAN_DEST(0.00)[freebsd-arm,freebsd-hackers] X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Nov 2020 01:19:15 -0000 --liOOAslEiF7prFVr Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Wed, Nov 04, 2020 at 07:36:44AM +0100, Carsten Bäcker wrote: [...] > Hi, > > i applied the patch on -CURRENT but got a panic right after loading the > kernel. Most likely an unrelated problem. > > But i was able to apply the patch on releng/12.2 (with an offset). > Unfortunately it doesn't change the previously described behavior with > rxcsum and i didn't manage to get any reasonable debug-output. > > Since i can easily reproduce the problem. How else can i help? > Finally had time to read the LAN89530 data sheet. The data sheet still does not clear on several cases and it requires real H/W to experiment for various cases. I created a patch which adds more RX validation but it was not tested at all due to lack of H/W. Also I even don't know whether it works or not after this change. When it does not work it would be good to know debug out of smsc(4). --liOOAslEiF7prFVr Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="smsc.rx.patch" 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 #include +#include +#include #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 * --liOOAslEiF7prFVr--