Subject: Re: net troubles
To: None <samerde@eudoramail.com>
From: Takeshi Nakayama <tn@catvmics.ne.jp>
List: port-sparc64
Date: 06/11/2002 13:11:44
----Next_Part(Tue_Jun_11_12:58:47_2002_229)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
>>> "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.
-- Takeshi Nakayama
----Next_Part(Tue_Jun_11_12:58:47_2002_229)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="netra-x1.fix.diff"
Index: sys/arch/sparc64/dev/iommu.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/sparc64/dev/iommu.c,v
retrieving revision 1.51
diff -u -d -r1.51 iommu.c
--- iommu.c 2002/05/13 21:01:15 1.51
+++ iommu.c 2002/06/11 02:19:44
@@ -537,7 +553,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));
@@ -803,12 +819,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);
}
@@ -874,12 +890,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.119.6.1
diff -u -d -r1.119.6.1 machdep.c
--- machdep.c 2002/06/05 04:09:19 1.119.6.1
+++ machdep.c 2002/06/11 02:20:07
@@ -1157,6 +1157,7 @@
}
sgsize = round_page(buflen + ((int)vaddr & PGOFSET));
+ vaddr = trunc_page(vaddr);
/*
* We always use just one segment.
@@ -1165,7 +1166,7 @@
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);
@@ -1174,15 +1175,17 @@
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 (EFBIG); /* XXX better return value here? */
+ 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);
}
@@ -1201,6 +1204,7 @@
bus_dma_segment_t segs[MAX_DMA_SEGS];
int i;
size_t len;
+ int max_dma_segs = min(MAX_DMA_SEGS, map->_dm_segcnt);
/* Record mbuf for *_unload */
map->_dm_type = _DM_TYPE_MBUF;
@@ -1208,12 +1212,17 @@
i = 0;
len = 0;
+ segs[i].ds_addr = NULL;
+ segs[i].ds_len = 0;
+ segs[i]._ds_boundary = 0;
+ segs[i]._ds_align = 0;
+ segs[i]._ds_mlist = NULL;
while (m) {
vaddr_t vaddr = mtod(m, vaddr_t);
long buflen = (long)m->m_len;
len += buflen;
- while (buflen > 0 && i < MAX_DMA_SEGS) {
+ while (buflen > 0) {
paddr_t pa;
long incr;
@@ -1224,26 +1233,27 @@
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 (segs[i].ds_len == 0)
+ segs[i].ds_addr = pa;
+ if (pa == (segs[i].ds_addr + segs[i].ds_len)
+ && ((segs[i].ds_len + incr) <= map->_dm_maxsegsz)) {
/* Hey, waddyaknow, they're contiguous */
- segs[i-1].ds_len += incr;
+ segs[i].ds_len += incr;
continue;
}
+ if (++i >= max_dma_segs) {
+ /* Exceeded the size of our dmamap */
+ map->_dm_type = 0;
+ map->_dm_source = NULL;
+ return E2BIG;
+ }
segs[i].ds_addr = pa;
segs[i].ds_len = incr;
segs[i]._ds_boundary = 0;
segs[i]._ds_align = 0;
segs[i]._ds_mlist = NULL;
- i++;
}
m = m->m_next;
- if (m && i >= MAX_DMA_SEGS) {
- /* Exceeded the size of our dmamap */
- map->_dm_type = 0;
- map->_dm_source = NULL;
- return E2BIG;
- }
}
#ifdef DEBUG
@@ -1268,7 +1278,7 @@
sglen, len);
Debugger();
}
- retval = bus_dmamap_load_raw(t, map, segs, i,
+ retval = bus_dmamap_load_raw(t, map, segs, i + 1,
(bus_size_t)len, flags);
if (map->dm_mapsize != len) {
printf("load_mbuf: mapsize %ld != len %lx\n",
@@ -1286,7 +1296,7 @@
return (retval);
}
#endif
- return (bus_dmamap_load_raw(t, map, segs, i,
+ return (bus_dmamap_load_raw(t, map, segs, i + 1,
(bus_size_t)len, flags));
#else
panic("_bus_dmamap_load_mbuf: not implemented");
@@ -1365,7 +1375,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;
----Next_Part(Tue_Jun_11_12:58:47_2002_229)----