Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Neil Horman <nhorman@tuxdriver.com>]

On Wed, Dec 24, 2014 at 10:12:58AM +0800, Jia-Ju Bai wrote:
> On 12/23/2014 11:43 PM, Neil Horman wrote:
> >No, I don't think so.  vortex_close predicates each free with a NULL check, so
> >if its not been allocated, it shouldn't be freed.  vortex_close also puts the
> >adapter back into a known state (undoing all the setup that vortex_open does).
> >I really think its better to go with the proper close path than just unwinding
> >the allocation
> >
> >Neil
> >
> 
> Firstly, I run my match on the real hardware(3com 3c905B 100Base
> PCI Ethernet Controller) and make vortex_up failed on purpose
> (make "pci_enable_device" in vortex_up failed). During runtime, the driver
> works well and memory leaks are fixed.
> 
> Secondly, I revise the code according to your opinion:
> 
>         retval = vortex_up(dev);
>         if (!retval)
>             goto out;
> 
> +      vortex_close(dev);
> +      return -ENOMEM;
> 
> Then I repeat my experiment, but system hang occurs!
> 
> After adding some "printk"s into the code and running the driver, I find
> the problem's source:
> vortex_close calls vortex_down in runtime, and vortex_down calls
> "del_timer_sync(&vp->rx_oom_timer);" in the code. However, I make
> "pci_enable_device" failed in vortext_up to let vortex_up return an
> error code directly, but "vp->rx_oom_timer" is initialized only by
> "init_timer" after "pci_enable_device". Thus when
> "del_timer_sync(&vp->rx_oom_timer);" is called in vortex_down,
> a null dereference may occur.
> Moreover, only "pci_enable_device" can make vortex_up failed.
> 
> 
Sooo, fix it.  Add some checks to not delete the timer if its not been
initalized.  Its really preferable to have a single teardown path and a single
bringup path if at all possible
Neil