Messages in this thread | | | Date | Tue, 8 Aug 2000 18:59:10 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH] fixed: checking kmalloc, init_etherdev and other fixes |
| |
Em Tue, Aug 08, 2000 at 03:40:52PM -0400, Jeff Garzik escreveu: > Arnaldo, > > You have definitely found a heap of bugs. The patch does need a little > work though. Patch review:
yap, I'll be working on it tonight, and yes, there's lots more on other drivers, that I'm working now. From now on I'll be sending patches to individual drivers and not a whole bunch of fixes (or new bugs ;) ).
> 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.
nod, will be checking this as well.
> 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.
Linus asked me to do it the way you ask here, I've already reworked the patch to do it this way and have already sent to you.
> 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.
the acenic one isn't needed, and yes, I'm now sending the patches to the maintainers and to lkml.
> 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. :/
ok, this driver needs more work, one guy here have already spotted one bug I've introduced, fixing this as well.
> 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 :(
Ok, it handles some of the issues that you've talked here and that other people pointed to me, I'll be looking at the rmmod cases tonight, as well as auditing these cases on the pcmcia drivers, that also have several cases like the ones discussed here.
- Arnaldo
- 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/
| |