[lkml]   [2000]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] fixed: checking kmalloc, init_etherdev and other fixes

    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),
    + if ((dev->priv = kmalloc(sizeof(struct net_local),
    + 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
    acenic: zero *dma_handle also. make sure to CC Jes on all acenic

    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
    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
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:58    [W:0.022 / U:13.252 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site