Date: Sun, 22 Aug 2010 01:39:09 +0000 (UTC) From: Pyun YongHyeon <yongari@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r211596 - head/sys/dev/bge Message-ID: <201008220139.o7M1d9r8086345@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: yongari Date: Sun Aug 22 01:39:09 2010 New Revision: 211596 URL: http://svn.freebsd.org/changeset/base/211596 Log: It seems all Broadcom controllers have a bug that can generate UDP datagrams with checksum value 0 when TX UDP checksum offloading is enabled. Generating UDP checksum value 0 is RFC 768 violation. Even though the probability of generating such UDP datagrams is low, I don't want to see FreeBSD boxes to inject such datagrams into network so disable UDP checksum offloading by default. Users still override this behavior by setting a sysctl variable or loader tunable, dev.bge.%d.forced_udpcsum. I have no idea why this issue was not reported so far given that bge(4) is one of the most commonly used controller on high-end server class systems. Thanks to andre@ who passed the PR to me. PR: kern/104826 Modified: head/sys/dev/bge/if_bge.c head/sys/dev/bge/if_bgereg.h Modified: head/sys/dev/bge/if_bge.c ============================================================================== --- head/sys/dev/bge/if_bge.c Sun Aug 22 00:04:24 2010 (r211595) +++ head/sys/dev/bge/if_bge.c Sun Aug 22 01:39:09 2010 (r211596) @@ -120,7 +120,7 @@ __FBSDID("$FreeBSD$"); #include <dev/bge/if_bgereg.h> -#define BGE_CSUM_FEATURES (CSUM_IP | CSUM_TCP | CSUM_UDP) +#define BGE_CSUM_FEATURES (CSUM_IP | CSUM_TCP) #define ETHER_MIN_NOPAD (ETHER_MIN_LEN - ETHER_CRC_LEN) /* i.e., 60 */ MODULE_DEPEND(bge, pci, 1, 1, 1); @@ -2795,6 +2795,8 @@ bge_attach(device_t dev) goto fail; } + bge_add_sysctls(sc); + /* Set default tuneable values. */ sc->bge_stat_ticks = BGE_TICKS_PER_SEC; sc->bge_rx_coal_ticks = 150; @@ -2802,6 +2804,11 @@ bge_attach(device_t dev) sc->bge_rx_max_coal_bds = 10; sc->bge_tx_max_coal_bds = 10; + /* Initialize checksum features to use. */ + sc->bge_csum_features = BGE_CSUM_FEATURES; + if (sc->bge_forced_udpcsum != 0) + sc->bge_csum_features |= CSUM_UDP; + /* Set up ifnet structure */ ifp = sc->bge_ifp = if_alloc(IFT_ETHER); if (ifp == NULL) { @@ -2818,7 +2825,7 @@ bge_attach(device_t dev) ifp->if_snd.ifq_drv_maxlen = BGE_TX_RING_CNT - 1; IFQ_SET_MAXLEN(&ifp->if_snd, ifp->if_snd.ifq_drv_maxlen); IFQ_SET_READY(&ifp->if_snd); - ifp->if_hwassist = BGE_CSUM_FEATURES; + ifp->if_hwassist = sc->bge_csum_features; ifp->if_capabilities = IFCAP_HWCSUM | IFCAP_VLAN_HWTAGGING | IFCAP_VLAN_MTU; if ((sc->bge_flags & BGE_FLAG_TSO) != 0) { @@ -2975,8 +2982,6 @@ again: device_printf(sc->bge_dev, "couldn't set up irq\n"); } - bge_add_sysctls(sc); - return (0); fail: @@ -3960,7 +3965,7 @@ bge_encap(struct bge_softc *sc, struct m return (ENOBUFS); csum_flags |= BGE_TXBDFLAG_CPU_PRE_DMA | BGE_TXBDFLAG_CPU_POST_DMA; - } else if ((m->m_pkthdr.csum_flags & BGE_CSUM_FEATURES) != 0) { + } else if ((m->m_pkthdr.csum_flags & sc->bge_csum_features) != 0) { if (m->m_pkthdr.csum_flags & CSUM_IP) csum_flags |= BGE_TXBDFLAG_IP_CSUM; if (m->m_pkthdr.csum_flags & (CSUM_TCP | CSUM_UDP)) { @@ -4237,6 +4242,17 @@ bge_init_locked(struct bge_softc *sc) /* Program VLAN tag stripping. */ bge_setvlan(sc); + /* Override UDP checksum offloading. */ + if (sc->bge_forced_udpcsum == 0) + sc->bge_csum_features &= ~CSUM_UDP; + else + sc->bge_csum_features |= CSUM_UDP; + if (ifp->if_capabilities & IFCAP_TXCSUM && + ifp->if_capenable & IFCAP_TXCSUM) { + ifp->if_hwassist &= ~(BGE_CSUM_FEATURES | CSUM_UDP); + ifp->if_hwassist |= sc->bge_csum_features; + } + /* Init RX ring. */ if (bge_init_rx_ring_std(sc) != 0) { device_printf(sc->bge_dev, "no memory for std Rx buffers.\n"); @@ -4562,9 +4578,9 @@ bge_ioctl(struct ifnet *ifp, u_long comm ifp->if_capenable ^= IFCAP_HWCSUM; if (IFCAP_HWCSUM & ifp->if_capenable && IFCAP_HWCSUM & ifp->if_capabilities) - ifp->if_hwassist |= BGE_CSUM_FEATURES; + ifp->if_hwassist |= sc->bge_csum_features; else - ifp->if_hwassist &= ~BGE_CSUM_FEATURES; + ifp->if_hwassist &= ~sc->bge_csum_features; } if ((mask & IFCAP_TSO4) != 0 && @@ -4940,6 +4956,24 @@ bge_add_sysctls(struct bge_softc *sc) "Number of fragmented TX buffers of a frame allowed before " "forced collapsing"); + /* + * It seems all Broadcom controllers have a bug that can generate UDP + * datagrams with checksum value 0 when TX UDP checksum offloading is + * enabled. Generating UDP checksum value 0 is RFC 768 violation. + * Even though the probability of generating such UDP datagrams is + * low, I don't want to see FreeBSD boxes to inject such datagrams + * into network so disable UDP checksum offloading by default. Users + * still override this behavior by setting a sysctl variable, + * dev.bge.0.forced_udpcsum. + */ + sc->bge_forced_udpcsum = 0; + snprintf(tn, sizeof(tn), "dev.bge.%d.bge_forced_udpcsum", unit); + TUNABLE_INT_FETCH(tn, &sc->bge_forced_udpcsum); + SYSCTL_ADD_INT(ctx, children, OID_AUTO, "forced_udpcsum", + CTLFLAG_RW, &sc->bge_forced_udpcsum, 0, + "Enable UDP checksum offloading even if controller can " + "generate UDP checksum value 0"); + if (BGE_IS_5705_PLUS(sc)) return; Modified: head/sys/dev/bge/if_bgereg.h ============================================================================== --- head/sys/dev/bge/if_bgereg.h Sun Aug 22 00:04:24 2010 (r211595) +++ head/sys/dev/bge/if_bgereg.h Sun Aug 22 01:39:09 2010 (r211596) @@ -2644,6 +2644,8 @@ struct bge_softc { int bge_link_evt; /* pending link event */ int bge_timer; int bge_forced_collapse; + int bge_forced_udpcsum; + int bge_csum_features; struct callout bge_stat_ch; uint32_t bge_rx_discards; uint32_t bge_tx_discards;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201008220139.o7M1d9r8086345>