Linux-kernel mailing list archive 2000-32,: Re: [PATCH] fixed: checking kmalloc, init_etherdev and other fixes

# Re: [PATCH] fixed: checking kmalloc, init_etherdev and other fixes

Jeff Garzik (jgarzik@mandrakesoft.com)
Tue, 08 Aug 2000 15:40:52 -0400

- Messages sorted by: [ date ][ thread ][ subject ][ author ]
- Next message: Nathan Straz: "[Announce] Linux Test Project"
- Previous message: Bill Wendling: "Re: [PATCH] aironet4500_cs.c kmalloc checking"
- Maybe in reply to: Arnaldo Carvalho de Melo: "[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/

- Next message: Nathan Straz: "[Announce] Linux Test Project"
- Previous message: Bill Wendling: "Re: [PATCH] aironet4500_cs.c kmalloc checking"
- Maybe in reply to: Arnaldo Carvalho de Melo: "[PATCH] fixed: checking kmalloc, init_etherdev and other fixes"