On Jan 26, 10:25pm, pcah8322@artax.karlin.mff.cuni.cz (Pavel Cahyna) wrote:
-- Subject: kern/32644: gsip(4) sends byte-swapped vlan tags

| >Number:         32644
| >Category:       kern
| >Synopsis:       gsip(4) sends byte-swapped vlan tags
| >Confidential:   no
| >Severity:       serious
| >Priority:       high
| >Responsible:    kern-bug-people
| >State:          open
| >Class:          sw-bug
| >Submitter-Id:   net
| >Arrival-Date:   Thu Jan 26 22:25:00 +0000 2006
| >Originator:     Pavel Cahyna
| >Release:        NetBSD 3.0
| >Organization:
| >Environment:
| System: NetBSD beta.imc.cas.cz 3.0 NetBSD 3.0 (GENERIC-RLPHY) #3: Thu Jan 26 16:18:14 CET 2006 cahyna@beta.imc.cas.cz:/usr/src/sys/arch/i386/compile/GENERIC-RLPHY i386
| Architecture: i386
| Machine: i386
| >Description:
| When using a tagged VLAN on a gsip(4) NIC, I discovered that the VLAN
| tag is sent byte-swapped. If I configure it as 4, I see 1024 on the
| wire. If I configure 1024, I see 4.  If I configure 2, I see 512.
| 
| See also http://mail-index.netbsd.org/tech-net/2006/01/24/0003.html
| and several following messages for attempts at analyzing the problem.
| >How-To-Repeat:
| Configure a vlan on a gsip interface, watch with tcpdump on another
| machine in the same Ethernet segment.
| >Fix:
| (tested on both i386 - little endian and sgimips - big endian)
| --- if_sip.c.~1.101.~	Sun Feb 27 01:27:33 2005
| +++ if_sip.c	Wed Jan 25 00:51:11 2006
| @@ -1354,10 +1354,20 @@
|  		 * This apparently has to be on the last descriptor of
|  		 * the packet.
|  		 */
| +
| +		/*
| +		 * Byte swapping is tricky. We need to provide the tag
| +		 * in a network byte order. On a big-endian machine,
| +		 * the byteorder is correct, but we need to swap it
| +		 * anyway, because this will be undone by the outside
| +		 * htole32(). That's why there must be an
| +		 * unconditional swap instead of htons() inside.
| +		 */
|  		if ((mtag = VLAN_OUTPUT_TAG(&sc->sc_ethercom, m0)) != NULL) {
|  			sc->sc_txdescs[lasttx].sipd_extsts |=
| -			    htole32(EXTSTS_VPKT |
| -				    (VLAN_TAG_VALUE(mtag) & EXTSTS_VTCI));
| +			    htole32(EXTSTS_VPKT | 
| +					(bswap16(VLAN_TAG_VALUE(mtag)) &
| +					 EXTSTS_VTCI));
|  		}
|  
|  		/*
| @@ -1969,8 +1979,19 @@
|  		 * If VLANs are enabled, VLAN packets have been unwrapped
|  		 * for us.  Associate the tag with the packet.
|  		 */
| +
| +		/*
| +		 * Again, byte swapping is tricky. Hardware provided
| +		 * the tag in the network byte order, but extsts was
| +		 * passed through le32toh() in the meantime. On a
| +		 * big-endian machine, we need to swap it again. On a
| +		 * little-endian machine, we need to convert from the
| +		 * network to host byte order. This means that we must
| +		 * swap it in any case, so unconditional swap instead
| +		 * of htons() is used.
| +		 */
|  		if ((extsts & EXTSTS_VPKT) != 0) {
| -			VLAN_INPUT_TAG(ifp, m, ntohs(extsts & EXTSTS_VTCI),
| +			VLAN_INPUT_TAG(ifp, m, bswap16(extsts & EXTSTS_VTCI),
|  			    continue);
|  		}
|  
-- End of excerpt from Pavel Cahyna


This does not seem right because in the first part of the patch you do:

+			    htole32(EXTSTS_VPKT | 
+					(bswap16(VLAN_TAG_VALUE(mtag)) &
+					 EXTSTS_VTCI));

and in the second:

| +			VLAN_INPUT_TAG(ifp, m, bswap16(extsts & EXTSTS_VTCI),

because in the first patch you are not bswapping16 EXTSTS_VTCI and in the
second you do?

christos