On Wed, 26 Jun 2019 at 14:03, Roopa Prabhu <[email protected]> wrote: > > On Tue, Jun 25, 2019 at 9:08 AM Taehee Yoo <[email protected]> wrote: > > > > On Tue, 25 Jun 2019 at 13:12, Roopa Prabhu <[email protected]> > > wrote: > > > > > > > Hi Roopa, > > > > Thank you for the review! > > > > > On Sun, Jun 23, 2019 at 7:18 PM Taehee Yoo <[email protected]> wrote: > > > > > > > > On Mon, 24 Jun 2019 at 03:07, David Miller <[email protected]> wrote: > > > > > > > > > > > > > Hi David, > > > > > > > > Thank you for the review! > > > > > > > > > From: Taehee Yoo <[email protected]> > > > > > Date: Thu, 20 Jun 2019 20:51:08 +0900 > > > > > > > > > > > __vxlan_dev_create() destroys FDB using specific pointer which > > > > > > indicates > > > > > > a fdb when error occurs. > > > > > > But that pointer should not be used when register_netdevice() fails > > > > > > because > > > > > > register_netdevice() internally destroys fdb when error occurs. > > > > > > > > > > > > In order to avoid un-registered dev's notification, fdb destroying > > > > > > routine > > > > > > checks dev's register status before notification. > > > > > > > > > > Simply pass do_notify as false in this failure code path of > > > > > __vxlan_dev_create(), > > > > > thank you. > > > > > > > > Failure path of __vxlan_dev_create() can't handle do_notify in that case > > > > because if register_netdevice() fails it internally calls > > > > ->ndo_uninit() which is > > > > vxlan_uninit(). > > > > vxlan_uninit() internally calls vxlan_fdb_delete_default() and it callls > > > > vxlan_fdb_destroy(). > > > > do_notify of vxlan_fdb_destroy() in vxlan_fdb_delete_default() is > > > > always true. > > > > So, failure path of __vxlan_dev_create() doesn't have any opportunity to > > > > handle do_notify. > > > > > > > > > I don't see register_netdevice calling ndo_uninit in case of all > > > errors. In the case where it does not, > > > does your patch leak the fdb entry ?. > > > > > > Wondering if we should just use vxlan_fdb_delete_default with a notify > > > flag to delete the entry if exists. > > > Will that help ? > > > > > > There is another commit that touched this code path: > > > commit 6db9246871394b3a136cd52001a0763676563840 > > > > > > Author: Petr Machata <[email protected]> > > > Date: Tue Dec 18 13:16:00 2018 +0000 > > > vxlan: Fix error path in __vxlan_dev_create() > > > > I have checked up failure path of register_netdevice(). > > Yes, this patch leaks fdb entry. > > There are 3 failure cases in the register_netdevice(). > > A. error occurs before calling ->ndo_init(). > > it doesn't call ->ndo_uninit(). > > B. error occurs after calling ->ndo_init(). > > it calls ->ndo_uninit() and dev->reg_state is NETREG_UNINITIALIZED. > > C. error occurs after registering netdev. it calls rollback_registered(). > > rollback_registered() internally calls ->ndo_uninit() > > and dev->reg_state is NETREG_UNREGISTERING. > > > > A panic due to these problem could be fixed by using > > vxlan_fdb_delete_default() with notify flag. > > But notification problem could not be fixed clearly > > because of the case C. > > yes, you are right. The notification issue still remains. > > > > > I don't have clear solution for the case C. > > Please let me know, if you have any good idea for fixing the case C. > > One option is a variant of fdb create. alloc the fdb but don't assign > it to the vxlan dev. > __vxlan_dev_create > create fdb entry > register_netdevice > rtnl_configure_link > link fdb to vxlan > fdb notify > > > Yet another option is moving fdb create after register_netdevice > __vxlan_dev_create > register_netdevice > rtnl_configure_link > create fdb entry > fdb notify > But if fdb create fails, user-space will see , NEWLINK + DELLINK when > creating a vxlan device and that seems weird.
Thank you for suggesting ideas! I think first one is very nice. It's simple and clear. So, I have been testing first one. I will send a new patch after more testing. Thank you again!
