| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20080317223153.GA3462@electric-eye.fr.zoreil.com>
Date: Mon, 17 Mar 2008 23:31:53 +0100
From: Francois Romieu <romieu@...zoreil.com>
To: Florian Fainelli <florian.fainelli@...ecomint.eu>
Cc: Johannes Berg <johannes@...solutions.net>,
David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
Jeff Garzik <jeff@...zik.org>, Felix Fietkau <nbd@...nwrt.org>
Subject: Re: [PATCH] Add support the Korina (IDT RC32434) Ethernet MAC
Florian Fainelli <florian.fainelli@...ecomint.eu> :
[...]
> Thanks for your comments, here is a third try which adresses the
> mentionned problems.
Sorry for the delay.
1. skb_new->dev = dev; in korina_rx() is useless. eth_type_trans() will
do it anyway.
2. skb->dev = dev; in korina_init() is useless as well.
3. korina_rx() still drops the received skb each time dev_alloc_skb fails.
This is far from optimal.
4. The value returned from korina_init() is now checked. That's fine.
It is carefully overwritten with -ENOMEM when korina_init() fails
while it could/should simply propagate the error code returned from
korina_init() (which currentl equals -ENOMEM btw).
5. I would add some korina_{alloc/free}_ring() helpers to remove
dev_kfree_skb_any() and dev_alloc_skb from korina_init(), then release
the ring when it is really needed (i.e. in korina_restart() instead of
korina_init()).
6. korina_open() leaks memory allocated in korina_init() if any of the
request_irq() fails...
7. ... and if none of the request_irq fails, it should probably skip the
error path :o)
diff --git a/drivers/net/korina.c b/drivers/net/korina.c
index 04ce894..f866fe4 100644
--- a/drivers/net/korina.c
+++ b/drivers/net/korina.c
@@ -956,59 +956,57 @@ static int korina_open(struct net_device *dev)
if (korina_init(dev) < 0) {
printk(KERN_ERR DRV_NAME "%s: cannot open device\n", dev->name);
ret = -ENOMEM;
- goto err_rx_irq;
+ goto out;
}
/* Install the interrupt handler
* that handles the Done Finished
* Ovr and Und Events */
- if (request_irq(lp->rx_irq, &korina_rx_dma_interrupt,
- IRQF_SHARED | IRQF_DISABLED,
- "Korina ethernet Rx", dev)) {
+ ret = request_irq(lp->rx_irq, &korina_rx_dma_interrupt,
+ IRQF_SHARED | IRQF_DISABLED, "Korina ethernet Rx", dev);
+ if (ret < 0) {
printk(KERN_ERR DRV_NAME "%s: unable to get Rx DMA IRQ %d\n",
dev->name, lp->rx_irq);
- ret = -ENXIO;
- goto err_rx_irq;
+ goto err_release_0;
}
- if (request_irq(lp->tx_irq, &korina_tx_dma_interrupt,
- IRQF_SHARED | IRQF_DISABLED,
- "Korina ethernet Tx", dev)) {
+
+ ret = request_irq(lp->tx_irq, &korina_tx_dma_interrupt,
+ IRQF_SHARED | IRQF_DISABLED, "Korina ethernet Tx", dev);
+ if (ret < 0) {
printk(KERN_ERR DRV_NAME "%s: unable to get Tx DMA IRQ %d\n",
dev->name, lp->tx_irq);
- free_irq(lp->rx_irq, dev);
- ret = -ENXIO;
- goto err_tx_irq;
+ goto err_free_rx_irq_1;
}
/* Install handler for overrun error. */
- if (request_irq(lp->ovr_irq, &korina_ovr_interrupt,
- IRQF_SHARED | IRQF_DISABLED,
- "Ethernet Overflow", dev)) {
+ ret = request_irq(lp->ovr_irq, &korina_ovr_interrupt,
+ IRQF_SHARED | IRQF_DISABLED, "Ethernet Overflow", dev);
+ if (ret < 0) {
printk(KERN_ERR DRV_NAME"%s: unable to get OVR IRQ %d\n",
dev->name, lp->ovr_irq);
- ret = -ENXIO;
- goto err_ovr_irq;
+ goto err_free_tx_irq_2;
}
/* Install handler for underflow error. */
- if (request_irq(lp->und_irq, &korina_und_interrupt,
- IRQF_SHARED | IRQF_DISABLED,
- "Ethernet Underflow", dev)) {
+ ret = request_irq(lp->und_irq, &korina_und_interrupt,
+ IRQF_SHARED | IRQF_DISABLED, "Ethernet Underflow", dev);
+ if (ret < 0)
printk(KERN_ERR DRV_NAME "%s: unable to get UND IRQ %d\n",
dev->name, lp->und_irq);
- ret = -ENXIO;
- goto err_und_irq;
+ goto err_free_out_irq_3;
}
+out:
+ return ret;
-err_und_irq:
+err_free_ovr_irq_3:
free_irq(lp->ovr_irq, dev);
-err_ovr_irq:
+err_free_tx_irq_2:
free_irq(lp->tx_irq, dev);
-err_tx_irq:
+err_free_rx_irq_1:
free_irq(lp->rx_irq, dev);
-err_rx_irq:
-
- return ret;
+err_release_0:
+ // Help me, I'm leaking memory allocated in korina_init().
+ goto out;
}
static int korina_close(struct net_device *dev)
8. korinal_close() leaks the memory allocated in korina_init().
9. The void * casts in korina_remove() are useless.
10. The casts from void * to struct net_device * are not needed in
korina_tx_dma_interrupt, korina_und_interrupt and korina_ovr_interrupt
either.
11. Same thing as 10 with platform_data and (struct korina_device *) in
korina_probe. You could use platform_get_drvdata btw.
12. I am not sure that the unbalanced use of disable_irq() in korina_close()
is really polite when one of the irq is shared.
13. Add a "FIXME: check korina_restart returned status code" somewhere.
14. Remove kfree(lp) and kfree(bif->dev) from korina_remove(). free_netdev()
will clean the allocated device.
15. Nit: there are a few EOL tabs and tabs after space.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists