| >>> "Sam Erde" <samerde@eudoramail.com> wrote
|
| > On X1 netra, NetBSD 1.6 (-current) quickly (a couple of minutes after
| > running multi-user) crashes on a netra when used with a bunch of vlans
| > (more than 10).
|
| This is probably the probrem reported in kern/16648.
|
| According to my analysis, _bus_dmamap_load{,_mbuf}() in
| sparc64/machdep.c has a buffer overrun possibility. And dev/iommu.c
| has bound check issue. By these, corrupted memory causes a kernel
| panic.
|
| The point of this issue is as follows:
|
| - tulip.c allocates only one transmission segment (case of DM9102A).
| - _bus_dmamap_load_mbuf() uses MAX_DMA_SEGS for bound check instead
|   of map->_dm_segcnt.
| - iommu_dvmamap_load_raw() do not check with the map->_dm_segcnt.
|
| Please try the attached patch. This fix is very ugly, but my Netra
| X1 works fine.

Thanks for the patch.  There were a couple of issues with it.

First of all, _bus_dmamap_load() is only used by DMA devices that
are not going through an IOMMU.  Since I am unaware of any such
devices, it should never be called.  The fix you provided for it
was throwing away the low address bits of the DMA segment by
doing:

	vaddr = trunc_page(vaddr);

before calling pmap_extract().  I suppose if we want to fix
dead code we should try to fix it right.

Secondly, the segments created by _bus_dmamap_load_mbuf() are
only used to pass a list of KVAs to _bus_dmamap_load_raw().  
They are not directly used inside the dmamap.  
_bus_dmamap_load_raw() will then allocate a DVMA range and
repack them using DVMA addresses into the segs provided by
the dmamap.  This process may increase or decrease the number
of segments needed in the dmamap and we won't know until the
operation is complete.  Because of that, limiting the number
of segments used in _bus_dmamap_load_mbuf() to map->_dm_segcnt
may turn out to be counter productive if _bus_dmamap_load_raw()
manages to coalesce segments.

After looking at what you did to _bus_dmamap_load_mbuf() I 
cannot see anything that would change it's behavior.  Other
than limiting the numver of segments used to map->_dm_segcnt
it should result in the exact same data layout as the old
version.

Here's another patch that should fix the problem in
_bus_dmamap_load() (we won't really know unless something
uses it) and has the old version of _bus_dmamap_load_mbuf().

I'd appreciate it if someone with an X1 would test it and
tell me the results.

Eduardo


Index: sys/arch/sparc64/dev/iommu.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/sparc64/dev/iommu.c,v
retrieving revision 1.52
diff -u -r1.52 iommu.c
--- sys/arch/sparc64/dev/iommu.c	2002/06/02 14:44:41	1.52
+++ sys/arch/sparc64/dev/iommu.c	2002/06/11 19:48:32
@@ -536,7 +536,7 @@
 			map->dm_segs[seg].ds_len));
 		map->dm_segs[seg].ds_len =
 		    boundary - (sgstart & (boundary - 1));
-		if (++seg > map->_dm_segcnt) {
+		if (++seg >= map->_dm_segcnt) {
 			/* Too many segments.  Fail the operation. */
 			DPRINTF(IDB_INFO, ("iommu_dvmamap_load: "
 				"too many segments %d\n", seg));
@@ -802,12 +802,12 @@
 				(sgend & ~(boundary - 1))) {
 				/* Need a new segment. */
 				map->dm_segs[j].ds_len =
-					sgstart & (boundary - 1);
+					boundary - (sgstart & (boundary - 1));
 				DPRINTF(IDB_INFO, ("iommu_dvmamap_load_raw: "
 					"seg %d start %lx size %lx\n", j,
 					(long)map->dm_segs[j].ds_addr, 
 					map->dm_segs[j].ds_len));
-				if (++j > map->_dm_segcnt) {
+				if (++j >= map->_dm_segcnt) {
 					iommu_dvmamap_unload(t, is, map);
 					return (E2BIG);
 				}
@@ -873,12 +873,12 @@
 	map->dm_segs[i].ds_addr = sgstart;
 	while ((sgstart & ~(boundary - 1)) != (sgend & ~(boundary - 1))) {
 		/* Oops.  We crossed a boundary.  Split the xfer. */
-		map->dm_segs[i].ds_len = sgstart & (boundary - 1);
+		map->dm_segs[i].ds_len = boundary - (sgstart & (boundary - 1));
 		DPRINTF(IDB_INFO, ("iommu_dvmamap_load_raw: "
 			"seg %d start %lx size %lx\n", i,
 			(long)map->dm_segs[i].ds_addr,
 			map->dm_segs[i].ds_len));
-		if (++i > map->_dm_segcnt) {
+		if (++i >= map->_dm_segcnt) {
 			/* Too many segments.  Fail the operation. */
 			s = splhigh();
 			/* How can this fail?  And if it does what can we do? */
Index: sys/arch/sparc64/sparc64/machdep.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/sparc64/sparc64/machdep.c,v
retrieving revision 1.121
diff -u -r1.121 machdep.c
--- sys/arch/sparc64/sparc64/machdep.c	2002/06/04 14:48:09	1.121
+++ sys/arch/sparc64/sparc64/machdep.c	2002/06/11 19:48:33
@@ -1112,7 +1112,8 @@
 	bus_dma_tag_t t;
 	bus_dmamap_t map;
 {
-
+	if (map->dm_nsegs)
+		bus_dmamap_unload(t, map);
 	free(map, M_DMAMAP);
 }
 
@@ -1165,24 +1166,26 @@
 	i = 0;
 	map->dm_segs[i].ds_addr = NULL;
 	map->dm_segs[i].ds_len = 0;
-	while (sgsize > 0 && i < map->_dm_segcnt) {
+	while (sgsize > 0) {
 		paddr_t pa;
 
 		(void) pmap_extract(pmap_kernel(), vaddr, &pa);
-		sgsize -= NBPG;
-		vaddr += NBPG;
+		sgsize -= (NBPG - (vaddr & PGOFSET));
+		vaddr = (vaddr + NBPG) & ~PGOFSET;
 		if (map->dm_segs[i].ds_len == 0)
 			map->dm_segs[i].ds_addr = pa;
 		if (pa == (map->dm_segs[i].ds_addr + map->dm_segs[i].ds_len)
-		    && ((map->dm_segs[i].ds_len + NBPG) < map->_dm_maxsegsz)) {
+		    && ((map->dm_segs[i].ds_len + NBPG) <= map->_dm_maxsegsz)) {
 			/* Hey, waddyaknow, they're contiguous */
 			map->dm_segs[i].ds_len += NBPG;
 			continue;
 		}
-		map->dm_segs[++i].ds_addr = pa;
+		if (i++ > map->_dm_segcnt)
+			return (E2BIG);
+		map->dm_segs[i].ds_addr = pa;
 		map->dm_segs[i].ds_len = NBPG;
 	}
-	map->dm_nsegs = i;
+	map->dm_nsegs = i + 1;
 	/* Mapping is bus dependent */
 	return (0);
 }
@@ -1197,7 +1200,6 @@
 	struct mbuf *m;
 	int flags;
 {
-#if 1
 	bus_dma_segment_t segs[MAX_DMA_SEGS];
 	int i;
 	size_t len;
@@ -1217,15 +1219,17 @@
 			paddr_t pa;
 			long incr;
 
-			incr = NBPG - (vaddr&PGOFSET);
+			incr = NBPG - (vaddr & PGOFSET);
 			incr = min(buflen, incr);
 
 			(void) pmap_extract(pmap_kernel(), vaddr, &pa);
 			buflen -= incr;
 			vaddr += incr;
 
-			if (i > 0 && pa == (segs[i-1].ds_addr + segs[i-1].ds_len)
-			    && ((segs[i-1].ds_len + incr) < map->_dm_maxsegsz)) {
+			if (i > 0 && 
+				pa == (segs[i-1].ds_addr + segs[i-1].ds_len) &&
+				((segs[i-1].ds_len + incr) <= 
+					map->_dm_maxsegsz)) {
 				/* Hey, waddyaknow, they're contiguous */
 				segs[i-1].ds_len += incr;
 				continue;
@@ -1285,13 +1289,8 @@
 		}
 		return (retval);
 	}
-#endif
-	return (bus_dmamap_load_raw(t, map, segs, i,
-			    (bus_size_t)len, flags));
-#else
-	panic("_bus_dmamap_load_mbuf: not implemented");
-	return 0;
 #endif
+	return (bus_dmamap_load_raw(t, map, segs, i, (bus_size_t)len, flags));
 }
 
 /*
@@ -1365,7 +1364,7 @@
 
 
 			if (i > 0 && pa == (segs[i-1].ds_addr + segs[i-1].ds_len)
-			    && ((segs[i-1].ds_len + incr) < map->_dm_maxsegsz)) {
+			    && ((segs[i-1].ds_len + incr) <= map->_dm_maxsegsz)) {
 				/* Hey, waddyaknow, they're contiguous */
 				segs[i-1].ds_len += incr;
 				continue;