--PEIAKu/WMn1b1Hv9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

when Perry asked for NETISO testing code, I wrote some, and found bugs.
Basically, when ether_input() calls (indirectly) clnp_input(), it does
horrible things. 

Namely, it does an overlapping struct assign (on a memory area
formerly m_adj'ed away!) which might have worked with ancient
compilers, but creates address corruption now. (== netbsd-3-1).

All  to get rid of the intermediate 3-byte 802.x llc field.

As the clnp_input is aware of being called by ethernet anyway (and
sort of needs to), I decided to just give it the full ethernet
header with llc, and let it throw it away itself (it does an m_adj
itself, so this is for free). Saving in the ISO networking path:
two memcpy if done right, one memmove if the current code was 
trivially repaired.

I suspect that any modern compiler would integrate one additional
compare-to-constant and and conditional branch (which are executed 
additionally for the DIX protocols) into the big DIX switch, so no
cost created there, or else, say, 2 instruction cycles (== about one
nanosecond for modern machines).

if_fddisubr and if_tokensubr will need similar patches; at least for
fddi this won't create a penalty because it hasn't any DIX type
field, so there's a convenient switch case already to move the
m_adj() to.

Any comments?

(Code is tested for AF_INET, AF_INET6 and AF_ISO).

Regards,
	-is


--PEIAKu/WMn1b1Hv9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=ether_diff_netiso

Index: sys/netiso/clnp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netiso/clnp_input.c,v
retrieving revision 1.30
diff -u -r1.30 clnp_input.c
--- sys/netiso/clnp_input.c	11 Dec 2005 12:25:12 -0000	1.30
+++ sys/netiso/clnp_input.c	1 Dec 2006 13:38:52 -0000
@@ -199,9 +199,7 @@
 	case IFT_ETHER:
 		bcopy((caddr_t) (mtod(m, struct ether_header *)->ether_dhost),
 		  (caddr_t) sh.snh_dhost, 2 * sizeof(sh.snh_dhost));
-		m->m_data += sizeof(struct ether_header);
-		m->m_len -= sizeof(struct ether_header);
-		m->m_pkthdr.len -= sizeof(struct ether_header);
+		m_adj(m, sizeof(struct ether_header) + 3);
 		break;
 	case IFT_FDDI:
 		bcopy((caddr_t) (mtod(m, struct fddi_header *)->fddi_dhost),
Index: sys/net/if_ethersubr.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_ethersubr.c,v
retrieving revision 1.138
diff -u -r1.138 if_ethersubr.c
--- sys/net/if_ethersubr.c	24 Nov 2006 01:04:30 -0000	1.138
+++ sys/net/if_ethersubr.c	1 Dec 2006 13:38:52 -0000
@@ -859,62 +859,65 @@
 		}
 	}
 
-	/* Strip off the Ethernet header. */
-	m_adj(m, sizeof(struct ether_header));
-
 	/* If the CRC is still on the packet, trim it off. */
 	if (m->m_flags & M_HASFCS) {
 		m_adj(m, -ETHER_CRC_LEN);
 		m->m_flags &= ~M_HASFCS;
 	}
 
-	switch (etype) {
+	if (etype > ETHERMTU + sizeof (struct ether_header)) {
+		/* Strip off the Ethernet header. */
+		m_adj(m, sizeof(struct ether_header));
+
+		switch (etype) {
 #ifdef INET
-	case ETHERTYPE_IP:
+		case ETHERTYPE_IP:
 #ifdef GATEWAY
-		if (ipflow_fastforward(m))
-			return;
+			if (ipflow_fastforward(m))
+				return;
 #endif
-		schednetisr(NETISR_IP);
-		inq = &ipintrq;
-		break;
+			schednetisr(NETISR_IP);
+			inq = &ipintrq;
+			break;
 
-	case ETHERTYPE_ARP:
-		schednetisr(NETISR_ARP);
-		inq = &arpintrq;
-		break;
+		case ETHERTYPE_ARP:
+			schednetisr(NETISR_ARP);
+			inq = &arpintrq;
+			break;
 
-	case ETHERTYPE_REVARP:
-		revarpinput(m);	/* XXX queue? */
-		return;
+		case ETHERTYPE_REVARP:
+			revarpinput(m);	/* XXX queue? */
+			return;
 #endif
 #ifdef INET6
-	case ETHERTYPE_IPV6:
-		schednetisr(NETISR_IPV6);
-		inq = &ip6intrq;
-		break;
+		case ETHERTYPE_IPV6:
+			schednetisr(NETISR_IPV6);
+			inq = &ip6intrq;
+			break;
 #endif
 #ifdef IPX
-	case ETHERTYPE_IPX:
-		schednetisr(NETISR_IPX);
-		inq = &ipxintrq;
-		break;
+		case ETHERTYPE_IPX:
+			schednetisr(NETISR_IPX);
+			inq = &ipxintrq;
+			break;
 #endif
 #ifdef NETATALK
-        case ETHERTYPE_ATALK:
-                schednetisr(NETISR_ATALK);
-                inq = &atintrq1;
-                break;
-        case ETHERTYPE_AARP:
-		/* probably this should be done with a NETISR as well */
-                aarpinput(ifp, m); /* XXX */
-                return;
+        	case ETHERTYPE_ATALK:
+                	schednetisr(NETISR_ATALK);
+                	inq = &atintrq1;
+                	break;
+        	case ETHERTYPE_AARP:
+			/* probably this should be done with a NETISR as well */
+                	aarpinput(ifp, m); /* XXX */
+                	return;
 #endif /* NETATALK */
-	default:
+		default:
+			m_freem(m);
+			return;
+		}
+	} else {
 #if defined (ISO) || defined (LLC) || defined (NETATALK)
-		if (etype > ETHERMTU)
-			goto dropanyway;
-		l = mtod(m, struct llc *);
+		l = (struct llc *)(eh+1);
 		switch (l->llc_dsap) {
 #ifdef NETATALK
 		case LLC_SNAP_LSAP:
@@ -929,7 +932,8 @@
 				    ntohs(l->llc_snap_ether_type) ==
 				    ETHERTYPE_ATALK) {
 					inq = &atintrq2;
-					m_adj(m, sizeof(struct llc));
+					m_adj(m, sizeof(struct ether_header)
+					    + sizeof(struct llc));
 					schednetisr(NETISR_ATALK);
 					break;
 				}
@@ -939,7 +943,8 @@
 				    sizeof(aarp_org_code)) == 0 &&
 				    ntohs(l->llc_snap_ether_type) ==
 				    ETHERTYPE_AARP) {
-					m_adj( m, sizeof(struct llc));
+					m_adj( m, sizeof(struct ether_header)
+					    + sizeof(struct llc));
 					aarpinput(ifp, m); /* XXX */
 				    return;
 				}
@@ -954,18 +959,13 @@
 			switch (l->llc_control) {
 			case LLC_UI:
 				/* LLC_UI_P forbidden in class 1 service */
-				if ((l->llc_dsap == LLC_ISO_LSAP) &&
+				if ((l->llc_dsap == LLC_ISO_LSAP) &&	/* XXX? case tested */
 				    (l->llc_ssap == LLC_ISO_LSAP)) {
 					/* LSAP for ISO */
-					if (m->m_pkthdr.len > etype)
+					/* XXX length computation?? */
+					if (m->m_pkthdr.len > etype + sizeof(struct ether_header))
 						m_adj(m, etype - m->m_pkthdr.len);
-					m->m_data += 3;		/* XXX */
-					m->m_len -= 3;		/* XXX */
-					m->m_pkthdr.len -= 3;	/* XXX */
-					M_PREPEND(m, sizeof *eh, M_DONTWAIT);
-					if (m == 0)
-						return;
-					*mtod(m, struct ether_header *) = *eh;
+					
 #ifdef ARGO_DEBUG
 					if (argo_debug[D_ETHER])
 						printf("clnp packet");
@@ -978,7 +978,8 @@
 
 			case LLC_XID:
 			case LLC_XID_P:
-				if(m->m_len < 6)
+				if(m->m_len < 6 + sizeof(struct ether_header))
+					/* XXX m_pullup? */
 					goto dropanyway;
 				l->llc_window = 0;
 				l->llc_fid = 9;
@@ -995,6 +996,8 @@
 
 				l->llc_dsap = l->llc_ssap;
 				l->llc_ssap = c;
+				m_adj(m, sizeof(struct ether_header));
+				/* XXX we can optimize here? */
 				if (m->m_flags & (M_BCAST | M_MCAST))
 					bcopy(LLADDR(ifp->if_sadl),
 					      (caddr_t)eh->ether_dhost, 6);
@@ -1023,9 +1026,9 @@
 			m_freem(m);
 			return;
 		}
-#else /* ISO || LLC  || NETATALK*/
-	    m_freem(m);
-	    return;
+#else /* ISO || LLC || NETATALK*/
+		m_freem(m);
+		return;
 #endif /* ISO || LLC || NETATALK*/
 	}
 

--PEIAKu/WMn1b1Hv9--