Date: Mon, 16 Nov 2020 10:19:10 +0900 From: YongHyeon PYUN <pyunyh@gmail.com> To: Carsten =?iso-8859-1?Q?B=E4cker?= <carbaecker@gmx.de> Cc: Hans Petter Selasky <hps@selasky.org>, Kristof Provost <kp@FreeBSD.org>, freebsd-arm@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: Problem with checksum offloading on RPi3 (PF + Jails involved) Message-ID: <20201116011910.GB1941@michelle> In-Reply-To: <e43b42e0-80fe-847a-f1bc-025b6914f98a@gmx.de> References: <748edc3d-4ef7-c4de-291f-7c0b460a6052@gmx.de> <D8CE4762-4D94-47C7-A8D1-6C537766813B@FreeBSD.org> <5130ee46-5832-d4df-d774-c6bd32e10b30@gmx.de> <A3890336-BE8F-438C-8C3E-7B21FB729FCA@FreeBSD.org> <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> <e43b42e0-80fe-847a-f1bc-025b6914f98a@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <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
*
--liOOAslEiF7prFVr--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20201116011910.GB1941>