* [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure @ 2009-08-07 17:57 Roel Kluin 2009-08-13 4:08 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-08-07 17:57 UTC (permalink / raw) To: netdev, Andrew Morton, David S. Miller When dev_alloc_skb fails in the first iteration, a buffer underrun occurs. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Is this the best way to fix this? Untested, please review. diff --git a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c index a075801..a82ac32 100644 --- a/drivers/net/yellowfin.c +++ b/drivers/net/yellowfin.c @@ -728,7 +728,7 @@ static void yellowfin_tx_timeout(struct net_device *dev) static void yellowfin_init_ring(struct net_device *dev) { struct yellowfin_private *yp = netdev_priv(dev); - int i; + int i, j; yp->tx_full = 0; yp->cur_rx = yp->cur_tx = 0; @@ -753,6 +753,11 @@ static void yellowfin_init_ring(struct net_device *dev) yp->rx_ring[i].addr = cpu_to_le32(pci_map_single(yp->pci_dev, skb->data, yp->rx_buf_sz, PCI_DMA_FROMDEVICE)); } + if (i != RX_RING_SIZE) { + for (j = 0; j < i; j++) + dev_kfree_skb(yp->rx_skbuff[j]); + return; + } yp->rx_ring[i-1].dbdma_cmd = cpu_to_le32(CMD_STOP); yp->dirty_rx = (unsigned int)(i - RX_RING_SIZE); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure 2009-08-07 17:57 [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure Roel Kluin @ 2009-08-13 4:08 ` David Miller 2009-08-14 9:59 ` Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2009-08-13 4:08 UTC (permalink / raw) To: roel.kluin; +Cc: netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Fri, 07 Aug 2009 19:57:28 +0200 > When dev_alloc_skb fails in the first iteration, a buffer underrun occurs. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > Is this the best way to fix this? Untested, please review. I think this is a case where this code is going to need to be majorly reworked so that you can pass error status up to the caller when this allocation failure happens, and the caller can properly act upon it. Just silently returning when no RX ring has been allocated, and the TX ring hasn't been setup at all, is going to be worse than the array overrun you're supposedly fixing. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure 2009-08-13 4:08 ` David Miller @ 2009-08-14 9:59 ` Roel Kluin 2009-08-14 23:17 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-08-14 9:59 UTC (permalink / raw) To: David Miller; +Cc: netdev, akpm When dev_alloc_skb fails in the first iteration, a buffer underrun occurs. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- > I think this is a case where this code is going to need to > be majorly reworked so that you can pass error status up > to the caller when this allocation failure happens, and > the caller can properly act upon it. > > Just silently returning when no RX ring has been allocated, > and the TX ring hasn't been setup at all, is going to be > worse than the array overrun you're supposedly fixing. I think it should be something like this. Should yellowfin_open() do more than just passing the yellowfin_init_ring() error? diff --git a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c index a075801..ee35b11 100644 --- a/drivers/net/yellowfin.c +++ b/drivers/net/yellowfin.c @@ -346,7 +346,7 @@ static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd); static int yellowfin_open(struct net_device *dev); static void yellowfin_timer(unsigned long data); static void yellowfin_tx_timeout(struct net_device *dev); -static void yellowfin_init_ring(struct net_device *dev); +static int yellowfin_init_ring(struct net_device *dev); static int yellowfin_start_xmit(struct sk_buff *skb, struct net_device *dev); static irqreturn_t yellowfin_interrupt(int irq, void *dev_instance); static int yellowfin_rx(struct net_device *dev); @@ -573,19 +573,22 @@ static int yellowfin_open(struct net_device *dev) { struct yellowfin_private *yp = netdev_priv(dev); void __iomem *ioaddr = yp->base; - int i; + int i, ret; /* Reset the chip. */ iowrite32(0x80000000, ioaddr + DMACtrl); - i = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); - if (i) return i; + ret = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); + if (ret) + return ret; if (yellowfin_debug > 1) printk(KERN_DEBUG "%s: yellowfin_open() irq %d.\n", dev->name, dev->irq); - yellowfin_init_ring(dev); + ret = yellowfin_init_ring(dev); + if (ret) + return ret; iowrite32(yp->rx_ring_dma, ioaddr + RxPtr); iowrite32(yp->tx_ring_dma, ioaddr + TxPtr); @@ -725,10 +728,10 @@ static void yellowfin_tx_timeout(struct net_device *dev) } /* Initialize the Rx and Tx rings, along with various 'dev' bits. */ -static void yellowfin_init_ring(struct net_device *dev) +static int yellowfin_init_ring(struct net_device *dev) { struct yellowfin_private *yp = netdev_priv(dev); - int i; + int i, j; yp->tx_full = 0; yp->cur_rx = yp->cur_tx = 0; @@ -753,6 +756,11 @@ static void yellowfin_init_ring(struct net_device *dev) yp->rx_ring[i].addr = cpu_to_le32(pci_map_single(yp->pci_dev, skb->data, yp->rx_buf_sz, PCI_DMA_FROMDEVICE)); } + if (i != RX_RING_SIZE) { + for (j = 0; j < i; j++) + dev_kfree_skb(yp->rx_skbuff[j]); + return -ENOMEM; + } yp->rx_ring[i-1].dbdma_cmd = cpu_to_le32(CMD_STOP); yp->dirty_rx = (unsigned int)(i - RX_RING_SIZE); @@ -769,8 +777,6 @@ static void yellowfin_init_ring(struct net_device *dev) yp->tx_ring[--i].dbdma_cmd = cpu_to_le32(CMD_STOP | BRANCH_ALWAYS); #else { - int j; - /* Tx ring needs a pair of descriptors, the second for the status. */ for (i = 0; i < TX_RING_SIZE; i++) { j = 2*i; @@ -805,7 +811,7 @@ static void yellowfin_init_ring(struct net_device *dev) } #endif yp->tx_tail_desc = &yp->tx_status[0]; - return; + return 0; } static int yellowfin_start_xmit(struct sk_buff *skb, struct net_device *dev) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure 2009-08-14 9:59 ` Roel Kluin @ 2009-08-14 23:17 ` David Miller 2009-08-18 8:50 ` Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2009-08-14 23:17 UTC (permalink / raw) To: roel.kluin; +Cc: netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Fri, 14 Aug 2009 11:59:14 +0200 > /* Reset the chip. */ > iowrite32(0x80000000, ioaddr + DMACtrl); > > - i = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); > - if (i) return i; > + ret = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); > + if (ret) > + return ret; > > if (yellowfin_debug > 1) > printk(KERN_DEBUG "%s: yellowfin_open() irq %d.\n", > dev->name, dev->irq); > > - yellowfin_init_ring(dev); > + ret = yellowfin_init_ring(dev); > + if (ret) > + return ret; > > iowrite32(yp->rx_ring_dma, ioaddr + RxPtr); > iowrite32(yp->tx_ring_dma, ioaddr + TxPtr); If yellowfin_init_ring() returns an error, you're leaking the IRQ allocated by request_irq(). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure 2009-08-14 23:17 ` David Miller @ 2009-08-18 8:50 ` Roel Kluin 2009-08-19 3:21 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-08-18 8:50 UTC (permalink / raw) To: David Miller; +Cc: netdev, akpm yellowfin_init_ring() needs to clean up if dev_alloc_skb() fails and should pass an error status up to the caller. This also prevents an buffer underrun if failure occurred in the first iteration. yellowfin_open() which calls yellowfin_init_ring() should free its requested irq upon failure. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- diff --git a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c index a075801..c2fd618 100644 --- a/drivers/net/yellowfin.c +++ b/drivers/net/yellowfin.c @@ -346,7 +346,7 @@ static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd); static int yellowfin_open(struct net_device *dev); static void yellowfin_timer(unsigned long data); static void yellowfin_tx_timeout(struct net_device *dev); -static void yellowfin_init_ring(struct net_device *dev); +static int yellowfin_init_ring(struct net_device *dev); static int yellowfin_start_xmit(struct sk_buff *skb, struct net_device *dev); static irqreturn_t yellowfin_interrupt(int irq, void *dev_instance); static int yellowfin_rx(struct net_device *dev); @@ -573,19 +573,24 @@ static int yellowfin_open(struct net_device *dev) { struct yellowfin_private *yp = netdev_priv(dev); void __iomem *ioaddr = yp->base; - int i; + int i, ret; /* Reset the chip. */ iowrite32(0x80000000, ioaddr + DMACtrl); - i = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); - if (i) return i; + ret = request_irq(dev->irq, &yellowfin_interrupt, IRQF_SHARED, dev->name, dev); + if (ret) + return ret; if (yellowfin_debug > 1) printk(KERN_DEBUG "%s: yellowfin_open() irq %d.\n", dev->name, dev->irq); - yellowfin_init_ring(dev); + ret = yellowfin_init_ring(dev); + if (ret) { + free_irq(dev->irq, dev); + return ret; + } iowrite32(yp->rx_ring_dma, ioaddr + RxPtr); iowrite32(yp->tx_ring_dma, ioaddr + TxPtr); @@ -725,10 +730,10 @@ static void yellowfin_tx_timeout(struct net_device *dev) } /* Initialize the Rx and Tx rings, along with various 'dev' bits. */ -static void yellowfin_init_ring(struct net_device *dev) +static int yellowfin_init_ring(struct net_device *dev) { struct yellowfin_private *yp = netdev_priv(dev); - int i; + int i, j; yp->tx_full = 0; yp->cur_rx = yp->cur_tx = 0; @@ -753,6 +758,11 @@ static void yellowfin_init_ring(struct net_device *dev) yp->rx_ring[i].addr = cpu_to_le32(pci_map_single(yp->pci_dev, skb->data, yp->rx_buf_sz, PCI_DMA_FROMDEVICE)); } + if (i != RX_RING_SIZE) { + for (j = 0; j < i; j++) + dev_kfree_skb(yp->rx_skbuff[j]); + return -ENOMEM; + } yp->rx_ring[i-1].dbdma_cmd = cpu_to_le32(CMD_STOP); yp->dirty_rx = (unsigned int)(i - RX_RING_SIZE); @@ -769,8 +779,6 @@ static void yellowfin_init_ring(struct net_device *dev) yp->tx_ring[--i].dbdma_cmd = cpu_to_le32(CMD_STOP | BRANCH_ALWAYS); #else { - int j; - /* Tx ring needs a pair of descriptors, the second for the status. */ for (i = 0; i < TX_RING_SIZE; i++) { j = 2*i; @@ -805,7 +813,7 @@ static void yellowfin_init_ring(struct net_device *dev) } #endif yp->tx_tail_desc = &yp->tx_status[0]; - return; + return 0; } static int yellowfin_start_xmit(struct sk_buff *skb, struct net_device *dev) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure 2009-08-18 8:50 ` Roel Kluin @ 2009-08-19 3:21 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2009-08-19 3:21 UTC (permalink / raw) To: roel.kluin; +Cc: netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Tue, 18 Aug 2009 10:50:34 +0200 > yellowfin_init_ring() needs to clean up if dev_alloc_skb() fails and > should pass an error status up to the caller. This also prevents an > buffer underrun if failure occurred in the first iteration. > yellowfin_open() which calls yellowfin_init_ring() should free its > requested irq upon failure. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> Looks good, applied, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-08-19 3:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-07 17:57 [PATCH] yellowfin: Fix buffer underrun after dev_alloc_skb() failure Roel Kluin 2009-08-13 4:08 ` David Miller 2009-08-14 9:59 ` Roel Kluin 2009-08-14 23:17 ` David Miller 2009-08-18 8:50 ` Roel Kluin 2009-08-19 3:21 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).