Index | Thread | Search

From:
Kirill A. Korinsky <kirill@korins.ky>
Subject:
Re: octeon: fix cnmac corrupting VLAN packets transmitted by veb
To:
Visa Hankala <visa@hankala.org>
Cc:
sergii@rudchenko.nl, tech@openbsd.org
Date:
Tue, 14 Jul 2026 14:17:21 +0200

Download raw body.

Thread
On Tue, 14 Jul 2026 13:52:44 +0200,
Visa Hankala <visa@hankala.org> wrote:
> 
> On Tue, Jul 14, 2026 at 12:52:22PM +0200, Kirill A. Korinsky wrote:
> > On Mon, 13 Jul 2026 23:30:50 +0200,
> > Sergii Rudchenko <sergii@rudchenko.nl> wrote:
> > > 
> > > On an EdgeRouter 6P, cnmac(4) corrupts VLAN tagged TCP and UDP packets
> > > transmitted by veb(4).
> > > 
> > > It happens because the TCP/UDP checksum offloading setup in cnmac(4) assumes
> > > that IP packet starts right after ethernet header:
> > > 
> > >        ipoffp1 = (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT |
> > > M_UDP_CSUM_OUT))
> > >            ? (ETHER_HDR_LEN + 1) : 0;
> > > 
> > > For VLAN-encapsulated packets it produces a wrong checksum written by a
> > > wrong
> > > offset, resulting in corrupt TCP and UDP packets.
> > > 
> > > vlan(4) are not affected by this problem because unline veb(4) it does not
> > > declare IFCAP_CSUM_* capabilities and all checksums are calculated
> > > before the
> > > packet reaches cnmac(4).
> > > 
> > > 
> > > Test setup (on the EdgeRouter):
> > > 
> > > # ifconfig vport0 inet 192.168.0.1/24
> > > # ifconfig veb0 add cnmac1 tagged cnmac1 1 -untagged cnmac1
> > > # ifconfig veb0 add vport0 tagged vport0 1 untagged vport0 1 up
> > > # ifconfig cnmac1 up
> > > # nc -l 8000
> > > 
> > > On a peer machine with 192.168.0.2/24 assigned to a VLAN trunk interface
> > > connected to EdgeRouter:
> > > 
> > > $ echo Test | nc 192.168.0.1 8000
> > > 
> > > 
> > > Expected result: nc(1) on the client completes immediately and nc(1) the
> > > EdgeRouter prints "Test" and completes.
> > > 
> > > Actual result: nc(1) on the client hangs, nc(1) on on the EdgeRouter
> > > does not
> > > print anything. Examining the traffic on the client with Wireshark shows
> > > that
> > > TCP SYN,SYN_ACK packets from EdgeRouter have an invalid TCP option.
> > > Comparing
> > > to tcpdump on cnmac1 reveals two altered bytes at offsets 0x46 and 0x47.
> > > 
> > > 
> > 
> > Thanks for detailed use case and explanation of the bug!
> > 
> > 
> > > @@ -881,8 +884,19 @@ cnmac_send_makecmd(struct cnmac_softc *s
> > >         }
> > > 
> > >         /* Get the IP packet offset for TCP/UDP checksum offloading. */
> > > -       ipoffp1 = (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT |
> > > M_UDP_CSUM_OUT))
> > > -           ? (ETHER_HDR_LEN + 1) : 0;
> > > +       if (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT | M_UDP_CSUM_OUT)) {
> > > +               eh = mtod(m, struct ether_header *);
> > > +               ether_type = ntohs(eh->ether_type);
> > > +
> > > +               if (ether_type == ETHERTYPE_IP) {
> > > +                       ipoffp1 = ETHER_HDR_LEN + 1;
> > > +               } else if (ether_type == ETHERTYPE_VLAN) {
> > > +                       evh = mtod(m, struct ether_vlan_header*);
> > > +                       if (ntohs(evh->evl_proto) == ETHERTYPE_IP) {
> > > +                               ipoffp1 = sizeof(*evh) + 1;
> > > +                       }
> > > +               }
> > > +       }
> > > 
> > 
> > I think the issue is not cnmac related, this code reads sane for me.
> > 
> > My take that issue in ether_offload_ifcap()
> > 
> > Somethink like that, I think:
> > 
> > Index: sys/net/if_ethersubr.c
> > ===================================================================
> > RCS file: /home/cvs/src/sys/net/if_ethersubr.c,v
> > diff -u -p -r1.308 if_ethersubr.c
> > --- sys/net/if_ethersubr.c	19 Dec 2025 02:04:13 -0000	1.308
> > +++ sys/net/if_ethersubr.c	14 Jul 2026 10:47:15 -0000
> > @@ -1267,6 +1267,7 @@ struct mbuf *
> >  ether_offload_ifcap(struct ifnet *ifp, struct mbuf *m)
> >  {
> >  	struct ether_extracted ext;
> > +	struct ifnet *csum_ifp = ifp;
> >  	int csum = 0;
> >  
> >  #if NVLAN > 0
> > @@ -1274,8 +1275,11 @@ ether_offload_ifcap(struct ifnet *ifp, s
> >  	    !ISSET(ifp->if_capabilities, IFCAP_VLAN_HWTAGGING)) {
> >  		/*
> >  		 * If the underlying interface has no VLAN hardware tagging
> > -		 * support, inject one in software.
> > +		 * support, inject one in software. Calculate checksums in
> > +		 * software unless it supports offloading with an inline tag.
> >  		 */
> > +		if (!ISSET(ifp->if_capabilities, IFCAP_VLAN_HWOFFLOAD))
> > +			csum_ifp = NULL;
> >  		m = vlan_inject(m, ETHERTYPE_VLAN, m->m_pkthdr.ether_vtag);
> >  		if (m == NULL)
> >  			return (NULL);
> > @@ -1283,16 +1287,19 @@ ether_offload_ifcap(struct ifnet *ifp, s
> >  #endif
> >  
> >  	if (ISSET(m->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT) &&
> > -	    !ISSET(ifp->if_capabilities, IFCAP_CSUM_IPv4))
> > +	    (csum_ifp == NULL ||
> > +	     !ISSET(ifp->if_capabilities, IFCAP_CSUM_IPv4)))
> >  		csum = 1;
> >  
> >  	if (ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) &&
> > -	    (!ISSET(ifp->if_capabilities, IFCAP_CSUM_TCPv4) ||
> > +	    (csum_ifp == NULL ||
> > +	     !ISSET(ifp->if_capabilities, IFCAP_CSUM_TCPv4) ||
> >  	     !ISSET(ifp->if_capabilities, IFCAP_CSUM_TCPv6)))
> >  		csum = 1;
> >  
> >  	if (ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT) &&
> > -	    (!ISSET(ifp->if_capabilities, IFCAP_CSUM_UDPv4) ||
> > +	    (csum_ifp == NULL ||
> > +	     !ISSET(ifp->if_capabilities, IFCAP_CSUM_UDPv4) ||
> >  	     !ISSET(ifp->if_capabilities, IFCAP_CSUM_UDPv6)))
> >  		csum = 1;
> >  
> > @@ -1320,11 +1327,11 @@ ether_offload_ifcap(struct ifnet *ifp, s
> >  		m->m_pkthdr.len -= ethlen;
> >  
> >  		if (ext.ip4) {
> > -			in_hdr_cksum_out(m, ifp);
> > -			in_proto_cksum_out(m, ifp);
> > +			in_hdr_cksum_out(m, csum_ifp);
> > +			in_proto_cksum_out(m, csum_ifp);
> >  #ifdef INET6
> >  		} else if (ext.ip6) {
> > -			in6_proto_cksum_out(m, ifp);
> > +			in6_proto_cksum_out(m, csum_ifp);
> >  #endif
> >  		}
> >  
> 
> It is relatively common that a NIC can compute the TCP/UDP checksum
> in the presence of a VLAN header even if the NIC did not support VLAN
> tag insertion.
> 
> I think the initial patch could be improved a little by using
> ether_extract_headers(). This avoids adding another custom packet
> peeker, and adds IPv6 handling.
>

Indeed, your version is clean cnmac specific, I like it, FWIW OK kirill@

But I still think we need a global fix of ether_offload_ifcap()

> Index: arch/octeon/dev/if_cnmac.c
> ===================================================================
> RCS file: src/sys/arch/octeon/dev/if_cnmac.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 if_cnmac.c
> --- arch/octeon/dev/if_cnmac.c
> +++ arch/octeon/dev/if_cnmac.c
> @@ -868,8 +868,9 @@ int
>  cnmac_send_makecmd(struct cnmac_softc *sc, struct mbuf *m,
>      uint64_t *gbuf, uint64_t *rpko_cmd_w0, uint64_t *rpko_cmd_w1)
>  {
> +	struct ether_extracted ext;
>  	uint64_t pko_cmd_w0, pko_cmd_w1;
> -	int ipoffp1;
> +	int ipoffp1 = 0;
>  	int segs;
>  	int result = 0;
>  
> @@ -881,8 +882,16 @@ cnmac_send_makecmd(struct cnmac_softc *s
>  	}
>  
>  	/* Get the IP packet offset for TCP/UDP checksum offloading. */
> -	ipoffp1 = (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT | M_UDP_CSUM_OUT))
> -	    ? (ETHER_HDR_LEN + 1) : 0;
> +	if (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT | M_UDP_CSUM_OUT)) {
> +		ether_extract_headers(m, &ext);
> +
> +		if (ext.tcp || ext.udp) {
> +			if (ext.evh)
> +				ipoffp1 = sizeof(*ext.evh) + 1;
> +			else
> +				ipoffp1 = sizeof(*ext.eh) + 1;
> +		}
> +	}
>  
>  	/*
>  	 * segs == 1	-> link mode (single continuous buffer)

-- 
wbr, Kirill