Hi David, > From: David Miller [mailto:da...@davemloft.net] > From: gfree.w...@foxmail.com > Date: Tue, 2 May 2017 13:58:42 +0800 > > > These following drivers allocate kinds of resources in its ndo_init > > func, free some of them or all in the destructor func. Then there is > > one memleak that some errors happen after register_netdevice invokes > > the ndo_init callback. Because only the ndo_uninit callback is invoked > > in the error handler of register_netdevice, but destructor not. > > > > In my original approach, I tried to free the resources in the newlink > > func when fail to register_netdevice, like destructor did except not > > free the net_dev. This method is not good when destructor is changed, > > and the memleak could be not fixed when there is no newlink callback. > > > > Now create one new func used to free the resources in the destructor, > > and the ndo_uninit func also could invokes it when fail to register > > the net_device by comparing the dev->reg_state with > > NETREG_UNINITIALIZED. If there is no existing ndo_uninit, just add > > one. > > > > This solution doesn't only make sure free all resources in any case, > > but also follows the original desgin that some resources could be kept > > until the destructor executes normally after register the device > > successfully. > > Device private teardown is in two stages for the following reason. > The issue is that netdev_ops->ndo_init() allocates two types of resources. > > One type is OK to release during destruction before the netdev refs goes to > zero. This is what netdev_ops->ndo_uninit() is for. > > The second type is for releasing things which are not safe to drop until the very > last netdev reference disappears. This is what > netdev->destructor() is for. > > If you look around there are hacks in place all over to try and deal with this > issue. Basically, look for code that checks the return value of > register_netdev() and if an error is indicated it does some local driver state > freeing. Bonding is one example. It is trying to deal with the problem this > patch set is targetting.
Yes. When I was fixing this bug, I had found the bond had deal with this issue, frees the resources by itself. > > What really needs to happen is we must divorce the logic of > dev->destructor() from free_netdev(). > > That way we can do the free_netdev in the unregister netdevice path after > calling netdev->destructor(). > > Then the only issue callers of register_netdevice() need to be aware of is that if > an error is returned, that caller must call free_netdev(). Which has been the > case for decades. > > So I would fix this as follows: > > 1) Rename netdev->destructor() into "netdev->priv_destructor()" It > performs all post ndo_ops->ndo_uninit() cleanups, _except_ > free_netdev(). Update all drivers with netdev->destructor(). > > 2) Add a boolean state to netdev, which indicates if free_netdev() > should be performed after netdev->priv_destructor() during > unregister. > > That provides all of the flexibility necessary to fix this bug in the core. > > In register_netdevice() if something after ndo_ops->ndo_init() succeeeds, we > invoke _both_ ndo_ops->ndo_uninit() and > netdev->priv_destructor(). We do not look at the netdev "needs > free_netdev()" boolean. > > In netdev_run_todo(), where we have the one and only > netdev->destructor() call, change it to: > > if (dev->priv_destructor) > dev->priv_destructor(dev); > if (dev->needs_free_netdev) > free_netdev(dev); > > That fixes the bug in all cases. And makes the purpose and logic extremely > clear. Also, no internal state tests leak into the drivers. > > Finally, drivers that try to cover up this issue, such as bonding, need to be > changed to no try and free up device private state if their invocation of > register_netdevice() fails. > > Thanks. Ok. It is better to fix it by changing the framework of net_dev. I was afraid that I would bring other bugs if I changed it. Because it would affect too many codes. I would like to see you fix it by yourself. Best Regards Feng
