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/
This archive was generated by hypermail 2b29 : Tue Aug 15 2000 - 21:00:16 EST