Messages in this thread | | | Date | Tue, 08 Aug 2000 15:40:52 -0400 | From | Jeff Garzik <> | Subject | Re: [PATCH] fixed: checking kmalloc, init_etherdev and other fixes |
| |
Arnaldo,
You have definitely found a heap of bugs. The patch does need a little work though. Patch review:
In general, there is a logic flaw I see. In some drivers, you see
if (dev == NULL) dev = init_etherdev()
You add a check for init_etherdev failure, which is correct. However, you also need to check because some drivers do not correctly free the device structure at unload time. The remove module code for such cases needs to include something like
if (init_etherdev_was_called) { unregister_netdevice (dev); kfree (dev); }
Many of the drivers are lazy and simply do not handle this case.
A similar case occurs with dev->priv allocation. Some existing code does this:
if (dev->priv == NULL) dev->priv = kmalloc()
The driver needs to handle this correctly too:
if (dev_priv_allocated_manually) kfree (dev->priv);
Also, many of your patches put two operations on one line:
- dev->priv = kmalloc(sizeof(struct net_local), GFP_KERNEL); + if ((dev->priv = kmalloc(sizeof(struct net_local), GFP_KERNEL)) == NULL) + return -ENOMEM;
There is no need to merge the kmalloc and NULL check into one line. It causes errors (as your earlier patch proved) and makes the diff larger than necessary.
3c507: don't put two operations on the same line 3c515: looks like more cleanup is necessary before returning NULL on error acenic: zero *dma_handle also. make sure to CC Jes on all acenic patches.
aironet4500_card: awc_pci_init failure causes leaks. You should alloc dev->priv using init_etherdev. Cleanup for failures in awc_pci_init is incomplete. Return the return val from request_irq, not a generic error. Don't stuff two operations onto one line. request_region handling and cleanup is incomplete (see awc_pci_init failure, above). This driver has a lot of problems before your patch, so I'm not surprised that more work is required. :/
apne: two ops on one line. possibly unhandled cases in remove module. ariadne2.c: ditto at1700: ditto atari_bionet: ditto atari_pamsnet: ditto atarilance: ditto bagetlance: ditto declance: unhandled remove module cases dgrs: should all devices be freed, if an alloc fails for only one device? e2100: two ops on one line. possibly unhandled cases at rmmod time. es3210: ditto eth16i: ditto fmv18x: ditto. also you add a leak if request_region fails. hp-plus: two ops on one line. possibly unhandled cases at rmmod time. hp: ditto hp100: continue the loop, do not return an error hplance: two ops on one line. possibly unhandled cases at rmmod time.
Since you apparently just sent out an updated patch, I'll stop here and start looking over that one. It doesn't look like your new patch handles the rmmod cases :(
-- Jeff Garzik | Building 1024 | Yossarian lives. MandrakeSoft, Inc. |
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |