korina: Read buffer overflow

PATCH In review Roel Kluin 3 participants 13 messages Aug 7, 2009 – Aug 13, 2009 3 messages not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Roel Kluin

author

2009-08-07 15:29 UTC #0 lore raw

PATCH 1/1 korina: Read buffer overflow

If the loop breaks with an i of 0, then we read lp->rd_ring[-1].

Signed-off-by Roel Kluin <[email protected]>

---
Should we clean up like this? please review

drivers/net/korina.c +8 -2

index b4cf602..b965b2b 100644
@@ -754,7 +754,7 @@ static void korina_alloc_ring(struct net_device *dev)
 {
 	struct korina_private *lp = netdev_priv(dev);
 	struct sk_buff *skb;
-	int i;
+	int i, j;
 
 	/* Initialize the transmit descriptors */
 	for (i = 0; i < KORINA_NUM_TDS; i++) {

@@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev)
 	for (i = 0; i < KORINA_NUM_RDS; i++) {
 		skb = dev_alloc_skb(KORINA_RBSIZE + 2);
 		if (!skb)
-			break;
+			goto err_free;
 		skb_reserve(skb, 2);
 		lp->rx_skb[i] = skb;
 		lp->rd_ring[i].control = DMA_DESC_IOD |

@@ -790,6 +790,12 @@ static void korina_alloc_ring(struct net_device *dev)
 	lp->rx_chain_head = 0;
 	lp->rx_chain_tail = 0;
 	lp->rx_chain_status = desc_empty;
+err_free:
+	for (j = 0; j < i; j++) {
+		lp->rd_ring[j].control = 0;
+		dev_kfree_skb_any(lp->rx_skb[j]);
+		lp->rx_skb[j] = NULL;
+	}
 }
 
 static void korina_free_ring(struct net_device *dev)

roel kluin

author

2009-08-08 13:14 UTC #2 lore raw

Quoted 12 lines

drivers/net/korina.c static void korina_alloc_ring(struct net_device *dev) · line 774
Quoted 8 lines
If the loop breaks with an i of 0, then we read lp->rd_ring[-1].
drivers/net/korina.c +1 -1
@@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev)
       for (i = 0; i < KORINA_NUM_RDS; i++) {
               skb = dev_alloc_skb(KORINA_RBSIZE + 2);
               if (!skb)
-                     break;
+                     goto err_free;
This implies that all KORINA_NUM_TDS receive descriptors need to be available for the driver to work.

Quoted 3 lines

Also I guess there should be some error handling, as the driver probably wont be useful without a single receive descriptor. What do you think about the following:

A few comments:

Quoted 5 lines

| diff --git a/drivers/net/korina.c b/drivers/net/korina.c
| index a2701f5..d1c5276 100644
| --- a/drivers/net/korina.c
| +++ b/drivers/net/korina.c

Quoted 7 lines

| @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev)
|               lp->rd_ring[i].ca = CPHYSADDR(skb->data);
|               lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]);
|       }
| +     if (!i) {
| +             printk(KERN_ERR DRV_NAME "%s: could not allocate a single"
| +                             " receive descriptor\n", dev->name)

printk(KERN_ERR "%s: could not allocate a single receive
descriptor\n",

                            dev->name);

Don't interrupt strings, they are an exception in the 80 character limit (since they are
easier to grep that way). Also I think the DRV_NAME should be omitted, Doesn't `DRV_NAME "%s:...\n", dev->name)' result in "korinakorina:..."? It occurs at more
places in the file. Also a semicolon was missing.

Quoted 1 line

| + return 1;

I think -ENOMEM is more appropriate.

Quoted 3 lines

| +     }
| +     printk(KERN_DEBUG DRV_NAME "%s: allocated %d receive descriptors\n",
| +                     dev->name, i);

same `DRV_NAME "%s:...\n", dev->name)' issue.

Quoted 7 lines

| @@ -824,7 +833,8 @@ static int korina_init(struct net_device *dev)
|       writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc);
|
|       /* Allocate rings */
| -     korina_alloc_ring(dev);
| +     if (korina_alloc_ring(dev))
| +             return 1;

-ENOMEM

Quoted 5 lines

|
|       writel(0, &lp->rx_dma_regs->dmas);
|       /* Start Rx DMA */

Greetings, Phil

Otherwise looks fine to me.

Thanks,

Roel

Phil Sutter

2009-08-08 16:45 UTC #3 lore raw

Hi,

Quoted 15 lines · roel kluin

A few comments:

Quoted 5 lines

| diff --git a/drivers/net/korina.c b/drivers/net/korina.c
| index a2701f5..d1c5276 100644
| --- a/drivers/net/korina.c
| +++ b/drivers/net/korina.c

Quoted 7 lines

| @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev)
|               lp->rd_ring[i].ca = CPHYSADDR(skb->data);
|               lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]);
|       }
| +     if (!i) {
| +             printk(KERN_ERR DRV_NAME "%s: could not allocate a single"
| +                             " receive descriptor\n", dev->name)

Quoted 6 lines

printk(KERN_ERR "%s: could not allocate a single receive
descriptor\n",
                            dev->name);
Don't interrupt strings, they are an exception in the 80 character limit (since they are easier to grep that way).

Ah, good to know.

Quoted 3 lines

Also I think the DRV_NAME should be omitted, Doesn't `DRV_NAME "%s:...\n", dev->name)' result in "korinakorina:..."? It occurs at more places in the file. Also a semicolon was missing.

Sounds like a valid point, I will check this. This patch was just a quick hack to illustrate what I had in mind, without even compile-testing it.

Quoted 1 line

Otherwise looks fine to me.

Great you like it, and many thanks for reviewing it. I'm still not completely sure the NIC will still work with less than 64 receive descriptors, but since the documentation wasn't really enlightening I'll add some run-time test to my TODO. Maybe one of the other readers has some advice here, I'll prepare a complete patch in the meantime.

Greetings, Phil

A message that is not in the archive (sent off-list or not yet received): [email protected]

Phil Sutter

2009-08-09 00:06 UTC #5 lore raw

PATCH 1/3 korina: fix printk formatting, add final info line

The macro DRV_NAME contains "korina", the field dev->name points to the actual interface name. So messages were formerly prefixed with 'korinaeth2:' (on my system).

Signed-off-by Phil Sutter <[email protected]>

---
 drivers/net/korina.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

drivers/net/korina.c +16 ~~-15~~

index a2701f5..c09e2ce 100644
@@ -337,7 +337,7 @@ static irqreturn_t korina_rx_dma_interrupt(int irq, void *dev_id)
 		napi_schedule(&lp->napi);
 
 		if (dmas & DMA_STAT_ERR)
-			printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name);
+			printk(KERN_ERR "%s: DMA error\n", dev->name);
 
 		retval = IRQ_HANDLED;
 	} else

@@ -555,7 +555,7 @@ static void korina_tx(struct net_device *dev)
 			dev->stats.tx_dropped++;
 
 			/* Should never happen */
-			printk(KERN_ERR DRV_NAME "%s: split tx ignored\n",
+			printk(KERN_ERR "%s: split tx ignored\n",
 							dev->name);
 		} else if (devcs & ETH_TX_TOK) {
 			dev->stats.tx_packets++;

@@ -641,7 +641,7 @@ korina_tx_dma_interrupt(int irq, void *dev_id)
 			dev->trans_start = jiffies;
 		}
 		if (dmas & DMA_STAT_ERR)
-			printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name);
+			printk(KERN_ERR "%s: DMA error\n", dev->name);
 
 		retval = IRQ_HANDLED;
 	} else

@@ -909,8 +909,7 @@ static int korina_restart(struct net_device *dev)
 
 	ret = korina_init(dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: cannot restart device\n",
-								dev->name);
+		printk(KERN_ERR "%s: cannot restart device\n", dev->name);
 		return ret;
 	}
 	korina_multicast_list(dev);

@@ -997,7 +996,7 @@ static int korina_open(struct net_device *dev)
 	/* Initialize */
 	ret = korina_init(dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: cannot open device\n", dev->name);
+		printk(KERN_ERR "%s: cannot open device\n", dev->name);
 		goto out;
 	}

@@ -1007,14 +1006,14 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->rx_irq, &korina_rx_dma_interrupt,
 			IRQF_DISABLED, "Korina ethernet Rx", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get Rx DMA IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get Rx DMA IRQ %d\n",
 		    dev->name, lp->rx_irq);
 		goto err_release;
 	}
 	ret = request_irq(lp->tx_irq, &korina_tx_dma_interrupt,
 			IRQF_DISABLED, "Korina ethernet Tx", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get Tx DMA IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get Tx DMA IRQ %d\n",
 		    dev->name, lp->tx_irq);
 		goto err_free_rx_irq;
 	}

@@ -1023,7 +1022,7 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->ovr_irq, &korina_ovr_interrupt,
 			IRQF_DISABLED, "Ethernet Overflow", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME"%s: unable to get OVR IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get OVR IRQ %d\n",
 		    dev->name, lp->ovr_irq);
 		goto err_free_tx_irq;
 	}

@@ -1032,7 +1031,7 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->und_irq, &korina_und_interrupt,
 			IRQF_DISABLED, "Ethernet Underflow", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get UND IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get UND IRQ %d\n",
 		    dev->name, lp->und_irq);
 		goto err_free_ovr_irq;
 	}

@@ -1111,7 +1110,7 @@ static int korina_probe(struct platform_device *pdev)
 	dev->base_addr = r->start;
 	lp->eth_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->eth_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap registers\n");
 		rc = -ENXIO;
 		goto probe_err_out;
 	}

@@ -1119,7 +1118,7 @@ static int korina_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_rx");
 	lp->rx_dma_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->rx_dma_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap Rx DMA registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap Rx DMA registers\n");
 		rc = -ENXIO;
 		goto probe_err_dma_rx;
 	}

@@ -1127,14 +1126,14 @@ static int korina_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_tx");
 	lp->tx_dma_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->tx_dma_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap Tx DMA registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap Tx DMA registers\n");
 		rc = -ENXIO;
 		goto probe_err_dma_tx;
 	}
 
 	lp->td_ring = kmalloc(TD_RING_SIZE + RD_RING_SIZE, GFP_KERNEL);
 	if (!lp->td_ring) {
-		printk(KERN_ERR DRV_NAME "cannot allocate descriptors\n");
+		printk(KERN_ERR DRV_NAME ": cannot allocate descriptors\n");
 		rc = -ENXIO;
 		goto probe_err_td_ring;
 	}

@@ -1175,9 +1174,11 @@ static int korina_probe(struct platform_device *pdev)
 	rc = register_netdev(dev);
 	if (rc < 0) {
 		printk(KERN_ERR DRV_NAME
-			": cannot register net device %d\n", rc);
+			": cannot register net device: %d\n", rc);
 		goto probe_err_register;
 	}
+	printk(KERN_INFO "%s: " DRV_NAME "-" DRV_VERSION " " DRV_RELDATE "\n",
+			dev->name);
 out:
 	return rc;

Signature · 1 line

1.6.0.6

A message that is not in the archive (sent off-list or not yet received): [email protected]

Phil Sutter

2009-08-09 00:06 UTC #7 lore raw

PATCH 2/3 korina: add error-handling to korina_alloc_ring

This also avoids a potential buffer overflow in case the very first receive descriptor fails to allocate, as an index of -1 would be used afterwards. Kudos to Roel Kluin for pointing this out and providing an initial patch.

Signed-off-by Phil Sutter <[email protected]>

---
 drivers/net/korina.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

drivers/net/korina.c +9 -3

index c09e2ce..d256022 100644
@@ -741,7 +741,7 @@ static struct ethtool_ops netdev_ethtool_ops = {
 	.get_link               = netdev_get_link,
 };
 
-static void korina_alloc_ring(struct net_device *dev)
+static int korina_alloc_ring(struct net_device *dev)
 {
 	struct korina_private *lp = netdev_priv(dev);
 	struct sk_buff *skb;

@@ -762,7 +762,7 @@ static void korina_alloc_ring(struct net_device *dev)
 	for (i = 0; i < KORINA_NUM_RDS; i++) {
 		skb = dev_alloc_skb(KORINA_RBSIZE + 2);
 		if (!skb)
-			break;
+			return -ENOMEM;
 		skb_reserve(skb, 2);
 		skb->dev = dev;
 		lp->rx_skb[i] = skb;

@@ -782,6 +782,8 @@ static void korina_alloc_ring(struct net_device *dev)
 	lp->rx_chain_head = 0;
 	lp->rx_chain_tail = 0;
 	lp->rx_chain_status = desc_empty;
+
+	return 0;
 }
 
 static void korina_free_ring(struct net_device *dev)

@@ -824,7 +826,11 @@ static int korina_init(struct net_device *dev)
 	writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc);
 
 	/* Allocate rings */
-	korina_alloc_ring(dev);
+	if (korina_alloc_ring(dev)) {
+		printk(KERN_ERR "%s: descriptor allocation failed\n", dev->name);
+		korina_free_ring(dev);
+		return -ENOMEM;
+	}
 
 	writel(0, &lp->rx_dma_regs->dmas);
 	/* Start Rx DMA */

Signature · 1 line

1.6.0.6

A message that is not in the archive (sent off-list or not yet received): [email protected]

Phil Sutter

2009-08-09 00:06 UTC #9 lore raw

PATCH 3/3 korina: convert to net_device_ops

Signed-off-by Phil Sutter <[email protected]>

---
 drivers/net/korina.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

drivers/net/korina.c +14 -9

index d256022..894f686 100644
@@ -1088,6 +1088,18 @@ static int korina_close(struct net_device *dev)
 	return 0;
 }
 
+static const struct net_device_ops korina_netdev_ops = {
+	.ndo_open               = korina_open,
+	.ndo_stop               = korina_close,
+	.ndo_start_xmit         = korina_send_packet,
+	.ndo_set_multicast_list = korina_multicast_list,
+	.ndo_tx_timeout         = korina_tx_timeout,
+	.ndo_do_ioctl           = korina_ioctl,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller    = korina_poll_controller,
+#endif
+};
+
 static int korina_probe(struct platform_device *pdev)
 {
 	struct korina_device *bif = platform_get_drvdata(pdev);

@@ -1156,17 +1168,10 @@ static int korina_probe(struct platform_device *pdev)
 	dev->irq = lp->rx_irq;
 	lp->dev = dev;
 
-	dev->open = korina_open;
-	dev->stop = korina_close;
-	dev->hard_start_xmit = korina_send_packet;
-	dev->set_multicast_list = &korina_multicast_list;
+	dev->netdev_ops = &korina_netdev_ops;
 	dev->ethtool_ops = &netdev_ethtool_ops;
-	dev->tx_timeout = korina_tx_timeout;
 	dev->watchdog_timeo = TX_TIMEOUT;
-	dev->do_ioctl = &korina_ioctl;
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	dev->poll_controller = korina_poll_controller;
-#endif
+
 	netif_napi_add(dev, &lp->napi, korina_poll, 64);
 
 	lp->phy_addr = (((lp->rx_irq == 0x2c? 1:0) << 8) | 0x05);

Signature · 1 line

1.6.0.6

Phil Sutter

2009-08-09 00:06 UTC #10 lore raw

In reply to Phil Sutter

Hi,

Testing of my approach to solve the buffer overrun issue showed that living with a subset of the requested receive descriptors is not as easy as initially assumed: uppon allocation failure the number of descriptors would have to be reduced to the next lower power of two (as also stated in the corresponding define's comment), which I consider too much overhead. A better solution would be to export the ring parameters via ethtool. For now, let's go Roel's way aborting completely and cleaning up in this case.

The following series fixes the incorrect printk formatting we already discussed, implements the solution from above and makes the driver use netdev_ops.

Greetings, Phil

Phil Sutter

2009-08-12 22:15 UTC #11 lore raw

Hi,

Quoted 3 lines · Phil Sutter

The following series fixes the incorrect printk formatting we already discussed, implements the solution from above and makes the driver use netdev_ops.

Obviously, I took the chance to mess things up again. These three patches were accidentially written on top of the linux-mips tree, right before Ralf pulled from Linus. So they do not apply cleanly to the netdev tree, and even worse the last one is completely useless since it's changes have already been implemented.

I will follow up to this email with an updated series of the two remaining, valid patches. Sorry for the inconvenience.

Greetings, Phil

Phil Sutter

2009-08-12 22:22 UTC #12 lore raw

PATCH 1/2 korina: fix printk formatting, add final info line

The macro DRV_NAME contains "korina", the field dev->name points to the actual interface name. So messages were formerly prefixed with 'korinaeth2:' (on my system).

Signed-off-by Phil Sutter <[email protected]>

---
 drivers/net/korina.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

drivers/net/korina.c +17 ~~-15~~

index b4cf602..6df9d25 100644
@@ -338,7 +338,7 @@ static irqreturn_t korina_rx_dma_interrupt(int irq, void *dev_id)
 		napi_schedule(&lp->napi);
 
 		if (dmas & DMA_STAT_ERR)
-			printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name);
+			printk(KERN_ERR "%s: DMA error\n", dev->name);
 
 		retval = IRQ_HANDLED;
 	} else

@@ -555,7 +555,7 @@ static void korina_tx(struct net_device *dev)
 			dev->stats.tx_dropped++;
 
 			/* Should never happen */
-			printk(KERN_ERR DRV_NAME "%s: split tx ignored\n",
+			printk(KERN_ERR "%s: split tx ignored\n",
 							dev->name);
 		} else if (devcs & ETH_TX_TOK) {
 			dev->stats.tx_packets++;

@@ -641,7 +641,7 @@ korina_tx_dma_interrupt(int irq, void *dev_id)
 			dev->trans_start = jiffies;
 		}
 		if (dmas & DMA_STAT_ERR)
-			printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name);
+			printk(KERN_ERR "%s: DMA error\n", dev->name);
 
 		retval = IRQ_HANDLED;
 	} else

@@ -917,8 +917,7 @@ static int korina_restart(struct net_device *dev)
 
 	ret = korina_init(dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: cannot restart device\n",
-								dev->name);
+		printk(KERN_ERR "%s: cannot restart device\n", dev->name);
 		return ret;
 	}
 	korina_multicast_list(dev);

@@ -1005,7 +1004,7 @@ static int korina_open(struct net_device *dev)
 	/* Initialize */
 	ret = korina_init(dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: cannot open device\n", dev->name);
+		printk(KERN_ERR "%s: cannot open device\n", dev->name);
 		goto out;
 	}

@@ -1015,14 +1014,14 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->rx_irq, &korina_rx_dma_interrupt,
 			IRQF_DISABLED, "Korina ethernet Rx", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get Rx DMA IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get Rx DMA IRQ %d\n",
 		    dev->name, lp->rx_irq);
 		goto err_release;
 	}
 	ret = request_irq(lp->tx_irq, &korina_tx_dma_interrupt,
 			IRQF_DISABLED, "Korina ethernet Tx", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get Tx DMA IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get Tx DMA IRQ %d\n",
 		    dev->name, lp->tx_irq);
 		goto err_free_rx_irq;
 	}

@@ -1031,7 +1030,7 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->ovr_irq, &korina_ovr_interrupt,
 			IRQF_DISABLED, "Ethernet Overflow", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME"%s: unable to get OVR IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get OVR IRQ %d\n",
 		    dev->name, lp->ovr_irq);
 		goto err_free_tx_irq;
 	}

@@ -1040,7 +1039,7 @@ static int korina_open(struct net_device *dev)
 	ret = request_irq(lp->und_irq, &korina_und_interrupt,
 			IRQF_DISABLED, "Ethernet Underflow", dev);
 	if (ret < 0) {
-		printk(KERN_ERR DRV_NAME "%s: unable to get UND IRQ %d\n",
+		printk(KERN_ERR "%s: unable to get UND IRQ %d\n",
 		    dev->name, lp->und_irq);
 		goto err_free_ovr_irq;
 	}

@@ -1137,7 +1136,7 @@ static int korina_probe(struct platform_device *pdev)
 	dev->base_addr = r->start;
 	lp->eth_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->eth_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap registers\n");
 		rc = -ENXIO;
 		goto probe_err_out;
 	}

@@ -1145,7 +1144,7 @@ static int korina_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_rx");
 	lp->rx_dma_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->rx_dma_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap Rx DMA registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap Rx DMA registers\n");
 		rc = -ENXIO;
 		goto probe_err_dma_rx;
 	}

@@ -1153,14 +1152,14 @@ static int korina_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_tx");
 	lp->tx_dma_regs = ioremap_nocache(r->start, r->end - r->start);
 	if (!lp->tx_dma_regs) {
-		printk(KERN_ERR DRV_NAME "cannot remap Tx DMA registers\n");
+		printk(KERN_ERR DRV_NAME ": cannot remap Tx DMA registers\n");
 		rc = -ENXIO;
 		goto probe_err_dma_tx;
 	}
 
 	lp->td_ring = kmalloc(TD_RING_SIZE + RD_RING_SIZE, GFP_KERNEL);
 	if (!lp->td_ring) {
-		printk(KERN_ERR DRV_NAME "cannot allocate descriptors\n");
+		printk(KERN_ERR DRV_NAME ": cannot allocate descriptors\n");
 		rc = -ENXIO;
 		goto probe_err_td_ring;
 	}

@@ -1193,10 +1192,13 @@ static int korina_probe(struct platform_device *pdev)
 	rc = register_netdev(dev);
 	if (rc < 0) {
 		printk(KERN_ERR DRV_NAME
-			": cannot register net device %d\n", rc);
+			": cannot register net device: %d\n", rc);
 		goto probe_err_register;
 	}
 	setup_timer(&lp->media_check_timer, korina_poll_media, (unsigned long) dev);
+
+	printk(KERN_INFO "%s: " DRV_NAME "-" DRV_VERSION " " DRV_RELDATE "\n",
+			dev->name);
 out:
 	return rc;

Signature · 1 line

1.6.0.6

Phil Sutter

2009-08-12 22:52 UTC #13 lore raw

PATCH 2/2 korina: add error-handling to korina_alloc_ring

Signed-off-by Phil Sutter <[email protected]>

---
 drivers/net/korina.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

drivers/net/korina.c +9 -3

index 6df9d25..51ca54c 100644
@@ -750,7 +750,7 @@ static struct ethtool_ops netdev_ethtool_ops = {
 	.get_link               = netdev_get_link,
 };
 
-static void korina_alloc_ring(struct net_device *dev)
+static int korina_alloc_ring(struct net_device *dev)
 {
 	struct korina_private *lp = netdev_priv(dev);
 	struct sk_buff *skb;

@@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev)
 	for (i = 0; i < KORINA_NUM_RDS; i++) {
 		skb = dev_alloc_skb(KORINA_RBSIZE + 2);
 		if (!skb)
-			break;
+			return -ENOMEM;
 		skb_reserve(skb, 2);
 		lp->rx_skb[i] = skb;
 		lp->rd_ring[i].control = DMA_DESC_IOD |

@@ -790,6 +790,8 @@ static void korina_alloc_ring(struct net_device *dev)
 	lp->rx_chain_head = 0;
 	lp->rx_chain_tail = 0;
 	lp->rx_chain_status = desc_empty;
+
+	return 0;
 }
 
 static void korina_free_ring(struct net_device *dev)

@@ -832,7 +834,11 @@ static int korina_init(struct net_device *dev)
 	writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc);
 
 	/* Allocate rings */
-	korina_alloc_ring(dev);
+	if (korina_alloc_ring(dev)) {
+		printk(KERN_ERR "%s: descriptor allocation failed\n", dev->name);
+		korina_free_ring(dev);
+		return -ENOMEM;
+	}
 
 	writel(0, &lp->rx_dma_regs->dmas);
 	/* Start Rx DMA */

Signature · 1 line

1.6.0.6

David Miller

maintainer Applied

2009-08-13 23:27 UTC #14 lore raw

Re: [PATCH 2/2] korina: add error-handling to korina_alloc_ring

From: Phil Sutter <[email protected]>
Date: Thu, 13 Aug 2009 00:52:32 +0200

Quoted 6 lines

This also avoids a potential buffer overflow in case the very first receive descriptor fails to allocate, as an index of -1 would be used afterwards. Kudos to Roel Kluin for pointing this out and providing an initial patch.
Signed-off-by Phil Sutter <[email protected]>

Applied to net-next-2.6

David Miller

maintainer Applied

2009-08-13 23:27 UTC #15 lore raw

Re: [PATCH 1/2] korina: fix printk formatting, add final info line

In reply to Phil Sutter

From: Phil Sutter <[email protected]>
Date: Thu, 13 Aug 2009 00:22:46 +0200

Quoted 5 lines

The macro DRV_NAME contains "korina", the field dev->name points to the actual interface name. So messages were formerly prefixed with 'korinaeth2:' (on my system).
Signed-off-by Phil Sutter <[email protected]>

Applied to net-next-2.6