Messages in this thread Patch in this message | | | Date | Mon, 06 Jul 1998 18:29:03 -0600 | From | Rob Riggs <> | Subject | Fatal bug in Boomerang driver found |
| |
The boomerang driver (3c59x.c) has two fatal bugs in boomerang_rx() that will cause a kernel panic.
Bug #1 will affect *all* machines once the 'signed int' counter vp->cur_rx goes greater than MAX_INT. At this point vp->cur_rx becomes a negative number and the for() loop used to fill the ring buffer always fails. The skbs in the ring buffer will never be allocated (all will be set to NULL). The driver does not test for a NULL skb and calls skb_put with a NULL value, generating a kernel panic. The solution is to test for the wrap and adjust both vp->cur_rx and vp->dirty_rx accordingly.
Bug #2 can affect any machine, but machines that have plenty of RAM are less likely to be affected. If the driver fails to replenish one of the skbs in the ring buffers (because DEV_ALLOC_SKB returned NULL) then the buffer will point to NULL (or 0x00). The driver does not test for a NULL skb and calls skb_put with a NULL value, generating a kernel panic. I beleive the correct solution here is to break from the read loop and try to replenish the ring buffer. There may be other issues involved here that I am not aware of, but Donald will need to check this out.
There is one other issue that I have with the driver. In boomerang_rx(), rx_work_limit is set and is not checked until *after* it has tried to put at least one packet into an skb. However, it is possible that the we do not have any skbs ready in the ring buffer (as described above). It would be better to check this variable *before* trying to fill the NULL skb. In fact, now that I think about it, that is probably the best way to deal with Bug #2.
The enclosed patch is against .99E and should fix both #1 and #2 (with the caveat noted above), but does not deal with the last issue. Please test and report results.
(Please note that I follow the above lists through the various archives, rather that subscribing directly. Should you have comments for me, please cc: me directly.)
Thanks.
-Rob -- Rob Riggs Devil's Thumb Entertainment Network Administrator Boulder, CO - (303) 938-1200 rob@DevilsThumb.COM http://www.DevilsThumb.COM/~rob "The notion of errors is ill-defined." - IRIX 'netstat' man page--- 3c59x.c.orig Mon Jul 6 12:16:25 1998 +++ 3c59x.c Mon Jul 6 17:31:17 1998 @@ -1801,6 +1801,11 @@ void *temp; /* Pass up the skbuff already on the Rx ring. */ skb = vp->rx_skbuff[entry]; + if (skb == NULL) { + printk(KERN_ERR "boomerang: Tried to fill a NULL skb!\n"); + break; + } + vp->rx_skbuff[entry] = NULL; #if LINUX_VERSION_CODE >= 0x10300 temp = skb_put(skb, pkt_len); @@ -1833,6 +1838,11 @@ vp->stats.rx_packets++; } entry = (++vp->cur_rx) % RX_RING_SIZE; + /* Wrap our counters -- otherwise the next for() loop fails */ + if (vp->cur_rx < 0) { + vp->cur_rx = 0; + vp->dirty_rx -= INT_MAX; + } if (--rx_work_limit < 0) break; } @@ -1842,8 +1852,10 @@ entry = vp->dirty_rx % RX_RING_SIZE; if (vp->rx_skbuff[entry] == NULL) { skb = DEV_ALLOC_SKB(PKT_BUF_SZ); - if (skb == NULL) + if (skb == NULL) { + printk(KERN_ERR "boomerang: Could not allocate skb on ring buffer!\n"); break; /* Bad news! */ + } skb->dev = dev; /* Mark as being used by this device. */ #if LINUX_VERSION_CODE > 0x10300 skb_reserve(skb, 2); /* Align IP on 16 byte boundaries */ | |